r? @asajeffrey

cc @ConnorGBrewster

Can someone summarise what that does?

Is history still conserved even if pipelines are purged?
Are non-accessible pipelines (pipeline1 navigate to pipeline2, goBack to pipeline1, navigate to pipeline3, pipeline2 is not accessible anymore) discarded (maybe it was already)?

@paulrouget I believe the idea is that we will keep 3(number can change) entries in the history, any pipelines past that will get closed. Once those pipelines are traversed to again, they are reloaded. This keeps us from having many many pipelines open after navigating a lot.

Once those pipelines are traversed to again, they are reloaded

Ok, so we keep the history even if the pipeline is purged. That's all I was worried about :)

@paulrouget yup! they are reloaded here https://github.com/servo/servo/pull/11893/files#diff-55c92a6a5ba7654ce45fe6fc6c63740fR1416

I'm quite concerned about what happens when pipelines share a script thread. If page A contains a same-origin iframe, which starts out at B1, then navigates to B2, B3 and B4. This will trigger Pipeline::exit on B1, which shares a script thread with A.

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

components/constellation/constellation.rs, line 222 [r1] (raw file):

        id: PipelineId,
    },
    Dead {

The terminology of alive vs dead doesn't seem aligned with the spec. https://html.spec.whatwg.org/multipage/browsers.html#session-history has a note saying "when a Document is not active, it's possible for it to be discarded to free resources," defined in https://html.spec.whatwg.org/multipage/browsers.html#discard-a-document.

components/constellation/constellation.rs, line 224 [r1] (raw file):

    Dead {
        url: Url,
        is_private: bool,

Should the data saved here be LoadData?

Is the privacy associated with the history entry or with the frame? (That is, can we have a frame, some of whose entries are private and some are public?)

components/constellation/constellation.rs, line 244 [r1] (raw file):

    }

    fn load(&mut self,

Can we add a comment saying what the semantics of this function is?

components/constellation/constellation.rs, line 246 [r1] (raw file):

    fn load(&mut self,
            pipeline_id: PipelineId,
            pipelines: &HashMap<PipelineId, Pipeline>,

Passing the pipelines hash map around seems like a break in the abstraction barrier between Frame and Constellation.

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

            pipeline_id: PipelineId,
            pipelines: &HashMap<PipelineId, Pipeline>,
            through_history: bool)

What is this parameter for?

components/constellation/constellation.rs, line 264 [r1] (raw file):

    }

    fn maybe_purge(&mut self, pipelines: &HashMap<PipelineId, Pipeline>) {

Shouldn't this functionality be in Constellation rather than Frame?

components/constellation/constellation.rs, line 273 [r1] (raw file):

                            None => continue,
                        };
                        pipeline.exit();

Doesn't this exit a pipeline even if the script thread is being used by its parent?

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

                    self.handle_load_start_msg(&pipeline_id);
                    self.push_pending_frame(pipeline_id, Some(prev), true);
                    self.compositor_proxy.send(ToCompositorMsg::ChangePageUrl(pipeline_id, url));

Should we set the window URL, even if it's a child iframe that caused this navigation?

Comments from Reviewable

As a general comment, it strikes me that this is one way of handling document discarding, but not the only way. Rather than having a per-frame hard limit on history, we could have a per-constellation soft limit.

@Ms2ger is this still active?

Yes, I'm going to get back to your review comments, hopefully this week.

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

@Ms2ger ping?

Rebased.

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

Review status: 0 of 1 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

components/constellation/constellation.rs, line 305 [r3] (raw file):

    }

    fn maybe_purge(&mut self, pipelines: &HashMap<PipelineId, Pipeline>) {

Should we be purging everything but the previous/next x entries at the joint session history(top-level browsing context) level or at the individual session history/frame(browsing context) level?

Comments from Reviewable

Added some docs for load(); the rest of the comments seem like questions the two of you are more qualified to answer than I am.

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

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

I have no idea what this is doing anymore. @asajeffrey @ConnorGBrewster, wanna take over?

This is quite tricky, especially with sharing script threads (and there are bugs around when script threads are shared, sigh). We may need to postpone this until we get the sharing script threads story straight. Either that or we need to work out how to get a script thread to cope with some of its globals being dead, and some being alive.

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

@asajeffrey is going to take over this work. He's planning to rework script thread sharing which will further impact this, and at this point it probably makes sense for him to just include purging in his plans.

@ConnorGBrewster: document unloading will be in our future 😄

#14211 fixes #633, so now I'm turning my attention to this.