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

Constellation informs script about documents becoming inactive, active or fully active. #14971

Merged
merged 2 commits into from Jan 28, 2017
Merged

Constellation informs script about documents becoming inactive, active or fully active. #14971

merged 2 commits into from Jan 28, 2017

Conversation

asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jan 11, 2017 •
edited by larsbergstrom

This PR replaces the current freeze/thaw mechanism by messages from the constellation to script informing it about when documents become inactive, active or fully active.

This means we can now implement |Document::is_active()| which is used in |document.write|.

This PR also changes how timers work: previously they were initialized running, and were then frozen/thawed. This means there was a transitory period when timers were running even though the document was not fully active. With this PR, timers are initially suspended, and are only resumed when the document is made fully active.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix Determine if a Document is active #14876
  • These changes do not require tests because it's an interal refactoring.

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/document.rs, components/script/timers.rs, components/script/dom/bindings/trace.rs, components/script/script_thread.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs
  • @KiChjang: components/script/dom/document.rs, components/script/timers.rs, components/script/dom/bindings/trace.rs, components/script/script_thread.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs

@highfive
Copy link

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 11, 2017
@asajeffrey asajeffrey added A-content/script Related to the script thread A-constellation Involves the constellation labels Jan 11, 2017
@asajeffrey
Copy link
Member Author

cc @jdm @cbrewster

@asajeffrey
Copy link
Member Author

Hmm, there are wpt failures for this, I've still not quite got timers sorted out. I'll let you know once I've got something that passes wpt locally.

@asajeffrey
Copy link
Member Author

I rewrote this PR to explicitly initialize the document activity, which means we can go back to timers initially running. This fixes the wpt failures locally.

This PR is now ready for review.

@asajeffrey
Copy link
Member Author

Oh good, an intermittent:

./mach test-wpt --release --repeat-until-unexpected ~/_mozilla/mozilla/websocket_connection_fail.html
...
Running 1 tests in web-platform-tests

Ran 185 tests finished in 1.0 seconds.
  • 185 ran as expected. 0 tests skipped.

Running 1 tests in web-platform-tests
...
  ▶ Unexpected subtest result in /_mozilla/mozilla/websocket_connection_fail.html:
  │ TIMEOUT [expected PASS] Untitled
  └   → Test timed out

@asajeffrey
Copy link
Member Author

That intermittent is reproducable on master: #14985.

@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Jan 12, 2017
@asajeffrey
Copy link
Member Author

Rebased.

@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Jan 13, 2017
@jdm
Copy link
Member

jdm commented Jan 19, 2017

@Ms2ger Review ping.

@asajeffrey asajeffrey removed the S-needs-rebase There are merge conflict errors. label Jan 19, 2017
@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 20, 2017

Can we add a test for the document.write() change?

r? @cbrewster

@highfive highfive assigned cbrewster and unassigned Ms2ger Jan 20, 2017
@asajeffrey
Copy link
Member Author

@Ms2ger OK, I'll give it a shot. The test might fail at the moment, because writing into iframe content documents isn't fully implemented IIRC. @nox?

@asajeffrey asajeffrey mentioned this pull request Jan 20, 2017
Closed
@nox
Copy link
Contributor

nox commented Jan 21, 2017

@asajeffrey I have no idea what you mean by "fully implemented". Why would iframe content documents be different than any other document? As long is it's same origin I don't see what's not implemented.

@asajeffrey
Copy link
Member Author

@nox: I pushed a test to this PR (90fb8e3) which will show you what I mean. It passes in FF, but fails in servo with the error 'The object is in an invalid state'.

@asajeffrey
Copy link
Member Author

Rebased.

@asajeffrey asajeffrey removed the S-needs-squash Some (or all) of the commits in the PR should be combined. label Jan 27, 2017
@asajeffrey
Copy link
Member Author

@bors-servo r=cbrewster

@bors-servo
Copy link
Contributor

📌 Commit 631fefb has been approved by cbrewster

@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 Jan 27, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 631fefb with merge 7cdc265...

bors-servo pushed a commit that referenced this pull request Jan 27, 2017
Auto merge of #14971 - asajeffrey:script-track-active-documents, r=cb…
7cdc265 
…rewster

Constellation informs script about documents becoming inactive, active or fully active.

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

This PR replaces the current freeze/thaw mechanism by messages from the constellation to script informing it about when documents become inactive, active or fully active.

This means we can now implement |Document::is_active()| which is used in |document.write|.

This PR also changes how timers work: previously they were initialized running, and were then frozen/thawed. This means there was a transitory period when timers were running even though the document was not fully active. With this PR, timers are initially suspended, and are only resumed when the document is made fully active.

---
<!-- 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 #14876
- [X] These changes do not require tests because it's an interal refactoring.

<!-- 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/14971)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-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 Jan 27, 2017
@cbrewster
Copy link
Contributor 

  ▶ Unexpected subtest result in /html/dom/dynamic-markup-insertion/document-write/write-active-document.html:
  │ FAIL [expected PASS] document.write only writes to active documents
  └   → The object is in an invalid state.

@asajeffrey
Copy link
Member Author

Oops, forgot to check in the .ini file.

@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 Jan 27, 2017
@asajeffrey
Copy link
Member Author

@bors-servo r=cbrewster

@bors-servo
Copy link
Contributor

📌 Commit ca9cee0 has been approved by cbrewster

@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 Jan 27, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit ca9cee0 with merge b5c94ba...

bors-servo pushed a commit that referenced this pull request Jan 28, 2017
Auto merge of #14971 - asajeffrey:script-track-active-documents, r=cb…
b5c94ba 
…rewster

Constellation informs script about documents becoming inactive, active or fully active.

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

This PR replaces the current freeze/thaw mechanism by messages from the constellation to script informing it about when documents become inactive, active or fully active.

This means we can now implement |Document::is_active()| which is used in |document.write|.

This PR also changes how timers work: previously they were initialized running, and were then frozen/thawed. This means there was a transitory period when timers were running even though the document was not fully active. With this PR, timers are initially suspended, and are only resumed when the document is made fully active.

---
<!-- 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 #14876
- [X] These changes do not require tests because it's an interal refactoring.

<!-- 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/14971)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

@bors-servo bors-servo merged commit ca9cee0 into servo:master Jan 28, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 28, 2017
This was referenced Jan 28, 2017
Merged
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm jdm left review comments

@cbrewster cbrewster cbrewster left review comments

Assignees

@cbrewster cbrewster

Labels
A-constellation Involves the constellation A-content/script Related to the script thread
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Determine if a Document is active
7 participants
@asajeffrey @highfive @bors-servo @jdm @Ms2ger @nox @cbrewster