core: add settings.internalDisableDeviceScreenEmulation #9377

paulirish · 2019-07-15T18:52:21Z

background: in https://chromium-review.googlesource.com/c/chromium/src/+/1699018 we found that audits panel has been double-emulating. We emulate on the devtools-side before LH starts.. and then due to some bugs in how we rolled out the config change of (disableDeviceEmulation => emulatedFormFactor), we emulated again inside LH.

We end up with odd emulation bugs when the devtools UI remains onscreen while LH runs the emulation show, so we think it's best to let devtools continue emulating. (Plus we get the attractive device frame)

~~Anyhow, the new TestedAsMobileDevice artifact doesn't account for this situation, but should.~~

Turns out that during a protocol-handoff the network emulation is lost. So we need LH to (re-)apply network emulation but not the viewport emulation. So I've added a new config setting SharedFlagsSetting.internalDisableDeviceScreenEmulation to indicate this.

DevTools will pass in emulatedFormFactor of either "mobile" or "desktop" along with internalDisableDeviceScreenEmulation: true.

paulirish · 2019-07-15T19:16:16Z

~~this is all broken. nevermind for now.~~

patrickhulce

LGTM! though some questions around the edge cases

lighthouse-core/gather/gather-runner.js

    const IsMobileHost = hostUserAgent.includes('Android') || hostUserAgent.includes('Mobile');
+    // IsMobilePage indicates whether the page is mobile (with any pre-applied mobile emulation or not)
+    // In DevTools, emulation is applied before Lighthouse starts (to deal with viewport emulation bugs)
+    // emulatedFormFactor will be 'none', but it's certainly still TestedAsMobileDevice


lighthouse-core/gather/gather-runner.js

+    const IsMobilePage = pageUserAgent.includes('Android') || pageUserAgent.includes('Mobile');
    const TestedAsMobileDevice = emulatedFormFactor === 'mobile' ||
-      (emulatedFormFactor !== 'desktop' && IsMobileHost);
+      (emulatedFormFactor !== 'desktop' && IsMobileHost) ||


lighthouse-core/test/gather/fake-driver.js

      return Promise.resolve(Object.assign({}, protocolGetVersionResponse, {milestone: 71}));
    },
+    getPageUserAgent() {
+      return Promise.resolve(driverGetPageUAResponse);


lighthouse-core/test/gather/gather-runner-test.js

      expect(results.TestedAsMobileDevice).toBe(false);
    });
+
+    it('works when running via DevTools with emulation applied outside of LH', async () => {


paulirish · 2019-07-15T20:46:20Z

Reworked this completely. Ready for a fresh look.

patrickhulce

love the parallelism with throttlingMethod 😍 , provided feels exactly like what we want here 👍

🚲 🏠 on the name

viewportEmulationMethod
screenEmulationMethod
formFactorScreenEmulationMethod
emulatedFormFactorScreenOverrideMethod

I think I like deviceScreenEmulationMethod best still, but the only bummer is it's not super obvious on first glance that it's so closely related to the emulatedFormFactor

lighthouse-core/gather/gather-runner.js

    const hostUserAgent = (await options.driver.getBrowserVersion()).userAgent;
-
    const {emulatedFormFactor} = options.settings;
+


patrickhulce · 2019-07-15T21:00:17Z

lighthouse-core/lib/emulation.js

 /**
+ *
 * @param {Driver} driver
- * @return {Promise<void>}


could leave a @return {Promise<void>} on the new function FWIW :)

patrickhulce · 2019-07-15T21:01:24Z

lighthouse-core/lib/emulation.js

+  if (settings.deviceScreenEmulationMethod !== 'provided') {
+    promises.push(
+      ...[
+        driver.sendCommand('Emulation.setDeviceMetricsOverride', params.metrics),


do these take that long? I feel like this function would be much easier to read if we just await'd each as they came

patrickhulce · 2019-07-15T21:02:42Z

lighthouse-core/lib/emulation.js

-  await Promise.all([
-    driver.sendCommand('Emulation.setDeviceMetricsOverride', NEXUS5X_EMULATION_METRICS),
+async function emulate(driver, settings) {
+  let params;


is this falling back to any or is ts smart enough to set this properly without explicit typing?

changed up this and got nice typing for it.

patrickhulce · 2019-07-15T21:03:32Z

lighthouse-core/lib/emulation.js

-  ]);
+    driver.sendCommand('Network.setUserAgentOverride', {userAgent: params.userAgent}),
+  ];
+  if (settings.deviceScreenEmulationMethod !== 'provided') {


should we explicitly check 'devtools' since it should be a default by this point?

it means adding the deviceScreenEmulationMethod prop to a bunch of gather-runner-test tests. but ok! done.

patrickhulce · 2019-07-15T21:04:08Z

proto/lighthouse-result.proto

+      // Unknown screen emulation method.  This should not be used
+      UNKNOWN_SCREEN_EMULATION_METHOD = 0;
+
+      // DevTools mobile/desktop viewport emulation shoudl be applied.


Suggested change

// DevTools mobile/desktop viewport emulation shoudl be applied.

// DevTools mobile/desktop viewport emulation should be applied.

patrickhulce · 2019-07-15T21:04:28Z

proto/lighthouse-result.proto

+
+    // The possible form factors an audit can be run in
+    enum DeviceScreenEmulationMethod {
+      // Unknown screen emulation method.  This should not be used


Suggested change

// Unknown screen emulation method. This should not be used

// Unknown screen emulation method. This should not be used.

lighthouse-core/lib/emulation.js

patrickhulce · 2019-07-15T21:12:24Z

lighthouse-core/test/gather/gather-runner-test.js

+      const options = {requestedUrl, driver, settings: config.settings};
+
+      const results = await GatherRunner.run(config.passes, options);
+      expect(results.TestedAsMobileDevice).toBe(true);


this test feels a tad hollow when the TestedAsMobileDevice is based on the mocked result of getBrowserVersion which is not affected by our deviceScreenEmulationMethod here :)

another case for some separate emulation-test tests

fair.
new emulation-test added.

patrickhulce · 2019-07-15T21:12:31Z

lighthouse-core/test/gather/gather-runner-test.js

      endTrace: asyncFunc,
      endDevtoolsLog: () => [],
      getBrowserVersion: async () => ({userAgent: ''}),
+      getPageUserAgent: async () => '',


not needed anymore

patrickhulce

lookin' great! CLI snapshots need an update for travis.

any thought to the naming bikeshed?

patrickhulce · 2019-07-17T21:09:24Z

lighthouse-core/lib/emulation.js

+  // UA emulation, however, is lost in the protocol handover from devtools frontend to the audits_worker. So it's always applied.
+
+  // Network.enable must be called for UA overriding to work
+  await driver.sendCommand('Network.enable');


beautiful ❤️

patrickhulce · 2019-07-17T21:10:59Z

lighthouse-core/test/lib/emulation-test.js

+'use strict';
+
+const emulation = require('../../lib/emulation.js');
+const {getMockedEmulationDriver} = require('../gather/gather-runner-test.js');


this is within the scope to refactor when the other mock test utils lands, right?

oh yeah. i'm dying to do that.

yeah that's gonna be great :D always weird to have test files export junk

patrickhulce · 2019-07-17T21:12:58Z

lighthouse-core/test/lib/emulation-test.js

+      await emulation.emulate(driver, getSettings('mobile', 'devtools'));
+      expect(setUAFn).toBeCalled();
+      expect(deviceMetricsFn).toBeCalled();
+      expect(getFnCallArgs(setUAFn)[0]).toMatchObject({userAgent: emulation.MOBILE_USERAGENT});


should this just get this first argument if it's going to exist? all the usages seem to use it that way.

setUAFn.mock.calls[0][0] is actually 1 fewer character than getFnCallArgs(setUAFn)[0] too btw ;)

patrickhulce · 2019-07-17T21:15:20Z

lighthouse-core/test/lib/emulation-test.js

+    });
+
+    it('handles: emulatedFormFactor: mobile / deviceScreenEmulationMethod: devtools', async () => {
+      await emulation.emulate(driver, getSettings('mobile', 'devtools'));


FWIW, I've come to deeply regret all the helper creator functions I made like this in the past and the below doesn't seem so bad 🤷‍♂

await emulation.emulate(driver, { emulatedFormFactor: 'mobile', deviceScreenEmulationMethod: 'devtools', });

:)

exterkamp · 2019-07-17T22:19:08Z

proto/lighthouse-result.proto

    string channel = 4;
+
+    // The possible form factors an audit can be run in
+    enum DeviceScreenEmulationMethod {


Is this really necessary in proto? LR will always use one option? Similar to throttling settings is always simulated? We elide much of config for this reason iirc.

nah. we can leave out of the proto for now. i forgot that we only did a subset.

paulirish · 2019-07-17T22:40:03Z

any thought to the naming bikeshed?

okay i can roll with these ones. They're all decent.

emulatedViewportMethod - get that "emulated" prefix to associated with eFormFactor
viewportEmulationMethod - you also proposed. I like this better than the current deviceScreenEmulationMethod
screenEmulationMethod - better than dSEM (which has an extra 'device' prefix)

exterkamp

LGTM proto 💯

connorjclark · 2019-07-19T21:04:37Z

This is a blocker for 5.2 (any roll to DT w/o this will be broken).

paulirish · 2019-11-06T04:05:46Z

wow github got angry with my merge.

all sorted.

googlebot · 2019-11-06T04:06:14Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

brendankenny · 2019-11-06T04:16:53Z

but at this point i don't know how our TestedAsMobileDevice logic would work, because someone can configure something that's definitely nonbinary.

given that i think we need to maintain control of the story.

that's convincing

This would also unlock what PaulKinlan is looking for re: feature phones.

FWIW, this use case doesn't exist anymore, so w/e about it :)

In short, this is a crazy setting that nobody except for us (in our odd devtools emulation story) will need. I'm sure of it.

I don't believe this :P but more seriously, I don't like that adding a settings.internal makes it easier to add these sorts of things in the future. I'd prefer internal just as a variable name prefix, but one question:

is !settings.internal.disableDeviceScreenEmulation always equal to to settings.channel === 'devtools' now? If so, maybe we should just use that.

stale

brendankenny · 2019-11-06T04:33:07Z

is !settings.internal.disableDeviceScreenEmulation always equal to to settings.channel === 'devtools' now? If so, maybe we should just use that.

eh, I'm not entirely sure how to feel

On the one hand, it is equivalent and we're replicating bits with no new information in them. We're down to three LH clients again and it's silly to pretend we support every possible permutation of our options when actually (as you point out), we have some pretty strict requirements of redundancies between them. Better to just specifically configure things for each client.

On the other hand, channel makes for gross mixing of responsibilities instead of decoupled modules with self describing interfaces. Doing it the settings.internalDisableScreenEmulation way means that if DevTools ever changes in functionality substantially, we'd have to hunt down all the channel === 'devtools' checks, debugging each one, rather than just setting new options and not worrying about the side effects of keeping channel set to 'devtools'. At worst we'd be stuck with an internalDisableScreenEmulation that no one was actually using until someone notices that and deletes it.

So I guess my vote is actually settings.internalDisableScreenEmulation but no settings.internal = {} :)

googlebot · 2019-11-06T04:57:49Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

paulirish · 2019-11-06T04:58:16Z

So I guess my vote is actually settings.internalDisableScreenEmulation but no settings.internal = {} :)

WFM!

thanks

Lighthouse master has breaking pre-6.0 changes, so we are cherry-picking a few things on top of 5.6.0. Cherry-picks from master: - d53284cf12e680cbe6a12429cc1c6c4219e7ef43 icu error - 466bd6f79e57950d736de5edfa9642a40f1100fd verbose i18n - 7c16d346593d86b33945d75a996829e1a25b4fed flatten PRs not yet in master, but merged into 5.7.0:: - GoogleChrome/lighthouse#9377 internalDisableDeviceScreenEmulation - GoogleChrome/lighthouse#9936 Bundling pubads Screenshot: https://imgur.com/a/XuHkgWk Bug: 772558 Change-Id: I4a03965ec879119a88faf40dbdca471a9a33e794 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/1900472 Commit-Queue: Connor Clark <[email protected]> Commit-Queue: Paul Irish <[email protected]> Reviewed-by: Paul Irish <[email protected]>

connorjclark · 2019-11-19T02:10:27Z

🎉

paulirish added 3 commits July 15, 2019 11:29

core(driver): grab signal that page is already in a mobile state

6290a09

tests

aba011f

comments

eab2dc6

paulirish requested a review from patrickhulce July 15, 2019 18:52

paulirish closed this Jul 15, 2019

patrickhulce reviewed Jul 15, 2019

View reviewed changes

paulirish added 2 commits July 15, 2019 13:21

hello deviceScreenEmulationMethod

95ca0f9

test cleanup

38f984b

paulirish reopened this Jul 15, 2019

paulirish requested review from brendankenny and exterkamp as code owners July 15, 2019 20:38

googlebot added the cla: yes label Jul 15, 2019

paulirish changed the title ~~core(driver): TestedAsMobileDevice considers a pre-applied emulation~~ core(driver): add flags.deviceScreenEmulationMethod Jul 15, 2019

paulirish changed the title ~~core(driver): add flags.deviceScreenEmulationMethod~~ core: add flags.deviceScreenEmulationMethod Jul 15, 2019

revert all changes to gather-runner

d7d55e9

patrickhulce reviewed Jul 15, 2019

View reviewed changes

paulirish added 3 commits July 17, 2019 13:32

feedback

c8cb9ff

tests. hello emulation-test.

f72c1bd

lint

43a5163

vercel bot deployed to staging July 17, 2019 20:34 View deployment

patrickhulce reviewed Jul 17, 2019

View reviewed changes

exterkamp requested changes Jul 17, 2019

View reviewed changes

feedback

c8ad5ff

vercel bot deployed to staging July 17, 2019 22:34 View deployment

exterkamp approved these changes Jul 17, 2019

View reviewed changes

connorjclark added the 5.2 label Jul 19, 2019

paulirish force-pushed the pageua branch from 741955f to 4729930 Compare November 6, 2019 04:05

vercel bot deployed to staging November 6, 2019 04:05 View deployment

paulirish added cla: yes and removed cla: no waiting4committer labels Nov 6, 2019

paulirish changed the title ~~core: add flags.deviceScreenEmulationMethod~~ core: add settings.internal.disableScreenEmulation Nov 6, 2019

prefixed prop name

7fc5070

vercel bot deployed to staging November 6, 2019 04:57 View deployment

googlebot added cla: no and removed cla: yes labels Nov 6, 2019

paulirish changed the title ~~core: add settings.internal.disableScreenEmulation~~ core: add settings.internalDisableScreenEmulation Nov 6, 2019

connorjclark mentioned this pull request Nov 7, 2019

bundling pubads for devtools #9936

Merged

connorjclark changed the title ~~core: add settings.internalDisableScreenEmulation~~ core: add settings.internalDisableDeviceScreenEmulation Nov 7, 2019

connorjclark added the 6.0 label Nov 8, 2019

connorjclark added 2 commits November 18, 2019 17:10

Merge remote-tracking branch 'origin/master' into pageua

f88f0bf

update snapshots

957938a

vercel bot deployed to staging November 19, 2019 01:11 View deployment

paulirish merged commit 648248e into master Nov 19, 2019

paulirish deleted the pageua branch November 19, 2019 02:05

paulirish mentioned this pull request Jun 4, 2020

Reconfigure emulation flags #10910

Closed

		const hostUserAgent = (await options.driver.getBrowserVersion()).userAgent;

		const {emulatedFormFactor} = options.settings;

	// DevTools mobile/desktop viewport emulation shoudl be applied.
	// DevTools mobile/desktop viewport emulation should be applied.

	// Unknown screen emulation method. This should not be used
	// Unknown screen emulation method. This should not be used.

core: add settings.internalDisableDeviceScreenEmulation #9377

core: add settings.internalDisableDeviceScreenEmulation #9377

Uh oh!

Conversation

paulirish commented Jul 15, 2019 • edited by connorjclark Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulirish commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

paulirish commented Jul 15, 2019

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

paulirish commented Jul 15, 2019 •

edited by connorjclark

Loading

paulirish commented Jul 15, 2019 •

edited

Loading

brendankenny commented Nov 6, 2019 •

edited

Loading