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

Jump to bottom

core: add emulatedFormFactor setting #6098

Merged
merged 2 commits into from
Sep 26, 2018
Merged

core: add emulatedFormFactor setting #6098

merged 2 commits into from
Sep 26, 2018

Conversation

patrickhulce
Copy link
Collaborator

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
patrickhulce commented Sep 24, 2018
View reviewed changes
lighthouse-core/lib/emulation.js Outdated
const DESKTOP_EMULATION_METRICS = {
mobile: false,
screenWidth: 1366,
screenHeight: 768,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
patrickhulce commented Sep 24, 2018
View reviewed changes
typings/externs.d.ts
@@ -83,6 +83,7 @@ declare global {
gatherMode?: boolean | string;
disableStorageReset?: boolean;
disableDeviceEmulation?: boolean;
emulatedFormFactor?: 'mobile'|'desktop'|'none';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

yes pls :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

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
brendankenny approved these changes Sep 24, 2018
View reviewed changes
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

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 :).

lighthouse-core/test/report/html/renderer/util-test.js
@@ -94,7 +94,9 @@ describe('util helpers', () => {
it('builds device emulation string', () => {
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');
Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done :)

lighthouse-cli/cli-flags.js Outdated
@@ -70,7 +70,8 @@ function getFlags(manualArgv) {
'blocked-url-patterns': 'Block any network requests to the specified URL patterns',
'disable-storage-reset':
'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)',
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

should also update the readme with new --help output

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah good call, done

paulirish
paulirish requested changes Sep 24, 2018
View reviewed changes
lighthouse-core/lib/emulation.js Outdated
const DESKTOP_EMULATION_METRICS = {
mobile: false,
screenWidth: 1366,
screenHeight: 768,
Copy link
Member

Choose a reason for hiding this comment

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

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.

lighthouse-core/lib/emulation.js Outdated
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',
Copy link
Member

Choose a reason for hiding this comment

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

i usually bump to canary. 71.0.3559.0
here and below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

lighthouse-core/gather/driver.js
@@ -992,7 +992,11 @@ class Driver {
*/
async beginEmulation(settings) {
if (!settings.disableDeviceEmulation) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done 👍

paulirish
paulirish reviewed Sep 24, 2018
View reviewed changes
lighthouse-cli/cli-flags.js
@@ -120,6 +121,7 @@ function getFlags(manualArgv) {
'list-trace-categories', 'view', 'verbose', 'quiet', 'help',
])
.choices('output', printer.getValidOutputOptions())
.choices('emulated-form-factor', ['mobile', 'desktop', 'none'])
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done!

@patrickhulce patrickhulce merged commit f5c043d into master Sep 26, 2018
@patrickhulce patrickhulce deleted the form_factor_enum branch September 26, 2018 13:48
@paulirish paulirish mentioned this pull request Jun 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wardpeet wardpeet wardpeet left review comments

@brendankenny brendankenny brendankenny approved these changes

@paulirish paulirish paulirish approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Correctly emulate desktop in headless scenarios
4 participants
@patrickhulce @paulirish @brendankenny @wardpeet