Implement beginnings of the History API

[cbrewster]

New PR for #11220 since Reviewable was getting slow.
r? @asajeffrey

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix #__ (github issue number if applicable).
• There are tests for these changes OR

---

This change is

[highfive]

Heads up! This PR modifies the following files:

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

[bors-servo]

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

[jgraham]

Reviewed 1 of 60 files at r1.
Review status: 1 of 57 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[cbrewster]

hmm so the FrameTreeIterator looks like it doesn't do quite what I need. I will need to rework some of this as the history of child frames in prev/next pipelines are not included in the frame tree, but they will need to be included for the joint session history.

[highfive]

New code was committed to pull request.

[highfive]

New code was committed to pull request.

[cbrewster]

@asajeffrey alright this should actually be ready for review whenever you have some open time. I imagine this will probably need a bit of cleanup before actually merging, but it would be nice to have another pair of eyes run over this just to make sure I am not doing something stupidly.

[highfive]

New code was committed to pull request.

[bors-servo]

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

[bors-servo]

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

[bors-servo]

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

[asajeffrey]

My sketch about how to implement the history iterator...

First, we have forward and back iterators for a single frame, just from

   frame.prev.iter()
   frame.next.iter()

both of which implement Iterator<&FrameEntry>.

Now, define something like:

struct MergeIncreasingIterators<I>(Vec<I>);
impl<I> Iterator for MergeIncreasingIterators<I>
    where I: PeekableIterator, I::Item: PartialOrd
{
    type Item = I::Item;
    fn next(&mut self) -> Option<I::Item> {
        self.0.iter()
            .filter_map(|iter| iter.peek().map(|item| (iter, item)))
            .min_by_key(|(iter, item)| item)
            .and_then(|(iter, item)| iter.next())
    }

and ditto MergeDecreasingIterators.

Implement PartialOrd for FrameEntry just by comparing timestamps, and plug it all together, and bingo an iterator which merges histories. Er, or something like that.

[asajeffrey]

I think this PR would be a lot easier to read if it was split up into it's parts: implement deltas, implement state push, implement joint session histories, etc.

Previously, asajeffrey (Alan Jeffrey) wrote…

My sketch about how to implement the history iterator...

First, we have forward and back iterators for a single frame, just from

   frame.prev.iter()

   frame.next.iter()

both of which implement Iterator<&FrameEntry>.

Now, define something like:

struct MergeIncreasingIterators<I>(Vec<I>);

impl<I> Iterator for MergeIncreasingIterators<I>

    where I: PeekableIterator, I::Item: PartialOrd

{

    type Item = I::Item;

    fn next(&mut self) -> Option<I::Item> {

        self.0.iter()

            .filter_map(|iter| iter.peek().map(|item| (iter, item)))

            .min_by_key(|(iter, item)| item)

            .and_then(|(iter, item)| iter.next())

    }

and ditto MergeDecreasingIterators.

Implement PartialOrd for FrameEntry just by comparing timestamps, and plug it all together, and bingo an iterator which merges histories. Er, or something like that.

---

Reviewed 2 of 60 files at r1, 1 of 4 files at r2, 2 of 11 files at r6, 1 of 3 files at r13.
Review status: 7 of 50 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

components/constellation/constellation.rs, line 229 [r13] (raw file):

struct FrameEntry {
    id: PipelineId,
    context_index: usize,

Not sure about whether we need this in the FrameEntry.

---

components/constellation/constellation.rs, line 231 [r13] (raw file):

    context_index: usize,
    reason: EntryReason,
    time: u64,

Better to use Instant for timestamps?

---

components/constellation/constellation.rs, line 247 [r13] (raw file):

/// Stores the navigation context for a single frame in the frame tree.
pub struct Frame {
    id: FrameId,

Yay, I've been wanting to make this change for a while!

---

components/constellation/constellation.rs, line 268 [r13] (raw file):

        // up the navigation change to each parent.
        self.prev.push(self.current);
        self.current = FrameEntry::new(pipeline_id, 0, EntryReason::Load);

I don't really understand context_index=0 here, should it be self.current.context_index + 1?

---

components/constellation/constellation.rs, line 283 [r13] (raw file):

}

struct HistoryIterator {

I think I was thinking of doing this differently... I'll add my thoughts as a separate comment.

---

components/constellation/constellation.rs, line 284 [r13] (raw file):

struct HistoryIterator {
    stack: Vec<(FrameId, Vec<u64>)>,

What does each stack entry represent?

---

components/constellation/constellation.rs, line 1401 [r13] (raw file):

                            frame.prev.push(old);

                            for _ in 0..delta - 1 {

Equivalently 1.. delta.

---

components/constellation/constellation.rs, line 1423 [r13] (raw file):

                            frame.next.push(old);

                            for _ in 0..delta - 1 {

Ditto.

---

components/constellation/constellation.rs, line 1503 [r13] (raw file):

    fn send_popstate_msg(&mut self, pipeline_id: PipelineId, index: usize) {
        let msg = ConstellationControlMsg::UpdateActiveHistoryEntry(pipeline_id, index);

Why do we need to inform script about the history index?

---

components/script/script_thread.rs, line 2039 [r13] (raw file):

    }

    fn handle_update_active_history_entry(&self, pipeline_id: PipelineId, active_index: usize) {

I don't really understand why script is involved with this, shouldn't it just be the constellation?

---

components/script/script_thread.rs, line 2049 [r13] (raw file):

    }

    fn handle_clear_forward_session_history(&self, pipeline_id: PipelineId) {

Ditto.

---

components/script/dom/history.rs, line 52 [r13] (raw file):

    // require this check
    // https://html.spec.whatwg.org/multipage/#fully-active
    fn is_fully_active(&self) -> ErrorResult {

This check may be problematic as it only gives a snapshot, by the time you call this and get the result, you might not be fully active. (This is just one instance where the specs have a disconnect with a concurrent implementation.)

---

Comments from Reviewable

[asajeffrey]

Got part way through reading the PR. There's a lot of stuff here!

[cbrewster]

I would like to split this out into more PRs. I think it would be good for me to go back through all my changes bit by bit since this is so huge. I do like that idea for the iterator a lot, sadly I still don't think it will work due to my issue where the back iter needs to drop the first entry and push the entry.current.

Previously, asajeffrey (Alan Jeffrey) wrote…

Got part way through reading the PR. There's a lot of stuff here!

---

Review status: 7 of 50 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

components/constellation/constellation.rs, line 268 [r13] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

I don't really understand context_index=0 here, should it be self.current.context_index + 1?

The `context_index` is is only for mapping state changes in the script thread. This here is for when a new pipeline(or entry) is loaded which implies a new script thread. This means that the `context_index` will be 0 for that given pipeline because there is only one `SessionHistory` entry on the script thread `BrowsingContext`.

---

components/constellation/constellation.rs, line 283 [r13] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

I think I was thinking of doing this differently... I'll add my thoughts as a separate comment.

See above

---

components/constellation/constellation.rs, line 284 [r13] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

What does each stack entry represent?

When looking at each entry, the only thing we actually compare is the time each entry occurred at and then we spit out the `FrameId` so each entry represents a `Frame` and the list of when its forward or backward entries occurred.

---

components/constellation/constellation.rs, line 1401 [r13] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Equivalently 1.. delta.

good catch

---

components/constellation/constellation.rs, line 1423 [r13] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Ditto.

good catch

---

components/constellation/constellation.rs, line 1503 [r13] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Why do we need to inform script about the history index?

The script thread for this History API has to store `state` https://html.spec.whatwg.org/multipage/browsers.html#dom-history-pushstate. You can call `pushstate` and `replacestate` and it will store a JS value and create a new session history entry. This has to all be done on the script thread. when we navigate we need to tell the script thread to activate the proper entry so `history.state` returns the correct value and so that the `popstate` event with the state value is fired.

---

components/script/script_thread.rs, line 2039 [r13] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

I don't really understand why script is involved with this, shouldn't it just be the constellation?

See above. This is mainly because of the History API. It might be better to separate these changes out from this PR and do those later.

---

components/script/script_thread.rs, line 2049 [r13] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Ditto.

Same as above.

---

components/script/dom/history.rs, line 52 [r13] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

This check may be problematic as it only gives a snapshot, by the time you call this and get the result, you might not be fully active. (This is just one instance where the specs have a disconnect with a concurrent implementation.)

hmmm do you have any ideas on a fix for that?

---

Comments from Reviewable

[cbrewster]

Closing as I will split this into multiple PRs