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

Useragentdata support #7378

Merged
merged 8 commits into from Jun 29, 2021
Merged

Conversation

rowan-m
Copy link
Contributor

@rowan-m rowan-m commented Jun 25, 2021

Context: #7269

Review requests

  • npm run doc complains about [MarkDown] Method Page.setUserAgent() userAgentData Object != UserAgentMetadata. Not really clear how to resolve. Other methods that are using a DevTools protocol type are also just referrring to Object.
  • Not sure if you have a preferred convention on setting an optional key in an array. I've erred on the side of being more verbose, but perhaps there's something more elegant to do.

Adds userAgentData to setUserAgent that supports specifying user agent
data for the new navigator.userAgentData and Client Hints headers.

Adds userAgentData to setUserAgent that supports specifying user agent
data for the new navigator.userAgentData and Client Hints headers.
@google-cla google-cla bot added the cla: yes label Jun 25, 2021
@rowan-m rowan-m marked this pull request as draft June 25, 2021 09:30
@mathiasbynens
Copy link
Member

Thanks for kicking this off, Rowan!

Jan and Alex, could you PTAL?

src/common/NetworkManager.ts Outdated Show resolved Hide resolved
@OrKoN
Copy link
Collaborator

OrKoN commented Jun 25, 2021

Not sure about npm run doc complains. Perhaps, @jschfflr or @jackfranklin know? For me locally, it complains about Method Page.emulateVisionDeficiency() type string != Object

@jschfflr
Copy link
Contributor

To fix the npm run doc command, you probably have to add an exception to utils/doclint/check_public_api/index.js.
We should probably improve the documentation around this!

@rowan-m
Copy link
Contributor Author

rowan-m commented Jun 25, 2021

add an exception to utils/doclint/check_public_api/index.js.

Will test this!

docs/api.md Outdated Show resolved Hide resolved
docs/api.md Outdated Show resolved Hide resolved
src/common/NetworkManager.ts Outdated Show resolved Hide resolved
@rowan-m rowan-m marked this pull request as ready for review June 25, 2021 14:31
@rowan-m
Copy link
Contributor Author

rowan-m commented Jun 25, 2021

@OrKoN @jschfflr I think your comments should be addressed now.

test/page.spec.ts Outdated Show resolved Hide resolved
test/page.spec.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@rowan-m rowan-m marked this pull request as draft June 25, 2021 17:55
@rowan-m
Copy link
Contributor Author

rowan-m commented Jun 25, 2021

Tests don't work, fixing.

@jschfflr
Copy link
Contributor

For the failing test you can use npm run eslint-fix to have the linter fix the empty line and add
// eslint-disable-next-line @typescript-eslint/ban-ts-comment above the // @ts-ignore line.

Correct checking of navigator.userAgentData values
@rowan-m
Copy link
Contributor Author

rowan-m commented Jun 28, 2021

Tests should now be passing.

@rowan-m rowan-m marked this pull request as ready for review June 28, 2021 10:41
@jschfflr
Copy link
Contributor

It seems like firefox doesn't support this yet - you can exclude this test from being run on FF by using itFailsFirefox instead of it.

@google-cla
Copy link

google-cla bot commented Jun 28, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Jun 28, 2021
@jschfflr
Copy link
Contributor

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Jun 28, 2021
@jschfflr
Copy link
Contributor

@rowan-m I updated the tests to pass but could you include the requested example that Mathias requested?

rowan-m and others added 2 commits June 29, 2021 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants