Skip to content

/ servo

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

History API #11220

Closed
wants to merge 10 commits into from
Closed

History API #11220

wants to merge 10 commits into from

Conversation

@cbrewster
Copy link
Member

cbrewster commented May 17, 2016 •
edited

Thank you for contributing to Servo! Please replace each [ ] by [X] when the step is complete, and replace __ with appropriate data:

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

Either:

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

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

What this PR covers

  • Add the history object
  • Support navigation by a delta
  • Support interleaved session history
  • Implement HistoryTraversalTaskSource
  • Add onpopstate window event listener
  • Use BrowsingContext's session history to store history state

What needs to be done:

  • Cleanup (Clear out things marked with TODO and cleanup commits)
  • Enabling the history interface tests
  • Security Checks
  • Window onpopstate
  • Clear forward state on BrowsingContext when frames are evicted

This change is Reviewable
@highfive
Copy link

highfive commented May 17, 2016

Heads up! This PR modifies the following files:

  • @jgraham: components/webdriver_server/lib.rs
  • @KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/bindings/global.rs, components/script/dom/history.rs, components/script/dom/webidls/History.webidl, components/script/dom/mod.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/dom/popstateevent.rs, components/script/dom/browsingcontext.rs, components/script/script_thread.rs, components/script/task_source/history_traversal.rs, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl, components/script_traits/lib.rs, components/script_traits/lib.rs
@highfive
Copy link

highfive commented May 17, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@cbrewster
Copy link
Member Author

cbrewster commented May 17, 2016

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented May 17, 2016

⌛ Trying commit 697d759 with merge dda9dc8...
bors-servo added a commit that referenced this pull request May 17, 2016
@bors-servo
Auto merge of #11220 - ConnorGBrewster:history_api, r=<try>
dda9dc8 
History API

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 --faster` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

Either:
- [ ] There are tests for these changes OR
 * Will enable these after making sure the current set of tests still pass.
- [ ] These changes do not require tests because _____

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

This is a work in progress. **Do not merge.**

What needs to be done:
 - [ ] Cleanup
 - [ ] Enabling the history interface tests
 - [ ] Security Checks
 - [ ] Changing `Url` on state change
@cbrewster cbrewster mentioned this pull request May 17, 2016
Open
18 of 24 tasks complete
@bors-servo
Copy link
Contributor

bors-servo commented May 17, 2016

💔 Test failed - mac-rel-wpt
@highfive
Copy link

highfive commented May 17, 2016 

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/history-traversal/PopStateEvent.html:
  └ PASS [expected FAIL] Dispatching a synthetic PopStateEvent

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/history-traversal/browsing_context_name_cross_origin.html:
  └ PASS [expected FAIL] Restoring window.name on cross-origin history traversal

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-basic.html:
  └ PASS [expected FAIL] Default value is &#34;auto&#34;

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-basic.html:
  └ PASS [expected FAIL] Invalid values are ignored

  ▶ TIMEOUT [expected OK] /html/browsers/browsing-the-web/history-traversal/popstate_event.html

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/history-traversal/popstate_event.html:
  │ TIMEOUT [expected FAIL] Queue a task to fire popstate event
  └   → Test timed out

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: existence and properties of interface object

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface object length

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface object name

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: existence and properties of interface prototype object

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: existence and properties of interface prototype object&#39;s &#34;constructor&#34; property

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

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

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

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: operation go(long)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: operation back()

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: operation forward()

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: operation pushState(any,DOMString,DOMString)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: operation replaceState(any,DOMString,DOMString)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History must be primary interface of window.history

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] Stringification of window.history

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: window.history must inherit property &#34;length&#34; with the proper type (0)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: window.history must inherit property &#34;scrollRestoration&#34; with the proper type (1)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: window.history must inherit property &#34;state&#34; with the proper type (2)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: window.history must inherit property &#34;go&#34; with the proper type (3)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: calling go(long) on window.history with too few arguments must throw TypeError

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: window.history must inherit property &#34;back&#34; with the proper type (4)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: window.history must inherit property &#34;forward&#34; with the proper type (5)

  ▶ Unexpected subt</span><span class="stdout">est result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: window.history must inherit property &#34;pushState&#34; with the proper type (6)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: calling pushState(any,DOMString,DOMString) on window.history with too few arguments must throw TypeError

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: window.history must inherit property &#34;replaceState&#34; with the proper type (7)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: calling replaceState(any,DOMString,DOMString) on window.history with too few arguments must throw TypeError

  ▶ Unexpected subtest result in /_mozilla/mozilla/interfaces.html:
  │ FAIL [expected PASS] Interfaces exposed on the window
  │   → assert_true: If this is failing: DANGER, are you sure you want to expose the new interface History to all webpages as a property on the window? Do not make a change to this file without review from jdm or Ms2ger for that specific change! expected true got false
  │ 
  │ @http://web-platform.test:8000/_mozilla/mozilla/interfaces.html:254:5
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  └ @http://web-platform.test:8000/_mozilla/mozilla/interfaces.html:248:1
@cbrewster
Copy link
Member Author

cbrewster commented May 17, 2016

Looks like nothing broke at all! 😄
@cbrewster cbrewster force-pushed the cbrewster:history_api branch from 697d759 to 3ae3341 May 17, 2016
@highfive
Copy link

highfive commented May 17, 2016

New code was committed to pull request.
@highfive
Copy link

highfive commented May 17, 2016

New code was committed to pull request.
@cbrewster cbrewster force-pushed the cbrewster:history_api branch May 17, 2016
@highfive
Copy link

highfive commented May 17, 2016

New code was committed to pull request.
@cbrewster cbrewster force-pushed the cbrewster:history_api branch to 59fc102 May 17, 2016
@cbrewster
Copy link
Member Author

cbrewster commented May 17, 2016

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented May 17, 2016

⌛ Trying commit 59fc102 with merge ae03bdb...
bors-servo added a commit that referenced this pull request May 17, 2016
@bors-servo
Auto merge of #11220 - ConnorGBrewster:history_api, r=<try>
ae03bdb 
History API

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 --faster` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

Either:
- [ ] There are tests for these changes OR
 * Will enable these after making sure the current set of tests still pass.
- [ ] These changes do not require tests because _____

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

This is a work in progress. **Do not merge.**

What needs to be done:
 - [ ] Cleanup (Clear out things marked with `TODO` and cleanup commits)
 - [ ] Enabling the history interface tests
 - [ ] Security Checks
 - [ ] Changing `Url` on state change
 - [ ] Scroll Restoration (I may save this for a different PR)
 - [ ] Window `onpopstate`
 - [x] Clear forward state on `BrowsingContext` when frames are evicted

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

bors-servo commented May 17, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows
@highfive
Copy link

highfive commented May 17, 2016

New code was committed to pull request.
@cbrewster cbrewster force-pushed the cbrewster:history_api branch from 6b6e183 to 122ad81 May 17, 2016
@highfive
Copy link

highfive commented May 17, 2016

New code was committed to pull request.
@cbrewster
Copy link
Member Author

cbrewster commented May 17, 2016

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented May 17, 2016

⌛ Trying commit 122ad81 with merge c78029a...
bors-servo added a commit that referenced this pull request May 17, 2016
@bors-servo
Auto merge of #11220 - ConnorGBrewster:history_api, r=<try>
c78029a 
History API

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 --faster` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

Either:
- [ ] There are tests for these changes OR
 * Will enable these after making sure the current set of tests still pass.
- [ ] These changes do not require tests because _____

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

This is a work in progress. **Do not merge.**

What needs to be done:
 - [ ] Cleanup (Clear out things marked with `TODO` and cleanup commits)
 - [ ] Enabling the history interface tests
 - [ ] Security Checks
 - [ ] Changing `Url` on state change
 - [ ] Scroll Restoration (I may save this for a different PR)
 - [ ] Window `onpopstate`
 - [x] Clear forward state on `BrowsingContext` when frames are evicted

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

bors-servo commented May 17, 2016

💔 Test failed - linux-rel
@cbrewster cbrewster force-pushed the cbrewster:history_api branch from 03a7420 to 869c649 May 21, 2016
@cbrewster
Copy link
Member Author

cbrewster commented May 21, 2016 •
edited

@asajeffrey it looks like I will have to do a bit of reworking since the different tabs in browserhtml all use one constellation.

Edit:
So it looks like we need to add a notion of a top-level browsing context. This would be aFrame that has no children, and then each of those needs to have a joint session history. Each tab in browser.html would then have its own top-level browsing context.

The only issue with that approach is that each tab's browsing context won't be a top-level browsing context. I am not sure how this should be dealt with, or if this requires some changes on the browser.html side.

https://html.spec.whatwg.org/multipage/browsers.html#top-level-browsing-context
https://html.spec.whatwg.org/multipage/browsers.html#joint-session-history
@asajeffrey
Copy link
Member

asajeffrey commented May 22, 2016

Review status: 39 of 51 files reviewed at latest revision, 21 unresolved discussions.

components/constellation/constellation.rs, line 143 [r41] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

It was my understanding that there would be a Constellation per tab, but I could wrong on that.

No, there's just one constellation, containing browser.html, which has a collection of tab iframes, each with its own history.

components/constellation/constellation.rs, line 146 [r41] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

See other.

Ditto.

components/constellation/constellation.rs, line 241 [r41] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

The Frame stores the session history for each page/browsing context. Each one needs to keep track of its own history. The entry reason is used to determine if a pipeline should be closed when clearing the forward session history. (ex. if the entry is only due to a state change, that pipeline should not be closed as there could be other entries still using it)

Hmm. It would still be nice if we could simplify this somehow. Do we forsee a need for any more detail in the state change? If not, can we just keep a count of how many state changes each `Pipeline` has gone through?

components/constellation/constellation.rs, line 243 [r41] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

The index is used to keep track of the active session history entry for BrowsingContext in the scrip thread. I don't see a great way around it, but that index isn't used anywhere in the constellation except when sending a state update message to the script thread.

I guess this is part of the general "can we simplify this?" question. If we resolve that, chances are we will resolve indexing along the way.

components/constellation/constellation.rs, line 1174 [r41] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

I can change the naming here, this is used by History to determine if the Document is fully active which just means that the document is active and not just a document stores somewhere in the session history.

A link to https://html.spec.whatwg.org/multipage/browsers.html#fully-active would do the trick.

components/constellation/constellation.rs, line 1177 [r41] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

Not sure if there is a better way to do this, I am open for suggestions.

Again, I think this is part of the "can we simplify?" question.

components/constellation/constellation.rs, line 1448 [r41] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

Here is a scenario: Frame 0 navigates once to a new URL, Frame 1 navigates, and then Frame 0 navigates again. If we do history.go(-3) in JS, this will cause a Navigate message to be sent here saying to navigate back 3. This will look through the joint session history and determine that Frame 0 needs to go back twice and Frame 1 needs to go back once.

I don't follow this example: why does each frame need to navigate? Is it so that `go(-3)` is the same as `go(-1); go(-1); go(-1)`?

components/constellation/constellation.rs, line 1287 [r46] (raw file):

    }

    fn navigate_frame(&mut self, frame_id: FrameId, direction: NavigationDirection) {

It's unclear to me how this function relates to the spec (https://html.spec.whatwg.org/multipage/browsers.html#traverse-the-history and https://html.spec.whatwg.org/multipage/browsers.html#traverse-the-history-by-a-delta).

Comments from Reviewable
@asajeffrey
Copy link
Member

asajeffrey commented May 22, 2016

IRC conversation: http://logs.glob.uno/?c=mozilla%23servo&s=22+May+2016&e=22+May+2016#c435804

TL;DR: we need input about how to handle the browser.html case, e.g. from @paulrouget
@asajeffrey asajeffrey mentioned this pull request May 23, 2016
Closed
@cbrewster
Copy link
Member Author

cbrewster commented May 23, 2016

Blocked by #11344
@bors-servo
Copy link
Contributor

bors-servo commented May 24, 2016

☔ The latest upstream changes (presumably #11326) made this pull request unmergeable. Please resolve the merge conflicts.
@cbrewster cbrewster force-pushed the cbrewster:history_api branch from 869c649 to 3f73e72 May 26, 2016
@asajeffrey
Copy link
Member

asajeffrey commented May 27, 2016

IRC conversation: http://logs.glob.uno/?c=mozilla%23servo&s=27+May+2016&e=27+May+2016#c442425
@bors-servo
Copy link
Contributor

bors-servo commented May 27, 2016

☔ The latest upstream changes (presumably #11471) made this pull request unmergeable. Please resolve the merge conflicts.
@cbrewster cbrewster force-pushed the cbrewster:history_api branch from 3f73e72 to 8b01633 May 28, 2016
@cbrewster
Copy link
Member Author

cbrewster commented May 28, 2016 •
edited

@asajeffrey the joint session history is now an iterator that lazily interleaves the frame tree history. Seems to work very well, plus if a Frame is removed from the tree, there should be no issue now.

Also, once we get this ready for actual reviewal, I will open a PR so reviewable isn't so slow
@bors-servo
Copy link
Contributor

bors-servo commented May 28, 2016

☔ The latest upstream changes (presumably #11472) made this pull request unmergeable. Please resolve the merge conflicts.
cbrewster added 9 commits May 11, 2016
@cbrewster
Implement history API
fe91d54 
window ref

push state

forward nav

more history api work

Added popstate event usage

more stuff

Added delta to NavigationDirection enum variants

Added history length and full delta nav

Added constellation - script communication for active history entry

Quick fixes to finish of proof of concept

Switched to use StructuredCloneData when possible

Use Heap<JS> instead of MutHeapJSVal

pushState and replaceState should be marked as Throw

Added FrameEntry

added interleaved session history

Cleanup

tidy fixes

cleaned up

clear forward browsing context session history

updated test expectations

enabled history interface tests

Added popstate event handler
fixed forward history not being cleared

Fix conditions on when popstate should be fired

push/replace state url handling

remove changing of hash as it causes a page reload

Updated test expectations

Added active document check
@cbrewster
Don't check cross-origin if the Url is not changed
bc90533
@cbrewster
Skip activating transitional entries with a larger delta
79f2930
@cbrewster
Rebased
5a341b5
@cbrewster
Fix debug messages
4298c1e 
Address other comments

Remove duplicate popstate event binding
@cbrewster
Added spec link for fully active
a13c2bc
@cbrewster
Added support for mozbrowser/multiple tlbc
24f5fa4 
rebase and fix bhtml nav issue
@cbrewster
Joint session history lazy iter
a62a005
@cbrewster
rebased
Loading status checks…
379497a 
added TODO for fully active document
@cbrewster cbrewster force-pushed the cbrewster:history_api branch from 8b01633 to 379497a May 28, 2016
@cbrewster
fix tidy
Loading status checks…
d5a040c
@highfive
Copy link

highfive commented May 28, 2016

New code was committed to pull request.
@cbrewster cbrewster mentioned this pull request May 28, 2016
Closed
3 of 4 tasks complete
@cbrewster
Copy link
Member Author

cbrewster commented May 28, 2016

New PR at #11481
@cbrewster cbrewster closed this May 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@asajeffrey asajeffrey

Labels
S-awaiting-review S-blocked-on-external
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@cbrewster @highfive @bors-servo @asajeffrey @Manishearth
You can’t perform that action at this time.