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 document.open to latest spec #21882

Merged
merged 1 commit into from Oct 16, 2018

Conversation

Projects
None yet
6 participants
@dguenther
Contributor

dguenther commented Oct 7, 2018

This is one of my first contributions, so I might need some direction cleaning it up -- I ran web-platform-tests locally, but the suite has several intermittent passes/failures for me. Thanks!

Few notes:

  • I may have been wrong to eliminate all of the resets listed in what was formerly Step 18 (starts with self.implementation.set(None);). It's not clear to me that they're still needed or if so, what step they would fall under, but I didn't notice any web platform tests break as a result.
  • If I'm reading the spec right, there's a discrepancy in the error returned by the three-parameter overload of Document.open between the spec and web-platform-tests/implementations in other browsers. As written, I favored the spec, but it causes one web-platform-test to fail. This has been resolved in whatwg/html#4066
  • I'm not 100% certain that tests pass as expected, I had several intermittent failures that disappeared when re-run.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #21445
  • There are tests for these changes (existing web-platform-tests)

This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Oct 7, 2018

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

@highfive

This comment has been minimized.

highfive commented Oct 7, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/document.rs, components/script/dom/webidls/Document.webidl
  • @KiChjang: components/script/dom/document.rs, components/script/dom/webidls/Document.webidl
@emilio

Sorry for the superficial review, I'm not the most familiar with this code so I just commented while skimming over it before sending it through CI.

The new test passes are very exciting, thanks!

// TODO: remove any tasks associated with the Document in any task source.
// Step 9. If document is the associated Document of document's relevant global object,
// then erase all event listeners and handlers given document's relevant global object.
match self.global().downcast::<Window>() {

This comment has been minimized.

@emilio

emilio Oct 7, 2018

Member

nit: can use if let.

// Step 6. If document's ignore-opens-during-unload counter is greater
// than 0, then return document.
if self.is_prompting_or_unloading()
{

This comment has been minimized.

@emilio

emilio Oct 7, 2018

Member

nit: I think this brace can go in the previous line.

// https://html.spec.whatwg.org/multipage/#dom-document-open-window
fn Open_(&self, url: DOMString, target: DOMString, features: DOMString) -> Fallible<DomRoot<WindowProxy>> {
// TODO: WhatWG spec states this should throw an InvalidState err, but web platform tests expect
// an InvalidAccess error

This comment has been minimized.

@emilio

emilio Oct 7, 2018

Member

What do other browsers do?

@emilio

This comment has been minimized.

Member

emilio commented Oct 7, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 7, 2018

⌛️ Trying commit 016c726 with merge cfd9608...

bors-servo added a commit that referenced this pull request Oct 7, 2018

Auto merge of #21882 - dguenther:update-document-open, r=<try>
Update document.open to latest spec

<!-- Please describe your changes on the following line: -->

This is one of my first contributions, so I might need some direction cleaning it up -- I ran web-platform-tests locally, but the suite has several intermittent passes/failures for me. Thanks!

Few notes:

* I may have been wrong to eliminate all of the resets listed in what was formerly Step 18 (starts with `self.implementation.set(None);`). It's not clear to me that they're still needed or if so, what step they would fall under, but I didn't notice any web platform tests break as a result.
* If I'm reading the spec right, there's a discrepancy in the error returned by the three-parameter overload of Document.open between the spec and web-platform-tests/implementations in other browsers. As written, I favored the spec, but it causes one web-platform-test to fail.
* I'm not 100% certain that tests pass as expected, I had several intermittent failures that disappeared when re-run.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21445

<!-- Either: -->
- [x] There are tests for these changes (existing web-platform-tests)

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

This comment has been minimized.

Contributor

bors-servo commented Oct 7, 2018

💔 Test failed - linux-rel-wpt

@dguenther

This comment has been minimized.

Contributor

dguenther commented Oct 7, 2018

No, it's much appreciated! I'm still quite new to Rust, any suggestions to code style improvements are really helpful.

I fixed the test that crashed in the previous run by fixing a TODO in document.write (it still fails the test, but at least doesn't crash).

For the error returned by document.open(url, name, features), it seems other browsers return InvalidAccessError: https://wpt.fyi/results/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/document.open-02.html

It appears the error was changed in this PR: whatwg/html#2672 I'll open a PR to the spec to get some feedback around whether the spec or the test should be changed.

@dguenther

This comment has been minimized.

Contributor

dguenther commented Oct 7, 2018

Scratch that, InvalidAccessError is deprecated: https://heycam.github.io/webidl/#invalidaccesserror

Updating the tests is probably the best path forward here.

@dguenther

This comment has been minimized.

Contributor

dguenther commented Oct 8, 2018

The error discrepancy was resolved in whatwg/html#4066 in favor of throwing InvalidAccessError.

Show resolved Hide resolved components/script/dom/document.rs Outdated

@jdm jdm assigned nox and unassigned jdm Oct 9, 2018

@nox

This is going well. I would like a few issues filed for the missing bits and some explanation for the crash. Please also squash all the commits together.

Show resolved Hide resolved components/script/dom/document.rs
// Step 12.
self.abort();
// Step 7
// TODO: Check if there is an existing attempt to navigate browsing context

This comment has been minimized.

@nox

nox Oct 11, 2018

Member

Can you file an issue for this and put the link to the issue in the comment?

This comment has been minimized.

@dguenther

This comment has been minimized.

@nox

nox Oct 15, 2018

Member

Just put the complete link to the issue.

// TODO: https://github.com/servo/servo/issues/21937
// Step 14.
// TODO: remove any tasks associated with the Document in any task source.
// Step 9
if let Some(window) = self.global().downcast::<Window>() {

This comment has been minimized.

@nox

nox Oct 11, 2018

Member

Just use self.window here.

// Step 9
if let Some(window) = self.global().downcast::<Window>() {
if window.Document() == DomRoot::from_ref(self) {
self.global().upcast::<EventTarget>().remove_all_listeners()

This comment has been minimized.

@nox

nox Oct 11, 2018

Member

Continue to use self.window here.

Show resolved Hide resolved components/script/dom/document.rs
// Step 25.
// Step 12
// TODO: mute iframe load.

This comment has been minimized.

@nox

nox Oct 11, 2018

Member

Filing an issue about that would be nice.

This comment has been minimized.

@dguenther
// TODO: add history entry.
// https://html.spec.whatwg.org/multipage/#dom-document-open-window
fn Open_(&self, url: DOMString, target: DOMString, features: DOMString) -> Fallible<DomRoot<WindowProxy>> {
let wo = match self.browsing_context() {

This comment has been minimized.

@nox

nox Oct 11, 2018

Member

What does wo stand for?

This comment has been minimized.

@dguenther

dguenther Oct 13, 2018

Contributor

Ah, I probably just hit some keys here. I'll change to context as mentioned in your other comment

let wo = match self.browsing_context() {
Some(w) => w.open(url, target, features),
None => return Err(Error::InvalidAccess),
};

This comment has been minimized.

@nox

nox Oct 11, 2018

Member

You can write that this way:

let context = self.browsing_context().ok_or(Error::InvalidAccess)?;
context.open(url, target, features).ok_or(Error::InvalidAccess)

This comment has been minimized.

@dguenther

dguenther Oct 13, 2018

Contributor

👍 thanks!

Show resolved Hide resolved components/script/dom/document.rs
Show resolved Hide resolved ...s/dynamic-markup-insertion/opening-the-input-stream/quirks.window.js.ini
@nox

nox approved these changes Oct 15, 2018

Just fix the spec links and then I will r+.

Show resolved Hide resolved components/script/dom/document.rs
// Step 12.
self.abort();
// Step 7
// TODO: Check if there is an existing attempt to navigate browsing context

This comment has been minimized.

@nox

nox Oct 15, 2018

Member

Just put the complete link to the issue.

// TODO: https://github.com/servo/servo/issues/21937
// Step 15.
// Step 10.
// TODO: Should replace all without firing mutation events. See #21936

This comment has been minimized.

@nox

nox Oct 15, 2018

Member

Complete link to issue, no need to paraphrase its title.

Show resolved Hide resolved ...s/dynamic-markup-insertion/opening-the-input-stream/quirks.window.js.ini

@dguenther dguenther force-pushed the dguenther:update-document-open branch from 19d16c7 to 1538587 Oct 16, 2018

@dguenther

This comment has been minimized.

Contributor

dguenther commented Oct 16, 2018

Great, I updated all of the issue comments to include full links only. Thanks for all the review!

@nox

This comment has been minimized.

Member

nox commented Oct 16, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 16, 2018

📌 Commit 1538587 has been approved by nox

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 16, 2018

⌛️ Testing commit 1538587 with merge 4625160...

bors-servo added a commit that referenced this pull request Oct 16, 2018

Auto merge of #21882 - dguenther:update-document-open, r=nox
Update document.open to latest spec

<!-- Please describe your changes on the following line: -->

This is one of my first contributions, so I might need some direction cleaning it up -- I ran web-platform-tests locally, but the suite has several intermittent passes/failures for me. Thanks!

Few notes:

* I may have been wrong to eliminate all of the resets listed in what was formerly Step 18 (starts with `self.implementation.set(None);`). It's not clear to me that they're still needed or if so, what step they would fall under, but I didn't notice any web platform tests break as a result.
* <s>If I'm reading the spec right, there's a discrepancy in the error returned by the three-parameter overload of Document.open between the spec and web-platform-tests/implementations in other browsers. As written, I favored the spec, but it causes one web-platform-test to fail.</s> This has been resolved in whatwg/html#4066
* I'm not 100% certain that tests pass as expected, I had several intermittent failures that disappeared when re-run.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21445

<!-- Either: -->
- [x] There are tests for these changes (existing web-platform-tests)

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

This comment has been minimized.

Contributor

bors-servo commented Oct 16, 2018

@bors-servo bors-servo merged commit 1538587 into servo:master Oct 16, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
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