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.

Sign up for GitHub

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

Jump to bottom

Expose navigator.appCodeName and navigator.product in Worker #1870

Merged
merged 1 commit into from
Oct 10, 2016
Merged

Expose navigator.appCodeName and navigator.product in Worker #1870

merged 1 commit into from
Oct 10, 2016

Conversation

foolip
Copy link
Member

@foolip foolip commented 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
Expose navigator.appCodeName and navigator.product in Worker
00c4b3c 
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

@cdumez. It looks like Safari began exposing productSub, vendor and vendorSub in Safari 10. Because this was until recently a 3/1 split, I've left those alone.

foolip added a commit to web-platform-tests/wpt that referenced this pull request Oct 6, 2016
@foolip
Update tests for NavigatorID
ade9e8c 
Aligns with these spec changes:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=27796
whatwg/html#78
whatwg/html#199
whatwg/html#207
whatwg/html#220
whatwg/html#1017
whatwg/html#1870
@foolip foolip mentioned this pull request Oct 6, 2016
Merged
@cdumez
Copy link

cdumez commented Oct 6, 2016

@foolip: Yes, I believe I worked on aligning WorkerNavigator with the specification for Safari 10.

@foolip
Copy link
Member Author

foolip commented Oct 6, 2016

Makes sense, if you didn't change NavigatorID at the same time. Are you OK with this change, and sprinkling some [Exposed=Window] over WebKit?

@cdumez
Copy link

cdumez commented Oct 6, 2016

I don't really mind exposing those 2 although they are constant and therefore kind of pointless. (Why bother exposing them?)

Note sure what you mean by sprinkling exposed=window since it seems like you want us to expose more things to workers and not restrict more things to Window.

@foolip
Copy link
Member Author

foolip commented Oct 6, 2016

@cdumez, right, I'm only removing [Exposed=Window] from two attributes that are already exposed in workers in Safari, but the three that are still [Exposed=Window] have also recently been exposed to workers in Safari, and the test I wrote will fail, see web-platform-tests/wpt#3881 and https://w3c-test.org/submissions/3881/html/webappapis/system-state-and-capabilities/the-navigator-object/NavigatorID.worker

@cdumez
Copy link

cdumez commented Oct 7, 2016

Ok, will fix WebKit via https://bugs.webkit.org/show_bug.cgi?id=163124

@domenic
Copy link
Member

domenic commented Oct 7, 2016

LGTM, if a bit sad. Do you plan to file bugs on Edge for appCodeName, and the other 2 browsers for productSub, vendor, and vendorSub?

kisg pushed a commit to paul99/webkit-mips that referenced this pull request Oct 7, 2016
Regression(r201970): productSub / vendor / vendorSub should not be ex…
4143da1 
…posed on WorkerNavigator

https://bugs.webkit.org/show_bug.cgi?id=163124

Reviewed by Ryosuke Niwa.

Source/WebCore:

productSub / vendor / vendorSub should not be exposed on WorkerNavigator:
- https://html.spec.whatwg.org/#navigatorid

Test case:
- http://w3c-test.org/submissions/3881/html/webappapis/system-state-and-capabilities/the-navigator-object/NavigatorID.worker

Note that the specification also restricts NavigatorID's appCodeName and
product attributes to Window. However, it seems the HTML specification is
about to get updated so that these get exposed to workers:
- whatwg/html#1870

No new tests, updated existing test.

* bindings/scripts/generate-bindings.pl:
(shouldPropertyBeExposed):
* page/NavigatorID.idl:

LayoutTests:

Update existing test to reflect behavior change.

* fast/workers/resources/worker-navigator.js:
* fast/workers/worker-navigator-expected.txt:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@206926 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@foolip
Copy link
Member Author

foolip commented Oct 10, 2016

Yes, I'll file an Edge bug when the test is landed, https://bugs.webkit.org/show_bug.cgi?id=163124 should suffice for WebKit.

domenic pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 10, 2016
@foolip @domenic
Update tests for NavigatorID
40cc122 
Aligns with these spec changes:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=27796
whatwg/html#78
whatwg/html#199
whatwg/html#207
whatwg/html#220
whatwg/html#1017
whatwg/html#1870
@foolip
Copy link
Member Author

foolip commented Oct 10, 2016

Edge issue filed: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/9275188/

@foolip foolip merged commit d57fa7d into master Oct 10, 2016
@foolip foolip deleted the navigator-id-worker branch October 10, 2016 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
normative change topic: workers
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@foolip @cdumez @domenic