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

Update tests for NavigatorID #3881

Merged
merged 2 commits into from
Oct 10, 2016
Merged

Update tests for NavigatorID #3881

merged 2 commits into from
Oct 10, 2016

Conversation

foolip
Copy link
Member

@foolip foolip commented Oct 6, 2016

@wpt-pr-bot
Copy link
Collaborator

Reviewers for this pull request are: @Ms2ger, @ayg, @gsnedders, @jdm, @jgraham, @plehegar, @sideshowbarker, @zcorpan, and @zqzhang.

@foolip
Copy link
Member Author

foolip commented Oct 6, 2016

How should the NavigatorID.worker.js variant be run? I think there'll be lots of failures, and I wanted to use that to maybe change the spec again.

@foolip
Copy link
Member Author

foolip commented Oct 6, 2016

For the window tests, I have verified that they all pass, except for Edge failing on the navigator.vendor test, where I expect "Google Inc." If this PR is merged I'll file an Edge issue to see if they'd go along with this. If not, another compatibility mode would be needed.

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 6, 2016

You may want to check if workers/interfaces/WorkerUtils/navigator/window-only.worker.js is redundant now.

@foolip
Copy link
Member Author

foolip commented Oct 6, 2016

Oh, so wptserve does it, filed https://crbug.com/653514

window-only.worker.js is indeed now redundant, will remove.

@foolip
Copy link
Member Author

foolip commented Oct 6, 2016

In http://w3c-test.org/submissions/3881/html/webappapis/system-state-and-capabilities/the-navigator-object/NavigatorID.worker, these tests fail:

  • Chrome 55: appCodeName and product
  • Edge 14: product, productSub, vendor and vendorSub
  • Firefox 51: appCodeName and product
  • Safari 10: appCodeName, product, productSub, vendor and vendorSub

Inverted:

  • appCodeName: all except Edge fail
  • product: all fail
  • productSub, vendor and vendorSub: Edge and Safari fail

Edit: The PR has changed since, the above will not reproduce.

foolip added a commit to whatwg/html that referenced this pull request Oct 6, 2016
Tests: web-platform-tests/wpt#3881

Attempting to write tests for the current spec revealed that product was
already exposed in all engines, and appCodeName in all but Edge.

This leaves productSub, vendor and vendorSub restricted, where there is
a 2/2 split between engines.

Follow up to #216.
@foolip
Copy link
Member Author

foolip commented Oct 6, 2016

I've submitted a spec PR to expose appCodeName and product and will now update the tests.

@foolip foolip force-pushed the update-NavigatorID branch from 06546bf to ade9e8c Compare October 6, 2016 13:09
@foolip
Copy link
Member Author

foolip commented Oct 6, 2016

This will leave Edge and WebKit with failures, will file bugs when spec and tests are merged.

@foolip
Copy link
Member Author

foolip commented Oct 7, 2016

@domenic, can you review?

// https://www.w3.org/Bugs/Public/show_bug.cgi?id=27820

test(function() {
if ("window" in self && navigator.userAgent.indexOf("WebKit") == -1) {
Copy link
Member

@domenic domenic Oct 7, 2016

Choose a reason for hiding this comment

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

This file uses several different ways of testing navigator compatibility mode, not all of which are compatible. E.g. here Gecko means "does not have the string WebKit", where above Gecko means "does not have the string WebKit or the string Chrome".

It would be good to factor this out into a variable (in the test file) that is one of "Chrome", "WebKit", or "Gecko", and then the test code proper could say if ("window" in self && navigatorCompatibilityMode === "Gecko"). Then you wouldn't need the spec quotes as much.

// Otherwise it should exist and return false.
else {
assert_false(navigator.taintEnabled());
if (navigator.userAgent.indexOf("Chrome") != -1 ||
Copy link
Member

@domenic domenic Oct 7, 2016

Choose a reason for hiding this comment

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

Nit: this could be navigator.userAgent.includes("Chrome") for clarity.

@foolip foolip assigned domenic and unassigned zcorpan Oct 10, 2016
@domenic domenic merged commit 40cc122 into master Oct 10, 2016
@domenic domenic deleted the update-NavigatorID branch October 10, 2016 15:30
foolip added a commit to whatwg/html that referenced this pull request Oct 10, 2016
alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants