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

implements NavigatorLanguage #10009

Merged
merged 1 commit into from Mar 18, 2016
Merged

implements NavigatorLanguage #10009

merged 1 commit into from Mar 18, 2016

Conversation

creativcoder
Copy link
Contributor

Fixes #9992

This change is Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 15, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented Mar 15, 2016

Tidy is a little grumpy:

./components/script/dom/navigator.rs:68: trailing whitespace
./components/script/dom/navigator.rs:69: link to WHATWG may break in the future, use this format instead: https://html.spec.whatwg.org/multipage/#navigatorlanguage
./components/script/dom/webidls/Navigator.webidl:29: link to WHATWG may break in the future, use this format instead: https://html.spec.whatwg.org/multipage/#navigatorlanguage

jdm
jdm reviewed Mar 15, 2016
View reviewed changes
tests/wpt/metadata/html/dom/interfaces.html.ini
@@ -7554,9 +7551,6 @@
[SharedWorker interface: attribute onerror]
expected: FAIL

[WorkerNavigator interface: attribute language]
Copy link
Member

Choose a reason for hiding this comment

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

I am surprised by this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, i see. So this need not change here. Okay.

@jdm jdm assigned jdm and unassigned wafflespeanut Mar 15, 2016
@jdm
Copy link
Member

jdm commented Mar 15, 2016

@bors-servo: r+
Thanks!

@bors-servo
Copy link
Contributor

📌 Commit 58c4eb8 has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 15, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit 58c4eb8 with merge 13a3c03...

bors-servo pushed a commit that referenced this pull request Mar 15, 2016
Auto merge of #10009 - creativcoder:impl-navigator-language, r=jdm
13a3c03 
implements NavigatorLanguage

For #9992

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10009)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-wpt

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Mar 15, 2016
@wafflespeanut
Copy link
Contributor 

▶ Unexpected subtest result in /html/dom/interfaces.html:
└ PASS [expected FAIL] Navigator interface: window.navigator must inherit property "language" with the proper type (7)

▶ Unexpected subtest result in /workers/interfaces/WorkerUtils/navigator/language.html:
│ FAIL [expected PASS] navigator.language
│   → assert_equals: expected (string) "en-US" but got (undefined) undefined
│ 
│ worker.onmessage<@http://web-platform.test:8000/workers/interfaces/WorkerUtils/navigator/language.html:4:5
│ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1381:20
└ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1405:1

@creativcoder
Copy link
Contributor Author

First test case, was resolved by removing the entry from interfaces.html.ini. But am not sure of what to do of this second failing case. If i add language to WorkerNavigator and when tested with devtools, navigator.language, returns "en-US", in worker's scope, but this still does not make this test pass.

▶ Unexpected subtest result in /workers/interfaces/WorkerUtils/navigator/language.html:
│ FAIL [expected PASS] navigator.language
│   → assert_equals: expected (string) "en-US" but got (undefined) undefined
│ 
│ worker.onmessage<@http://web-platform.test:8000/workers/interfaces/WorkerUtils/navigator/language.html:4:5
│ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1381:20
└ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1405:1

@jdm
Copy link
Member

jdm commented Mar 16, 2016

That's odd; I can't think of any reason that would happen. Did you uncomment http://mxr.mozilla.org/servo/source/components/script/dom/webidls/WorkerNavigator.webidl#10 as well?

@creativcoder
Copy link
Contributor Author

Yes, i did.

@KiChjang
Copy link
Contributor

Does the test use postMessage? Looking at the test file, the comment at the top says postMessage(navigator.language).

@jdm
Copy link
Member

jdm commented Mar 16, 2016

Workers support postMessage.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #9838) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Mar 16, 2016
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Mar 16, 2016
jdm
jdm reviewed Mar 16, 2016
View reviewed changes
components/script/dom/navigator.rs
@@ -72,5 +72,9 @@ impl NavigatorMethods for Navigator {
// https://webbluetoothcg.github.io/web-bluetooth/#dom-navigator-bluetooth
fn Bluetooth(&self) -> Root<Bluetooth> {
self.bluetooth.or_init(|| Bluetooth::new(self.global().r()))

Copy link
Member

Choose a reason for hiding this comment

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

Rebase error; the } is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, my bad. Fixing it. Btw, i got to know a lot, from this PR. :)

@jdm jdm added the S-needs-code-changes Changes have not yet been made that were requested by a reviewer. label Mar 16, 2016
@jdm jdm removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Mar 16, 2016
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Mar 16, 2016
@jdm
Copy link
Member

jdm commented Mar 16, 2016

@bors-servo: r+
Thanks!

@bors-servo
Copy link
Contributor

📌 Commit 0c3454b has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 16, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit 0c3454b with merge 1bd2479...

bors-servo pushed a commit that referenced this pull request Mar 16, 2016
Auto merge of #10009 - creativcoder:impl-navigator-language, r=jdm
1bd2479 
implements NavigatorLanguage

Fixes #9992

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10009)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-wpt

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Mar 16, 2016
@jdm
Copy link
Member

jdm commented Mar 16, 2016 

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] WorkerNavigator interface: attribute language

  ▶ Unexpected subtest result in /workers/interfaces.worker:
  └ PASS [expected FAIL] WorkerNavigator interface: attribute language

  ▶ Unexpected subtest result in /workers/interfaces.worker:
  └ PASS [expected FAIL] WorkerNavigator interface: self.navigator must inherit property "language" with the proper type (6)

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Mar 17, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented Mar 17, 2016

While we're having you jump through hoops, the spec changed DOMString? language to DOMString language, want to update that too?

@Ms2ger
Copy link
Contributor

Ms2ger commented Mar 18, 2016

Thanks!

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit facf591 has been approved by Ms2ger

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 18, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit facf591 with merge 20eb81c...

bors-servo pushed a commit that referenced this pull request Mar 18, 2016
Auto merge of #10009 - creativcoder:impl-navigator-language, r=Ms2ger
20eb81c 
implements NavigatorLanguage

Fixes #9992

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10009)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor

@bors-servo bors-servo merged commit facf591 into servo:master Mar 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@creativcoder @Ms2ger @jdm @bors-servo @wafflespeanut @KiChjang @highfive