Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(full-page-screenshot): get the MAX_TEXTURE_SIZE from the browser #11847

Merged
merged 8 commits into from
Jan 13, 2021
Merged

core(full-page-screenshot): get the MAX_TEXTURE_SIZE from the browser #11847

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f19d171
core(full-page-screenshot): determine the MAX_TEXTURE_SIZE from the b…
paulirish Dec 17, 2020
09b4027
Update lighthouse-core/gather/gatherers/full-page-screenshot.js
paulirish Dec 21, 2020
ce7680a
set fallback. destroy for gc
paulirish Dec 21, 2020
ab9de8c
use driver.evaluate()
paulirish Jan 12, 2021
8c61f9d
drop c8 ignore thang
paulirish Jan 12, 2021
3e9a9a8
use fn name instead
paulirish Jan 12, 2021
93d36fc
Merge remote-tracking branch 'origin/master' into maxtexturesize
paulirish Jan 12, 2021
17f7f04
lint
paulirish Jan 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lighthouse-core/gather/gatherers/full-page-screenshot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ class FullPageScreenshot extends Gatherer {
* @see https://bugs.chromium.org/p/chromium/issues/detail?id=770769
*/
async getMaxScreenshotHeight(driver) {
return await driver.evaluateAsync(`(${pageFunctions.getMaxTextureSize.toString()})()`, {
return await driver.evaluate(pageFunctions.getMaxTextureSize, {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious why a page function and not local to this module?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly because theres a lot of ways in which PFs are very diff than node code and so i'd like to ideally have all PFs live in separate files.

  • code coverage exemption (istanbul ignore/c8 ignore) is easier since they're colocated
  • can apply a eslint-env browser instead of our current handcrafted /* global window document Node ShadowRoot */ list.
  • maybe use some browserish config for typescript on these files

--

but until all are migrated there i'll just place new ones in pf.js. thats my thinking.

i don't care a lot about this particular case tho.

Copy link
Collaborator

@connorjclark connorjclark Jan 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but until all are migrated there i'll just place new ones in pf.js. thats my thinking.

Funny, I have the exact opposite thinking. Not speaking to the idea at all, but if we are currently doing "all page functions local if possible", we should keep doing that until we change how we structure things, instead of starting a transitionary thing w/o any discussion.

fwiw i like your bullet points

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

word.

well... looking at the fn's in pf.js, there already are plenty that are one-offs used by gatherrunner & driver. oh and a singleuse for iframe-elements! :)

args: [],
useIsolation: true,
deps: [],
});
}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/lib/page-functions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ function getOuterHTMLSnippet(element, ignoreAttrs = [], snippetCharacterLimit =
* Get the maximum size of a texture the GPU can handle
* @see https://bugs.chromium.org/p/chromium/issues/detail?id=770769#c13
*/
/* istanbul ignore next */ // todo, c8 ignore?
/* istanbul ignore next */
function getMaxTextureSize() {
try {
let canvas = document.createElement('canvas');
Expand Down
14 changes: 6 additions & 8 deletions lighthouse-core/test/gather/gatherers/full-page-screenshot-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,12 @@ const maxTextureSizeMock = 1024 * 8;
*/
function createMockDriver({contentSize, screenSize, screenshotData}) {
return {
evaluateAsync: async function(code) {
if (code === 'window.innerWidth') {
return contentSize.width;
}
if (code.includes('MAX_TEXTURE_SIZE')) {
return maxTextureSizeMock;
}
if (code.includes('document.documentElement.clientWidth')) {
evaluate: async function(fn) {
if (fn.name === 'resolveNodes') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this so much nicer

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fn.name is pretty tight. :) good call.

return {};
} if (fn.name === 'getMaxTextureSize') {
return maxTextureSizeMock;
} else if (fn.name === 'getObservedDeviceMetrics') {
return {
width: screenSize.width,
height: screenSize.height,
Expand Down