Implement History State #20638

cbrewster · 2018-04-14T21:55:14Z

This just handles history states in the session history. URL handling and hashchange events still nee to be implemented.

./mach build -d does not report any errors
./mach build-geckolib does not report any errors
./mach test-tidy does not report any errors
These changes fix #19156 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2018-04-14T21:55:18Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/popstateevent.rs, components/script/dom/history.rs, components/constellation/pipeline.rs, components/script/script_thread.rs, components/constellation/constellation.rs and 2 more
@paulrouget: components/constellation/pipeline.rs, components/constellation/constellation.rs, components/constellation/session_history.rs
@fitzgen: components/script_traits/lib.rs, components/script/dom/popstateevent.rs, components/script_traits/script_msg.rs, components/script/dom/history.rs, components/script/script_thread.rs and 1 more
@KiChjang: components/script_traits/lib.rs, components/net/http_loader.rs, components/script/dom/popstateevent.rs, components/net_traits/lib.rs, components/script_traits/script_msg.rs and 4 more

highfive · 2018-04-14T21:55:19Z

Warning

These commits modify unsafe code. Please review it carefully!

cbrewster · 2018-04-14T21:55:22Z

r? @asajeffrey

cbrewster · 2018-04-14T22:01:29Z

There are still some edge cases that will probably need to be addressed, but I think they should be done as followups.

@bors-servo try

bors-servo · 2018-04-14T22:01:34Z

⌛ Trying commit de0450f with merge 38930cb...


        Auto merge of #20638 - cbrewster:history_state, r=<try>

Implement History State  This just handles history states in the session history. URL handling and hashchange events still nee to be implemented. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach build-geckolib` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #19156 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20638)

bors-servo · 2018-04-14T23:05:01Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

asajeffrey

OK, this looks really good! Are we going to finally have history state after all these years?

asajeffrey · 2018-04-15T22:52:08Z

components/constellation/constellation.rs

+                            SessionHistoryDiff::PipelineDiff { ref pipeline_reloader, new_history_state_id, .. } => {
+                                if let NeedsToReload::No(pipeline_id) = *pipeline_reloader {
+                                    pipeline_changes.insert(pipeline_id, Some(new_history_state_id));
+                                }


Perhaps add a comment about what to do in the else case? Until we support history state with URLs, I don't think there's anything sensible we can do :(

Right, my naive plan is to have another HashMasp 😞 which is a <PipelineId, ServoUrl>. It would store the URL that should be used when reloading that Pipeline if different from the normal LoadData.

asajeffrey · 2018-04-15T22:52:08Z

components/constellation/constellation.rs

+                            SessionHistoryDiff::PipelineDiff { ref pipeline_reloader, old_history_state_id, .. } => {
+                                if let NeedsToReload::No(pipeline_id) = *pipeline_reloader {
+                                    pipeline_changes.insert(pipeline_id, old_history_state_id);
+                                }


asajeffrey · 2018-04-15T22:52:08Z

components/constellation/constellation.rs

+        for (pipeline_id, history_state_id) in pipeline_changes.drain() {
+            self.update_pipeline(pipeline_id, history_state_id);
+        }
+


Should we do this before updating the browsing contexts? Otherwise we might get a glitch where the browsing context's current pipeline has been updated but not its state.

I'm not sure how to avoid this, but I would rather the pipeline's activities are all properly set before updating the session history state so that the popstate event will run when the document is active.

asajeffrey · 2018-04-15T22:52:08Z

tests/wpt/metadata/html/browsers/browsing-the-web/history-traversal/popstate_event.html.ini

-  bug: https://github.com/servo/servo/issues/19905
-  [Queue a task to fire popstate event]
-    expected: TIMEOUT
-


Yay, more tests passing!

asajeffrey · 2018-04-15T22:52:08Z

tests/wpt/metadata/html/browsers/history/the-history-interface/001.html.ini

-
-  [history.state should be a separate clone of the object, not a reference to the object passed to the event handler]
-    expected: FAIL
-


Yay, even more tests passing!

asajeffrey · 2018-04-15T22:52:08Z

tests/wpt/metadata/html/browsers/history/the-history-interface/002.html.ini

-    expected: FAIL
-
-  [history.state should be a separate clone of the object, not a reference to the object passed to the event handler]
-    expected: FAIL


Yay, even even more tests passing!

asajeffrey · 2018-04-15T22:53:12Z

You can r=me, perhaps add some comments as mentioned above? Do we have issues for the follow-on tasks?


        Implement history state

cbrewster · 2018-04-16T15:21:37Z

@bors-servo r=asajeffrey

bors-servo · 2018-04-16T15:21:38Z

📌 Commit 17bd80a has been approved by asajeffrey

bors-servo · 2018-04-16T15:21:43Z

⌛ Testing commit 17bd80a with merge 11733d3...


        Auto merge of #20638 - cbrewster:history_state, r=asajeffrey

Implement History State  This just handles history states in the session history. URL handling and hashchange events still nee to be implemented. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach build-geckolib` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #19156 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20638)

bors-servo · 2018-04-16T15:27:01Z

💔 Test failed - mac-rel-wpt4

asajeffrey · 2018-04-16T15:34:01Z

@bors-servo retry

infra issues?

bors-servo · 2018-04-16T15:34:05Z

⚡ Previous build results for mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt2, mac-rel-wpt3 are reusable. Rebuilding only android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-rel-wpt1, mac-rel-wpt4, windows-msvc-dev...

bors-servo · 2018-04-16T16:30:06Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: asajeffrey
Pushing 11733d3 to master...

highfive assigned nox Apr 14, 2018

highfive added the S-awaiting-review label Apr 14, 2018

highfive assigned asajeffrey and unassigned nox Apr 14, 2018

asajeffrey approved these changes Apr 15, 2018

View changes

asajeffrey removed the S-awaiting-review label Apr 15, 2018

Implement history state

Loading status checks…

17bd80a

cbrewster force-pushed the cbrewster:history_state branch from de0450f to 17bd80a Apr 16, 2018

highfive added the S-awaiting-review label Apr 16, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Apr 16, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 16, 2018

bors-servo merged commit 17bd80a into servo:master Apr 16, 2018
3 checks passed

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

This was referenced Apr 16, 2018

History: update document URL on pushState/replaceState #20453

Merged

Implement MessagePort and MessageChannel #16622

Closed

cbrewster mentioned this pull request Apr 16, 2018

History API metabug #10992

Open

18 of 24 tasks complete

cbrewster deleted the cbrewster:history_state branch Apr 17, 2018

cbrewster mentioned this pull request Apr 21, 2018

Fire popstate event when traversing history #19905

Closed

Ms2ger mentioned this pull request Jun 24, 2018

Investigate issues blocking support for Google Docs #13942

Open

9 of 13 tasks complete

servo / servo

Implement History State #20638

Implement History State #20638

cbrewster commented Apr 14, 2018 •

edited by SimonSapin

highfive commented Apr 14, 2018

highfive commented Apr 14, 2018

cbrewster commented Apr 14, 2018

cbrewster commented Apr 14, 2018

bors-servo commented Apr 14, 2018

bors-servo commented Apr 14, 2018

asajeffrey left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

asajeffrey commented Apr 15, 2018

cbrewster commented Apr 16, 2018

bors-servo commented Apr 16, 2018

bors-servo commented Apr 16, 2018

bors-servo commented Apr 16, 2018

asajeffrey commented Apr 16, 2018

bors-servo commented Apr 16, 2018

bors-servo commented Apr 16, 2018


		[history.state should be a separate clone of the object, not a reference to the object passed to the event handler]
		expected: FAIL

servo / servo

Join GitHub today

Implement History State #20638

Implement History State #20638

Conversation

cbrewster commented Apr 14, 2018 • edited by SimonSapin

highfive commented Apr 14, 2018

highfive commented Apr 14, 2018

cbrewster commented Apr 14, 2018

cbrewster commented Apr 14, 2018

bors-servo commented Apr 14, 2018

bors-servo commented Apr 14, 2018

asajeffrey left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

asajeffrey commented Apr 15, 2018

cbrewster commented Apr 16, 2018

bors-servo commented Apr 16, 2018

bors-servo commented Apr 16, 2018

bors-servo commented Apr 16, 2018

asajeffrey commented Apr 16, 2018

bors-servo commented Apr 16, 2018

bors-servo commented Apr 16, 2018

cbrewster commented Apr 14, 2018 •

edited by SimonSapin