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.

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

feat: add browser type to device descriptors #3731

Conversation

mxschmitt
Copy link
Member

Found two cases which were not directly clear to me:

  • Samsung Galaxy S3 Android Browser 4: Already Chromium or still WebKit?
  • Nokia Lumia 520: In Theory IE10. I would drop it since its a device from April 2013

Also not sure if we want to keep it directly associated in the device descriptors, because that would maybe lead to breaking the backwards compatibility since preferredBrowserType is not directly related to the BrowserContextOptions.

Fixes #3728

@pavelfeldman
Copy link
Member

Samsung Galaxy S3 Android Browser 4: Already Chromium or still WebKit?
Nokia Lumia 520: In Theory IE10. I would drop it since its a device from April 2013

I think we should remove those ^^ from the list. Along with other devices older than 5 years old. Just to keep it in a good shape...

@pavelfeldman
Copy link
Member

since preferredBrowserType is not directly related to the BrowserContextOptions

That's a good point. But I guess it is ok-ish. Our options are:

  • Add the property as in the patch, a bit confusing, but works.
    • Optionally throw in case device and browser are incompatible
  • Strip the property when user reaches for devices, do not throw

@dgozman wdyt?

@pavelfeldman
Copy link
Member

Let's call it defaultBrowserType and leave your patch as is.

src/server/types.ts Outdated Show resolved Hide resolved
src/server/types.ts Outdated Show resolved Hide resolved
@mxschmitt mxschmitt force-pushed the feature/device-descriptors-browser-type branch 4 times, most recently from 218ee45 to 5eb8ddd Compare September 3, 2020 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] add preferred browser type to the device types
3 participants