core(full-page-screenshot): add full page screenshot to artifacts by mattzeunert · Pull Request #9064 · GoogleChrome/lighthouse

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

+      await emulation.enableNexus5X(this, deviceMetricOverrides);
    } 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

+      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

+  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

+
+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

   * @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

+    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

+    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

+   * @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

+  {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

    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


  /**
   * @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

+        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


		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]

Conversation

mattzeunert commented May 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorjclark left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorjclark commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mattzeunert commented May 26, 2019 •

edited

Loading

connorjclark left a comment •

edited

Loading

connorjclark commented Sep 2, 2020 •

edited

Loading