-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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.
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
core(emulation): use Moto G4 for mobile emulation #10191
Conversation
lighthouse-core/lib/emulation.js
Outdated
positionX: 0, | ||
positionY: 0, | ||
scale: 1, | ||
deviceScaleFactor: 2.625, | ||
deviceScaleFactor: 2.625, // ?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what the heck is this @paulirish
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that should be the device DPR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
math checks out too 640 * 3 = 1920
, 360 * 3 = 1080
screenHeight: 660, | ||
width: 412, | ||
height: 660, | ||
screenWidth: 360, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to look at an actual device and see what a browser reports this as.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
360 looks right https://github.com/WPO-Foundation/webpagetest/blob/901d2906711c32577f9c8e19603cf491096fcbf8/www/settings/mobile_devices.ini#L7
could use a WPT run if you need to to verify
@@ -161,6 +160,6 @@ module.exports = { | |||
enableCPUThrottling, | |||
disableCPUThrottling, | |||
goOffline, | |||
MOBILE_USERAGENT: NEXUS5X_USERAGENT, | |||
MOBILE_USERAGENT: MOTOG4_USERAGENT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice thinking here on the export
assert.ok(/RTT/.test(descriptionsTxt), 'should have added network'); | ||
assert.ok(/\dx/.test(descriptionsTxt), 'should have added CPU'); | ||
assert.ok(descriptionsTxt.includes(sampleResults.userAgent), 'user agent populated'); | ||
expect(descriptionsTxt).toContain('Moto G4'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asserts are bad.
@@ -169,12 +169,12 @@ const expectations = [ | |||
}, | |||
}, | |||
'uses-responsive-images': { | |||
displayValue: 'Potential savings of 69\xa0KB', | |||
displayValue: 'Potential savings of 53\xa0KB', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these values are calculated based on DPR. They are proportional to DPR^2
.
3^2 / 2.625^2 = ~1.3
, so I divided each expectation by 1.3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
part of #9876