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

implement navigator.platform #7582

Merged
merged 1 commit into from Sep 29, 2015
Merged

Conversation

@huonw
Copy link
Contributor

huonw commented Sep 10, 2015

FYI, one can use the cfg!(...) macro (rather than attribute) to write it all as a normal expression rather than having to have repeated definitions of the function, e.g.

if cfg!(all(any(target_os = "linux",
                       target_os = "android"),
                   target_arch = "x86_64")) {
    "Linux x86_64"
} else if cfg!(all(any(target_os = "linux",
                       target_os = "android"),
                   target_arch = "x86")) {
    "Linux i686"
} else {
    ""
}
@paulrouget
Copy link
Contributor Author

paulrouget commented Sep 10, 2015

Didn't know that. This will probably be better. Thanks.

@paulrouget
Copy link
Contributor Author

paulrouget commented Sep 10, 2015

Hm, with my current approach, it won't compile if the platform is not listed. So any port to a new platform requires adding a string for navigator.platorm. Which is, I believe, a good thing.

@huonw
Copy link
Contributor

huonw commented Sep 13, 2015

Good point; seems like a good thing to me too.

@nox
Copy link
Member

nox commented Sep 15, 2015

https://html.spec.whatwg.org/multipage/webappapis.html#dom-navigator-platform

Any information in this API that varies from user to user can be used to profile the user. In fact, if enough such information is available, a user can actually be uniquely identified. For this reason, user agent implementors are strongly urged to include as little information in this API as possible.

Couldn't we just emit a constant?

@paulrouget
Copy link
Contributor Author

paulrouget commented Sep 15, 2015

Couldn't we just emit a constant?

What do you mean?

@nox
Copy link
Member

nox commented Sep 15, 2015

@paulrouget
Copy link
Contributor Author

paulrouget commented Sep 15, 2015

I mean to always return the same thing. Even on different plateforms.

This is how it works now. It always returns an empty string.

At the very least I am not sure the architecture is important to include.

For browser.html, we need navigator.platform to know what modifiers to use for the keybindings and what kind of window controls we should use. We don't care much about the architecture.

@paulrouget
Copy link
Contributor Author

paulrouget commented Sep 16, 2015

I removed the architecture string.

@jdm
Copy link
Member

jdm commented Sep 16, 2015

-S-awaiting-review +S-needs-code-changes +@jdm


Reviewed 1 of 1 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


tests/wpt/mozilla/meta/mozilla/navigator.html.ini, line 6 [r2] (raw file):
Can we use `if os != "linux": FAIL?


tests/wpt/mozilla/meta/mozilla/navigator.html.ini, line 11 [r2] (raw file):
Ditto.


Comments from the review on Reviewable.io

@jdm jdm self-assigned this Sep 16, 2015
@nox
Copy link
Member

nox commented Sep 17, 2015

For browser.html, we need navigator.platform to know what modifiers to use for the keybindings and what kind of window controls we should use. We don't care much about the architecture.

Do you mean that browser.html will reimplement input methods etc in JS land? Won't that have the same insane amount of issues like the Atom editor suffers from?

https://github.com/andischerer/atom-keyboard-localization/issues

@paulrouget
Copy link
Contributor Author

paulrouget commented Sep 17, 2015

Do you mean that browser.html will reimplement input methods etc in JS land?

No. By “keybinding”, I refer to things like “Cmd-T” (which has nothing to do with text input), and "window controls” refers to the close|max|min buttons of a window.

@paulrouget paulrouget force-pushed the paulrouget:navigator.platform branch from 6836db0 to 6b49651 Sep 21, 2015
@paulrouget
Copy link
Contributor Author

paulrouget commented Sep 23, 2015

review ping

@jdm
Copy link
Member

jdm commented Sep 23, 2015

@bors-servo: r+
Thanks!


Reviewed 2 of 2 files at r3.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Sep 23, 2015

📌 Commit 6b49651 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Sep 23, 2015

Testing commit 6b49651 with merge 5e814b7...

bors-servo pushed a commit that referenced this pull request Sep 23, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Sep 23, 2015

💔 Test failed - linux-dev

@jdm
Copy link
Member

jdm commented Sep 23, 2015

/home/servo/buildbot/slave/linux-dev/build/components/script/dom/navigator.rs:56:9: 56:32 error: unresolved name `navigatorinfo::Platform`. Did you mean to call `self.navigatorinfo::Platform`? [E0425]
/home/servo/buildbot/slave/linux-dev/build/components/script/dom/navigator.rs:56         navigatorinfo::Platform()
                                                                                         ^~~~~~~~~~~~~~~~~~~~~~~
/home/servo/buildbot/slave/linux-dev/build/components/script/dom/navigator.rs:56:9: 56:32 help: run `rustc --explain E0425` to see a detailed explanation
/home/servo/buildbot/slave/linux-dev/build/components/script/dom/workernavigator.rs:57:9: 57:32 error: unresolved name `navigatorinfo::Platform`. Did you mean to call `self.navigatorinfo::Platform`? [E0425]
/home/servo/buildbot/slave/linux-dev/build/components/script/dom/workernavigator.rs:57         navigatorinfo::Platform()
                                                                                               ^~~~~~~~~~~~~~~~~~~~~~~
/home/servo/buildbot/slave/linux-dev/build/components/script/dom/workernavigator.rs:57:9: 57:32 help: run `rustc --explain E0425` to see a detailed explanation
@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

Testing commit 132724b with merge f0b64fe...

bors-servo pushed a commit that referenced this pull request Sep 29, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

💔 Test failed - mac-dev-ref-unit

@jdm
Copy link
Member

jdm commented Sep 29, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

Testing commit 132724b with merge aa2b87c...

bors-servo pushed a commit that referenced this pull request Sep 29, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

💔 Test failed - mac-rel-wpt

@paulrouget
Copy link
Contributor Author

paulrouget commented Sep 29, 2015

Intermittent? /html/semantics/embedded-content/the-canvas-element/size.attributes.setAttribute.hex.html

@nox
Copy link
Member

nox commented Sep 29, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

Testing commit 132724b with merge c356593...

bors-servo pushed a commit that referenced this pull request Sep 29, 2015
implement navigator.platform

Fix #7573

I used this as a reference: http://stackoverflow.com/questions/19877924/what-is-the-list-of-possible-values-for-navigator-platform-as-of-today

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7582)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

💔 Test failed - mac-rel-wpt

@jdm
Copy link
Member

jdm commented Sep 29, 2015

Infrastructure failure; hold tight.

@metajack
Copy link
Contributor

metajack commented Sep 29, 2015

@bors-servo retry

  • infra issues addressed i hope
@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

Previous build results for android, gonk, linux-dev, mac-dev-ref-unit are reusable. Rebuilding only linux-rel, mac-rel-css, mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

@bors-servo bors-servo merged commit 132724b into servo:master Sep 29, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.