core(full-page-screenshot): drop max datauri size constraints #11689
Conversation
| } | ||
| } | ||
|
|
||
| let screenshot = await this._takeScreenshot(passContext); |
There was a problem hiding this comment.
nit: Can we remove the afterPass_ function and call _takeScreenshot directly?
| @@ -119,8 +103,6 @@ class FullPageScreenshot extends Gatherer { | |||
| return { | |||
| width: document.documentElement.clientWidth, | |||
| height: document.documentElement.clientHeight, | |||
| screenWidth: window.screen.width, | |||
There was a problem hiding this comment.
Was this necessary before this patch? Looks like screenWidth and screenHeight are not change in this gatherer.
There was a problem hiding this comment.
fair. i meant to include it in #11688 instead. just moved it there.
|
Did you test in other browsers? Also, this isn't just preventing a possibly faulty image, it also attempts to put a cap on how much data a screenshot will be (the soon-to-be largest part of the LHR). For that purpose, maybe we ought to just lessen the quality instead of capping the height? |
connorjclark
changed the title
tested with a 14.9MB data uri string. works great in chrome/saf/ff.
kinda frustrating.... so i feel confident i've put the max_datauri_size issue to bed. the comments/code were wrong and i've proven that. as for LHR data size.. agree the FPS is big. but, we don't have any criteria to determine an LHR is too big. So I don't see a great path forward in picking a threshold to retake a smaller screenshot. |
Yea, we're on the same page here. This change LGTM
Just pointing out the secondary effect of this change. If we don't want to address it now, at least we might recall this is a way to shrink an LHR in the future if a specific need arises. |
This reverts commit 269adc3.
(this PR is building off #11688 )
back here i discovered this data uri limit that was in the implementation wasn't a real limit. #11402 (comment) patrick confirmed.
I made another test here:
The chromium bug regarding that limit: https://bugs.chromium.org/p/chromium/issues/detail?id=69227 Yah as patrick said, it's likely this limit is about the data uri that you can navigate to in chrome. I definitely can't nav to a 2.5MB data uri. But it's totally fine as an
<img src>