-
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: add new base artifact, HostFormFactor #9923
Conversation
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.
tests? :)
LGTM otherwise
@@ -456,14 +456,15 @@ class GatherRunner { | |||
|
|||
const {emulatedFormFactor} = options.settings; | |||
// Whether Lighthouse was run on a mobile device (i.e. not on a desktop machine). | |||
const IsMobileHost = hostUserAgent.includes('Android') || hostUserAgent.includes('Mobile'); | |||
const HostFormFactor = /android|mobile/i.test(hostUserAgent) ? 'mobile' : 'desktop'; |
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.
paul called me names (not out loud, but in his head) for wanting to use a regex here
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.
so you want to use a regex. now you got 2 problems 3 problems, including name-calling.
🙃
case insensitivity is fine. CONFIDENT PROGRAMMING. WE"LL BE FINE.
- https://cs.chromium.org/chromium/src/components/cronet/android/java/src/org/chromium/net/impl/UserAgent.java?l=42&rcl=0efaa7ede88f9448f19193dc513e57d651cabe79
- https://cs.chromium.org/chromium/src/third_party/devtools-frontend/src/node_modules/useragent/lib/regexps.js?l=727&rcl=879d97b05dfe9f26ede820b0296b125d4a68e5ff
surely.
@@ -29,6 +29,8 @@ declare global { | |||
LighthouseRunWarnings: string[]; | |||
/** Whether the page was loaded on either a real or emulated mobile device. */ | |||
TestedAsMobileDevice: boolean; | |||
/** Device which Chrome is running on. */ | |||
HostFormFactor: 'desktop'|'mobile'; |
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.
TestedAsMobileDevice
HostUserAgent
NetworkUserAgent
HostFormFactor
emulatedFormFactor
deviceScreenEmulationMethod
https://www.theonion.com/taco-bells-five-ingredients-combined-in-totally-new-way-1819564909
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 are starting to feel more like util functions than artifacts, but the suggestion was popular so LGTM :)
Extracted from #9911.
Needed for #9713