core: add emulatedFormFactor setting by patrickhulce · Pull Request #6098 · GoogleChrome/lighthouse

patrickhulce · 2018-09-24T14:40:16Z

Summary
Introduces a new emulatedFormFactor enum to settings to eventually replace disableDeviceEmulation which is still respected. emulatedFormFactor defaults to mobile but can be one of mobile, desktop, or none. none is equivalent to setting disableDeviceEmulation to true.

Related Issues/PRs
closes #6092

patrickhulce · 2018-09-24T14:41:18Z

+const DESKTOP_EMULATION_METRICS = {
+  mobile: false,
+  screenWidth: 1366,
+  screenHeight: 768,


@paulirish it's not 100% clear to me what of these are necessary to zero-out any potential previous emulation 🤔

you can just trim it down to these:

const DESKTOP_EMULATION_METRICS = { mobile: false, width: 1366, height: 768, deviceScaleFactor: 1, };

the rest are reset on the call.

patrickhulce · 2018-09-24T14:47:21Z

      gatherMode?: boolean | string;
      disableStorageReset?: boolean;
      disableDeviceEmulation?: boolean;
+      emulatedFormFactor?: 'mobile'|'desktop'|'none';


makes me want to add a 'none' to throttlingMethod that's just an alias of 'provided' 😆

makes me want to add a 'none' to throttlingMethod that's just an alias of 'provided'

yes pls :D

I think we need to as it would be confusing if this one has a none but throttling does not.
Maybe add an alias provided here too? 🙄

brendankenny

LGTM!

We should probably bikeshed for at least a minute on emulated-form-factor and if it's obvious enough just from the flag name, but I have no strong opinion on it (yet :).

brendankenny · 2018-09-24T21:58:54Z

    const get = opts => Util.getEmulationDescriptions(opts).deviceEmulation;
    assert.equal(get({disableDeviceEmulation: true}), 'No emulation');
-    assert.equal(get({disableDeviceEmulation: false}), 'Emulated Nexus 5X');
+    assert.equal(get({emulatedFormFactor: 'none'}), 'No emulation');


Should this include the disableDeviceEmulation: false -> 'No emulation' case as well (since no emulatedFormFactor)? Admittedly Config won't let you get into this situation due to the emulatedFormFactor default, but it feels weird to leave out when testing just this file :)

brendankenny · 2018-09-24T22:01:19Z

            'Disable clearing the browser cache and other storage APIs before a run',
-        'disable-device-emulation': 'Disable Nexus 5X emulation',
+        'disable-device-emulation': 'Disable all device form factor emulation',
+        'emulated-form-factor': 'Controls the emulated device form factor (mobile vs. desktop)',


Should we also mention the flag precedence here, like "Controls the emulated device form factor (mobile vs. desktop) if emulation not disabled" or something. It gets weird that none also disables it, but it isn't too confusing...

(could also add something about Deprecated: use '--emulated-form-factor none'. to the disable-device-emulation description)

should also update the readme with new --help output

yeah good call, done

paulirish · 2018-09-24T22:20:53Z

+const DESKTOP_EMULATION_METRICS = {
+  mobile: false,
+  screenWidth: 1366,
+  screenHeight: 768,


you can just trim it down to these:

const DESKTOP_EMULATION_METRICS = { mobile: false, width: 1366, height: 768, deviceScaleFactor: 1, };

the rest are reset on the call.

paulirish · 2018-09-24T22:21:40Z

 const NEXUS5X_USERAGENT = {
  userAgent: 'Mozilla/5.0 (Linux; Android 6.0.1; Nexus 5 Build/MRA58N) AppleWebKit/537.36' +
-    '(KHTML, like Gecko) Chrome/66.0.3359.30 Mobile Safari/537.36',
+    '(KHTML, like Gecko) Chrome/69.0.3497.100 Mobile Safari/537.36',


i usually bump to canary. 71.0.3559.0
here and below.

paulirish · 2018-09-24T22:23:20Z

@@ -992,7 +992,11 @@ class Driver {
   */
  async beginEmulation(settings) {
    if (!settings.disableDeviceEmulation) {


add TODO somewhere to remove this flag in a breaking version?

paulirish · 2018-09-24T22:24:47Z

        'list-trace-categories', 'view', 'verbose', 'quiet', 'help',
      ])
      .choices('output', printer.getValidOutputOptions())
+      .choices('emulated-form-factor', ['mobile', 'desktop', 'none'])


let's set the default to mobile here in the CLI too?

core: add emulatedFormFactor setting

091f644

patrickhulce requested review from brendankenny and paulirish as code owners September 24, 2018 14:40

patrickhulce commented Sep 24, 2018

View reviewed changes

brendankenny approved these changes Sep 24, 2018

View reviewed changes

paulirish requested changes Sep 24, 2018

View reviewed changes

paulirish reviewed Sep 24, 2018

View reviewed changes

feedback

a6cd2df

paulirish approved these changes Sep 25, 2018

View reviewed changes

patrickhulce merged commit f5c043d into master Sep 26, 2018

patrickhulce deleted the form_factor_enum branch September 26, 2018 13:48

paulirish mentioned this pull request Jun 4, 2020

Reconfigure emulation flags #10910

Closed

Conversation

patrickhulce commented Sep 24, 2018

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!

brendankenny 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants