Implement joint session history #11866

cbrewster · 2016-06-25T17:15:32Z

This is cleaned up and should align with the patches on https://github.com/ConnorGBrewster/ServoNavigation/blob/master/notes/notes.pdf
r? @asajeffrey

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

There are tests for these changes OR
These changes do not require tests because this is not testable until the History API is added.

This change is

highfive · 2016-06-25T17:15:36Z

Heads up! This PR modifies the following files:

@asajeffrey: components/webdriver_server/lib.rs, components/constellation/constellation.rs
@jgraham: components/webdriver_server/lib.rs
@KiChjang: components/script/dom/htmliframeelement.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script_traits/lib.rs, components/script_traits/lib.rs

highfive · 2016-06-25T17:15:37Z

Warning

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

bors-servo · 2016-06-26T14:14:34Z

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

cbrewster · 2016-06-26T14:32:13Z

Rebased.

jgraham · 2016-06-27T14:53:44Z

Does this try to implement the spec model? Based on input from Gecko and Blink engineers, it seems that extant implementations are different from the spec in ways that are likely to be difficult to reconcile without changing the spec model to match implementations. In particular the existing implementations store a representation of the frame tree for each position in history, and then diff that tree during navigations to work out which frame to navigate. IMO we should implement that model and change the spec to match.

cbrewster · 2016-06-27T14:57:33Z

@jgraham it's based off of https://github.com/ConnorGBrewster/ServoNavigation/blob/master/notes/notes.pdf. It tries to stay as close to the current spec as possible. That proposes some changes to the spec to match up with what other browsers do/making traversal homomorphic.

asajeffrey · 2016-06-27T20:03:19Z

@jgraham we tried to make the minimum changes to the spec but be a) as close as possible to existing implementations, and b) vaguely sensible :) We are expecting that spec changes will be a result of this work.

jgraham · 2016-06-27T20:14:01Z

OK, but "as few changes from the spec as possible" should not be a goal. From what I've heard so far it sounds like there is common agreement on how this stuff is implemented, and the spec should reflect that agreement (in this area particularly because other implementations are highly unlikely to change their code to reflect a different model in the spec). From Servo's point of view this doesn't seem like an area where being gratuitously different can buy much in terms of the higher level goals around quality and performance.

asajeffrey · 2016-06-27T20:19:05Z

Agreed, the goals are (a) and (b) and if we can get something that meets this with as little disruption, so much the better.

asajeffrey · 2016-06-27T21:16:43Z

This implementation is based on timestamps and lazily computing the joint session history. I think with the revised spec, we should be able to use an in-memory representation of the jsh, and not have to keep track of timestamps. Not sure which is better.

Reviewed 6 of 7 files at r7, 1 of 1 files at r8.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

components/constellation/constellation.rs, line 220 [r8] (raw file):

}

struct FrameIterator<'a> {

Rather than keeping two iterators, wouldn't it be easier to have separate ForwardFrameIterator and BackwardFrameInterator types?

components/constellation/constellation.rs, line 232 [r8] (raw file):

        match self.direction {
            TraversalDirection::Back(_) => {
                if let Some(entry) = self.current {

I think this is doing by hand what once(current).chain(prev) does.

components/constellation/constellation.rs, line 266 [r8] (raw file):

    }

    fn iter(&self, direction: TraversalDirection) -> FrameIterator {

I think this would be simpler with two different iterators, since they're different logic.

components/constellation/constellation.rs, line 364 [r8] (raw file):

                self.stack.iter_mut()
                          .filter_map(|&mut (frame_id, ref mut iter)| {
                              iter.peek().cloned().map(|(_, instant)| (frame_id, iter, instant))

Yay combinators!

Comments from Reviewable

asajeffrey · 2016-06-27T21:48:26Z

IRC chat: http://logs.glob.uno/?c=mozilla%23servo&s=27+Jun+2016&e=27+Jun+2016#c463962

TL;DR in-memory vs lazy computation of joint session history?

cbrewster · 2016-06-27T23:02:53Z

Cleaned up a bit, still a few things I would like to get cleaner, like reducing the number of Vec allocations. But this should clean up the Iterator stuff a bit.

Review status: 0 of 7 files reviewed at latest revision, 4 unresolved discussions.

components/constellation/constellation.rs, line 220 [r8] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Rather than keeping two iterators, wouldn't it be easier to have separate ForwardFrameIterator and BackwardFrameInterator types?

I removed the `FrameIterator` altogether, and now store a back and forward stack on the `HistoryIterator`, if it is going forward, the back stack is just an empty vec and vice versa.

components/constellation/constellation.rs, line 232 [r8] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

I think this is doing by hand what once(current).chain(prev) does.

Thanks for the tip on `once`, I am not able to remove `FrameIterator`.

components/constellation/constellation.rs, line 266 [r8] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

I think this would be simpler with two different iterators, since they're different logic.

see above.

components/constellation/constellation.rs, line 364 [r8] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Yay combinators!

😄

Comments from Reviewable

cbrewster · 2016-06-27T23:38:15Z

I just cleaned up the HistoryIterator quite a bit. Just added a new enum for HistoryDirection that holds the corresponding stack type. This gets rid of having an unused empty Vec.

cbrewster · 2016-06-27T23:51:16Z

components/constellation/constellation.rs, line 232 [r8] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

Thanks for the tip on once, I am not able to remove FrameIterator.

s\not\now

Comments from Reviewable

cbrewster · 2016-06-28T00:24:48Z

I did as much as I could think of to reduce the amount of vec allocations, now there are only 2 vec allocations for full_frame_tree(). The pipelines vec can't be switched to a iter because you can't chain while in a for/while loop 😢.

This is ready for another review pass.

asajeffrey · 2016-06-28T00:32:06Z

OK, I'll have a look tomorrow.

asajeffrey · 2016-06-28T17:15:47Z

Reviewed everything apart from the constellation, i.e. all the easy bits! I'm trying to work out how we can simplify the constellation code.

Reviewed 6 of 7 files at r11.
Review status: 6 of 7 files reviewed at latest revision, 3 unresolved discussions.

components/msg/constellation_msg.rs, line 246 [r17] (raw file):

#[derive(Clone, PartialEq, Eq, Copy, Hash, Debug, Deserialize, Serialize)]
pub enum TraversalDirection {

Yay for spec-compliant naming!

components/script_traits/lib.rs, line 551 [r17] (raw file):

    LoadUrl(PipelineId, LoadData),
    /// Request to traverse the joint session history.
    TraverseHistory(Option<(PipelineId)>, TraversalDirection),

Don't need parentheses around PipelineId.

components/script_traits/script_msg.rs, line 77 [r17] (raw file):

    MozBrowserEvent(PipelineId, SubpageId, MozBrowserEvent),
    /// HTMLIFrameElement Forward or Back traversal.
    TraverseHistory(Option<(PipelineId)>, TraversalDirection),

Don't need parentheses around PipelineId.

Comments from Reviewable

cbrewster · 2016-06-28T23:32:02Z

Finally got back around to this one. I would like to simplify the extra constellation code aswell, it seems like the constellation could grow a lot after this, old pipeline purging, and History API state stuff if we aren't careful.

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions.

components/msg/constellation_msg.rs, line 246 [r17] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Yay for spec-compliant naming!

🎉

components/script_traits/lib.rs, line 551 [r17] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Don't need parentheses around PipelineId.

haha woops!

components/script_traits/script_msg.rs, line 77 [r17] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Don't need parentheses around PipelineId.

Done.

Comments from Reviewable

bors-servo · 2016-07-03T16:57:45Z

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

asajeffrey · 2016-07-21T21:18:28Z

Reviewed 3 of 7 files at r56, 2 of 2 files at r62, 2 of 2 files at r63.
Review status: 6 of 7 files reviewed at latest revision, 6 unresolved discussions.

components/constellation/constellation.rs, line 1572 [r53] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

I now use the joint_session_future and joint_session_past and check if they are empty. This may still be too expensive, thoughts?

OK, we should at least file an issue saying "must get round to fixing this."

components/constellation/constellation.rs, line 583 [r63] (raw file):

    }

    fn joint_session_future(&self, frame_id_root: FrameId) -> Vec<(Instant, FrameId, PipelineId)> {

Yay, this looks a lot nicer!

components/constellation/constellation.rs, line 603 [r63] (raw file):

            };
            for &frame_id in &pipeline.children {
                future.extend_from_slice(&self.joint_session_future(frame_id));

Are we creating vectors each time we do a recursive call? Wouldn't it be more efficient to pass in the vector as an argument?

components/constellation/constellation.rs, line 608 [r63] (raw file):

        // reverse sorting
        future.sort_by(|a, b| b.cmp(a));

Similarly, wouldn't it be more efficient to sort just the once, rather than every recursive call?

components/constellation/constellation.rs, line 646 [r63] (raw file):

    // Get a `Vec` of all the frames that are descendants of a root frame. This includes all
    // active and inactive frames.
    fn full_frame_tree(&self, frame_id_root: FrameId) -> Vec<FrameId> {

Can we get rid of this function? it's only used to remove the joint session future, which we should be able to do without slurping the whole frame tree into a vector.

components/constellation/constellation.rs, line 2208 [r63] (raw file):

    }

    fn remove_forward_history_in_frame_tree(&mut self, frame_id: FrameId) {

Bikeshed: clear_joint_session_future?

Comments from Reviewable

cbrewster · 2016-07-21T21:39:55Z

Review status: 6 of 7 files reviewed at latest revision, 6 unresolved discussions.

components/constellation/constellation.rs, line 1572 [r53] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

OK, we should at least file an issue saying "must get round to fixing this."

Will do.

components/constellation/constellation.rs, line 603 [r63] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Are we creating vectors each time we do a recursive call? Wouldn't it be more efficient to pass in the vector as an argument?

Done.

components/constellation/constellation.rs, line 608 [r63] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Similarly, wouldn't it be more efficient to sort just the once, rather than every recursive call?

Done.

components/constellation/constellation.rs, line 646 [r63] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Can we get rid of this function? it's only used to remove the joint session future, which we should be able to do without slurping the whole frame tree into a vector.

I knew I was forgetting something...

components/constellation/constellation.rs, line 2208 [r63] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Bikeshed: clear_joint_session_future?

Done.

Comments from Reviewable

asajeffrey · 2016-07-21T21:47:29Z

Yay, nearly there, just one more nit.

Reviewed 1 of 1 files at r65.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

components/constellation/constellation.rs, line 592 [r65] (raw file):

    }

    fn get_future_entries(&self, frame_id_root: FrameId, mut future: &mut Vec<(Instant, FrameId, PipelineId)>) {

Can just make this a Vec rather than a &mut Vec?

Comments from Reviewable

asajeffrey · 2016-07-21T22:02:38Z

My goodness that was a bit epic!

@bors-servo r+

bors-servo · 2016-07-21T22:02:39Z

📌 Commit e72d1a5 has been approved by asajeffrey

bors-servo · 2016-07-21T22:51:36Z

⌛ Testing commit e72d1a5 with merge 0559e4f...


        Auto merge of #11866 - ConnorGBrewster:joint_session_history, r=asaje…

@asajeffrey

…ffrey Implement joint session history  This is cleaned up and should align with the patches on https://github.com/ConnorGBrewster/ServoNavigation/blob/master/notes/notes.pdf r? @asajeffrey ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11669 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because this is not testable until the History API is added.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11866)

bors-servo · 2016-07-21T23:49:20Z

💔 Test failed - linux-rel

asajeffrey · 2016-07-22T00:06:04Z

Tests with unexpected results:
  ▶ TIMEOUT [expected OK] /_mozilla/mozilla/mozbrowser/mozbrowserlocationchange_event.html
  │ 
  │ Xlib:  extension "XFree86-VidModeExtension" missing on display ":0".
  │ ERROR:js::rust: Error at :0:0: Error: assert_array_equals: property 13, expected false but got true
  └ 

  ▶ Unexpected subtest result in /_mozilla/mozilla/mozbrowser/mozbrowserlocationchange_event.html:
  │ TIMEOUT [expected PASS] Browser API; mozbrowserlocationchange event
  └   → Test timed out


        Implement beginnings of joint session history

Fix backward navigation make use of history iterator Add frame iterator add different back logic cleanup navigation_info Add extra explanation for iter logic Remove forward history on full frame tree Rename navigation to traversal where appropriate check full tree for can go back/forward simplify frame iter logic remove FrameIterator cleanup history iter reduce amount of vec allocations removed extra parenthesis Remove history iterator cleanup after rebasing avoid recursive vec allocation remove full_frame_tree remove_forward_history_in_frame_tree -> clear_joint_session_future

cbrewster · 2016-07-22T03:15:17Z

@bors-servo r=asajeffrey
I was checking the jsh future/past of the root pipeline instead of the mozbrowser iframe pipeline.

bors-servo · 2016-07-22T03:15:18Z

📌 Commit f131818 has been approved by asajeffrey

bors-servo · 2016-07-22T03:40:57Z

⌛ Testing commit f131818 with merge 05cc763...


        Auto merge of #11866 - ConnorGBrewster:joint_session_history, r=asaje…

@asajeffrey

…ffrey Implement joint session history  This is cleaned up and should align with the patches on https://github.com/ConnorGBrewster/ServoNavigation/blob/master/notes/notes.pdf r? @asajeffrey ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11669 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because this is not testable until the History API is added.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11866)

bors-servo · 2016-07-22T04:44:00Z

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

highfive assigned asajeffrey Jun 25, 2016

highfive added the S-awaiting-review label Jun 25, 2016

cbrewster force-pushed the cbrewster:joint_session_history branch from 4991c6c to 414fe1e Jun 25, 2016

highfive added the S-needs-rebase label Jun 26, 2016

cbrewster force-pushed the cbrewster:joint_session_history branch from 414fe1e to 2be8b13 Jun 26, 2016

cbrewster removed the S-needs-rebase label Jun 26, 2016

cbrewster force-pushed the cbrewster:joint_session_history branch from 2be8b13 to e5343c0 Jun 26, 2016

cbrewster force-pushed the cbrewster:joint_session_history branch from e5343c0 to 781265b Jun 27, 2016

cbrewster force-pushed the cbrewster:joint_session_history branch from fea85f6 to 751b04a Jun 27, 2016

cbrewster force-pushed the cbrewster:joint_session_history branch from 2eed173 to 1abdf6e Jun 28, 2016

cbrewster force-pushed the cbrewster:joint_session_history branch from 1abdf6e to 98fb96b Jun 28, 2016

cbrewster force-pushed the cbrewster:joint_session_history branch from 82baa7c to e72d1a5 Jul 21, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jul 21, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 21, 2016

cbrewster force-pushed the cbrewster:joint_session_history branch from e72d1a5 to f131818 Jul 22, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Jul 22, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 22, 2016

bors-servo merged commit f131818 into servo:master Jul 22, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 22, 2016

bors-servo mentioned this pull request Jul 22, 2016

Purge pipelines from distant history. #11893

Closed

cbrewster mentioned this pull request Jul 22, 2016

History API metabug #10992

Open

18 of 24 tasks complete

servo / servo

Implement joint session history #11866

Implement joint session history #11866

cbrewster commented Jun 25, 2016 •

edited by larsbergstrom

highfive commented Jun 25, 2016

highfive commented Jun 25, 2016

bors-servo commented Jun 26, 2016

cbrewster commented Jun 26, 2016

jgraham commented Jun 27, 2016

cbrewster commented Jun 27, 2016

asajeffrey commented Jun 27, 2016

jgraham commented Jun 27, 2016

asajeffrey commented Jun 27, 2016

asajeffrey commented Jun 27, 2016

asajeffrey commented Jun 27, 2016

cbrewster commented Jun 27, 2016

cbrewster commented Jun 27, 2016

cbrewster commented Jun 27, 2016

cbrewster commented Jun 28, 2016 •

edited

asajeffrey commented Jun 28, 2016

asajeffrey commented Jun 28, 2016

cbrewster commented Jun 28, 2016

bors-servo commented Jul 3, 2016

asajeffrey commented Jul 21, 2016

cbrewster commented Jul 21, 2016

asajeffrey commented Jul 21, 2016

asajeffrey commented Jul 21, 2016

bors-servo commented Jul 21, 2016

bors-servo commented Jul 21, 2016

bors-servo commented Jul 21, 2016

asajeffrey commented Jul 22, 2016

cbrewster commented Jul 22, 2016

bors-servo commented Jul 22, 2016

bors-servo commented Jul 22, 2016

bors-servo commented Jul 22, 2016

servo / servo

Join GitHub today

Implement joint session history #11866

Implement joint session history #11866

Conversation

cbrewster commented Jun 25, 2016 • edited by larsbergstrom

highfive commented Jun 25, 2016

highfive commented Jun 25, 2016

bors-servo commented Jun 26, 2016

cbrewster commented Jun 26, 2016

jgraham commented Jun 27, 2016

cbrewster commented Jun 27, 2016

asajeffrey commented Jun 27, 2016

jgraham commented Jun 27, 2016

asajeffrey commented Jun 27, 2016

asajeffrey commented Jun 27, 2016

asajeffrey commented Jun 27, 2016

cbrewster commented Jun 27, 2016

cbrewster commented Jun 27, 2016

cbrewster commented Jun 27, 2016

cbrewster commented Jun 28, 2016 • edited

asajeffrey commented Jun 28, 2016

asajeffrey commented Jun 28, 2016

cbrewster commented Jun 28, 2016

bors-servo commented Jul 3, 2016

asajeffrey commented Jul 21, 2016

cbrewster commented Jul 21, 2016

asajeffrey commented Jul 21, 2016

asajeffrey commented Jul 21, 2016

bors-servo commented Jul 21, 2016

bors-servo commented Jul 21, 2016

bors-servo commented Jul 21, 2016

asajeffrey commented Jul 22, 2016

cbrewster commented Jul 22, 2016

bors-servo commented Jul 22, 2016

bors-servo commented Jul 22, 2016

bors-servo commented Jul 22, 2016

cbrewster commented Jun 25, 2016 •

edited by larsbergstrom

cbrewster commented Jun 28, 2016 •

edited