core(full-page-screenshot): add full page screenshot to artifacts #9064

mattzeunert · 2019-05-26T09:10:07Z

Capture a screenshot of the entire page, rather than just the above the fold content.

Splitting this off from this PR which actually uses the artifact.

What's the best way to test this? Add a new smoke test and unit test the gatherer with a mocked driver?

Related Issues/PRs

Part of #7327

evenstensberg · 2019-05-27T01:57:17Z

lighthouse-core/gather/driver.js

    } else if (settings.emulatedFormFactor === 'desktop') {
-      await emulation.enableDesktop(this);
+      await emulation.enableDesktop(this, deviceMetricOverrides);


Would it be appropriate to hook the options on the driver itself?

What do you mean by that?

If the extra param could be removed in favor for having it as a prop on this

I'm not sure we want to clutter driver with additional properties just for the method here. It's a pretty busy class already 😆

patrickhulce

Looks pretty good! I might argue that if we're commenting out the audit we should probably not land the gatherer config change either, but most of this depends on how quickly we'll follow this up with user-facing changes to leverage this.

patrickhulce · 2019-05-29T13:49:03Z

lighthouse-core/gather/gatherers/full-page-screenshot.js

+      data,
+      width,
+      height,
+      // todo: do we need these? should we create a new type? should we make them optional on the existing type? should we try to provide accurate values?


IMO we should just nix these from the type

Sounds good. It makes sense to have the timings for the filmstrip, but Filmstrip is a separate type from Screenshot.

patrickhulce · 2019-05-29T13:50:12Z

lighthouse-core/gather/gatherers/full-page-screenshot.js

+  async _takeScreenshot(passContext, maxScreenshotHeight) {
+    const driver = passContext.driver;
+    const metrics = await driver.sendCommand('Page.getLayoutMetrics');
+    const width = await driver.evaluateAsync(`window.innerWidth`) * 2;


why * 2?

I thought we might want innerWidth * devicePixelRatio but then we force the deviceScaleFactor to 1 below so I'm not sure anymore

Whoops, that slipped in when testing large screenshots.

patrickhulce · 2019-05-29T13:51:50Z

lighthouse-core/gather/gatherers/full-page-screenshot.js

+
+const Gatherer = require('./gatherer.js');
+
+// JPEG quality setting


can we link to that awesome exploration doc you made for how we landed on 30?

patrickhulce · 2019-05-29T13:55:47Z

lighthouse-core/gather/driver.js

   * @return {Promise<void>}
   */
-  async beginEmulation(settings) {
+  async beginEmulation(settings, deviceMetricOverrides) {


my initial thought looking at this was that I was worried about emulatedFormFactor conflicting with deviceMetricOverrides, maybe add a brief comment explaining the use case for deviceMetricOverrides?

creating a nexus 5x with arbitrary screen height seems counter-intuitive at first :)

patrickhulce · 2019-05-29T13:57:14Z

lighthouse-core/gather/gatherers/full-page-screenshot.js

+    const height = Math.min(metrics.contentSize.height, maxScreenshotHeight);
+
+    await driver.beginEmulation(passContext.settings, {
+      height,


mostly talking to the air here did we ever figure out what screenHeight does separately from height? desktop doesn't set screenHeight at the moment at all just wonder if that'll impact anything

it sets the screen rect but not sure what impact that has...
https://cs.chromium.org/chromium/src/content/renderer/render_widget_screen_metrics_emulator.cc?type=cs&sq=package:chromium&g=0&l=78-83

seems like it notifies some observers and kicks off orientation changes, but doesn't actually affect viewport size?
https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?type=cs&sq=package:chromium&g=0&l=2183

🤷‍♂

screen size is the physical device size (DIP): https://cs.chromium.org/chromium/src/third_party/blink/public/web/web_device_emulation_params.h?l=26&rcl=c6f1509e8c13f8b0739c99b343d628a38f8a9da7

connorjclark

FYI: command for viewing fullscreen image (after you run LH with -G)

(echo -e '<img src=' && (jq .FullPageScreenshot.data latest-run/artifacts.json | tr -d '\n') && echo -e '>') | cat > /tmp/lhfull.html && google-chrome /tmp/lhfull.html

connorjclark · 2019-08-29T00:55:25Z

lighthouse-core/gather/gatherers/full-page-screenshot.js

+    const height = Math.min(metrics.contentSize.height, maxScreenshotHeight);
+
+    await driver.beginEmulation(passContext.settings, {
+      height,


screen size is the physical device size (DIP): https://cs.chromium.org/chromium/src/third_party/blink/public/web/web_device_emulation_params.h?l=26&rcl=c6f1509e8c13f8b0739c99b343d628a38f8a9da7

connorjclark · 2019-08-29T01:16:20Z

lighthouse-core/gather/gatherers/full-page-screenshot.js

+   * @return {Promise<LH.Artifacts.FullPageScreenshot>}
+   */
+  async afterPass(passContext) {
+    let screenshot = await this._takeScreenshot(passContext, MAX_SCREENSHOT_HEIGHT);


should we scroll to the top of the page first? I think devtools does that.

connorjclark · 2019-08-29T01:17:12Z

lighthouse-core/lib/emulation.js

+  {deviceMetrics, setTouchEmulationEnabled, userAgent, deviceMetricOverrides}
+) {
+  if (deviceMetricOverrides) {
+    deviceMetrics = Object.assign({}, deviceMetrics, deviceMetricOverrides);


Suggested change

deviceMetrics = Object.assign({}, deviceMetrics, deviceMetricOverrides);

deviceMetrics = {...deviceMetrics, ...deviceMetricOverrides};

connorjclark · 2019-08-29T01:18:38Z

lighthouse-core/lib/emulation.js

    // Network.enable must be called for UA overriding to work
    driver.sendCommand('Network.enable'),
-    driver.sendCommand('Network.setUserAgentOverride', {userAgent: NEXUS5X_USERAGENT}),
-    driver.sendCommand('Emulation.setTouchEmulationEnabled', {enabled: true}),
+    driver.sendCommand('Network.setUserAgentOverride', {userAgent}),


should we be balancing domain enable / disable?

connorjclark · 2019-08-29T01:22:42Z

lighthouse-core/gather/driver.js

@@ -1423,13 +1423,15 @@ class Driver {

  /**
   * @param {LH.Config.Settings} settings
+   * @param {{height?: number, screenHeight: number?, deviceScaleFactor?: number}} [deviceMetricOverrides]


Suggested change

* @param {{height?: number, screenHeight: number?, deviceScaleFactor?: number}} [deviceMetricOverrides]

* @param {{height?: number, screenHeight?: number, deviceScaleFactor?: number}} [deviceMetricOverrides]

connorjclark · 2019-08-29T01:25:15Z

lighthouse-core/test/gather/gatherers/full-page-screenshot-test.js

+        height: 5000,
+        screenHeight: 5000,
+      },
+    ]);


make expectations on the artifact too?

connorjclark · 2020-09-02T17:35:01Z

we did this in #10716

Capture full-page screenshot

2ab034b

mattzeunert requested review from brendankenny, patrickhulce and paulirish as code owners May 26, 2019 09:10

evenstensberg reviewed May 27, 2019

View reviewed changes

patrickhulce reviewed May 29, 2019

View reviewed changes

mattzeunert added 3 commits May 29, 2019 19:42

Remove * 2

f53b83c

Link jpeg quality exploration

4d03200

Comment

c9f1593

googlebot added the cla: yes label May 29, 2019

vercel bot deployed to staging May 29, 2019 18:58 View deployment

Remove timing/timestamp from screenshots

012b81e

mattzeunert requested a review from exterkamp as a code owner May 30, 2019 08:58

vercel bot deployed to staging May 30, 2019 08:58 View deployment

Add fps gatherer tests

7125d42

vercel bot deployed to staging May 31, 2019 06:55 View deployment

mattzeunert mentioned this pull request May 31, 2019

WIP: Element screenshots #8797

Closed

6 tasks

paulirish self-assigned this Jun 11, 2019

connorjclark requested changes Aug 29, 2019

View reviewed changes

connorjclark mentioned this pull request Nov 24, 2019

Incorporate final screenshot in report #10016

Closed

connorjclark closed this Sep 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(full-page-screenshot): add full page screenshot to artifacts #9064

core(full-page-screenshot): add full page screenshot to artifacts #9064

mattzeunert commented May 26, 2019 •

edited

Loading

evenstensberg May 27, 2019

mattzeunert May 27, 2019

evenstensberg May 27, 2019

patrickhulce May 27, 2019

patrickhulce left a comment

patrickhulce May 29, 2019

mattzeunert May 30, 2019

patrickhulce May 29, 2019

mattzeunert May 29, 2019

patrickhulce May 29, 2019

patrickhulce May 29, 2019

patrickhulce May 29, 2019

connorjclark Aug 29, 2019

connorjclark left a comment •

edited

Loading

connorjclark Aug 29, 2019

connorjclark Aug 29, 2019

connorjclark Aug 29, 2019

connorjclark Aug 29, 2019

connorjclark Aug 29, 2019

connorjclark Aug 29, 2019

connorjclark commented Sep 2, 2020 •

edited

Loading


		const Gatherer = require('./gatherer.js');

		// JPEG quality setting

	deviceMetrics = Object.assign({}, deviceMetrics, deviceMetricOverrides);
	deviceMetrics = {...deviceMetrics, ...deviceMetricOverrides};

	* @param {{height?: number, screenHeight: number?, deviceScaleFactor?: number}} [deviceMetricOverrides]
	* @param {{height?: number, screenHeight?: number, deviceScaleFactor?: number}} [deviceMetricOverrides]

core(full-page-screenshot): add full page screenshot to artifacts #9064

core(full-page-screenshot): add full page screenshot to artifacts #9064

Conversation

mattzeunert commented May 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Sep 2, 2020 • edited Loading

mattzeunert commented May 26, 2019 •

edited

Loading

connorjclark left a comment •

edited

Loading

connorjclark commented Sep 2, 2020 •

edited

Loading