Skip to content
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

Session history fixup #20629

Merged
merged 1 commit into from Apr 12, 2018
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

@@ -2453,13 +2453,13 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
self.trim_history(top_level_id);
}

self.notify_history_changed(change.top_level_browsing_context_id);
self.update_frame_tree_if_active(change.top_level_browsing_context_id);
}

fn trim_history(&mut self, top_level_browsing_context_id: TopLevelBrowsingContextId) {
let pipelines_to_evict = {
let session_history = self.joint_session_histories.entry(top_level_browsing_context_id)
.or_insert(JointSessionHistory::new());
let session_history = self.get_joint_session_history(top_level_browsing_context_id);

let history_length = PREFS.get("session-history.max-length").as_u64().unwrap_or(20) as usize;

@@ -2493,8 +2493,7 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
self.close_pipeline(evicted_id, DiscardBrowsingContext::No, ExitPipelineMode::Normal);
}

let session_history = self.joint_session_histories.entry(top_level_browsing_context_id)
.or_insert(JointSessionHistory::new());
let session_history = self.get_joint_session_history(top_level_browsing_context_id);

for (alive_id, dead) in dead_pipelines {
session_history.replace(NeedsToReload::No(alive_id), dead);
@@ -2778,6 +2777,11 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
Some(browsing_context) => browsing_context,
};

{
let session_history = self.get_joint_session_history(browsing_context.top_level_id);
session_history.remove_entries_for_browsing_context(browsing_context_id);
}

This comment has been minimized.

@asajeffrey

asajeffrey Apr 12, 2018

Member

Ah, this is interesting. Does this mean we're now eagerly removing entries from the session history, so the changes to history.length will be immediately visible?

This comment has been minimized.

@cbrewster

cbrewster Apr 12, 2018

Author Member

Yes


if BrowsingContextId::from(browsing_context.top_level_id) == browsing_context_id {
self.event_loops.remove(&browsing_context.top_level_id);
}
@@ -2905,6 +2909,10 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
}
}

fn get_joint_session_history(&mut self, top_level_id: TopLevelBrowsingContextId) -> &mut JointSessionHistory {
self.joint_session_histories.entry(top_level_id).or_insert(JointSessionHistory::new())
}

// Convert a browsing context to a sendable form to pass to the compositor
fn browsing_context_to_sendable(&self, browsing_context_id: BrowsingContextId) -> Option<SendableFrameTree> {
self.browsing_contexts.get(&browsing_context_id).and_then(|browsing_context| {
@@ -42,6 +42,15 @@ impl JointSessionHistory {
diff.replace(&old_reloader, &new_reloader);
}
}

pub fn remove_entries_for_browsing_context(&mut self, context_id: BrowsingContextId) {
self.past.retain(|&SessionHistoryDiff::BrowsingContextDiff { browsing_context_id, .. }| {
browsing_context_id != context_id
});
self.future.retain(|&SessionHistoryDiff::BrowsingContextDiff { browsing_context_id, .. }| {
browsing_context_id != context_id
});
}
}

/// Represents a pending change in a session history, that will be applied
{}
]
],
"html/browsers/history/joint-session-history/joint-session-history-filler.html": [
[
{}
]
],
"html/browsers/history/joint-session-history/joint-session-history-grandchild1.html": [
[
{}
{}
]
],
"html/browsers/history/joint-session-history/joint-session-history-remove-iframe.html": [
[
"/html/browsers/history/joint-session-history/joint-session-history-remove-iframe.html",
{}
]
],
"html/browsers/history/the-history-interface/001.html": [
[
"/html/browsers/history/the-history-interface/001.html",
"c8309aa28105f557d99d7a36e86df977bfe003e9",
"support"
],
"html/browsers/history/joint-session-history/joint-session-history-filler.html": [
"54568c8fe456d139e05620c0639a4e60765c8f90",
"support"
],
"html/browsers/history/joint-session-history/joint-session-history-grandchild1.html": [
"c4b5d65cf89e66fc6df209c7166ff56dd66bcf16",
"support"
"e5087c8e51160b36134efab21ed2fbc200a9697d",
"testharness"
],
"html/browsers/history/joint-session-history/joint-session-history-remove-iframe.html": [
"2ff693526c5637a11658831961c9ff0a738329de",
"testharness"
],
"html/browsers/history/the-history-interface/.gitkeep": [
"da39a3ee5e6b4b0d3255bfef95601890afd80709",
"support"
@@ -0,0 +1,23 @@
<!doctype html>
<meta charset="utf-8">
<title>Joint session history length does not include entries from a removed iframe.</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<body>
<iframe id="frame" src="about:blank"></iframe>
<script>
async_test(function(t) {
t.step_timeout(() => {
var child = document.getElementById("frame");
var old_history_len = history.length;
child.onload = () => {
assert_equals(old_history_len + 1, history.length);
document.body.removeChild(document.getElementById("frame"));
assert_equals(old_history_len, history.length);
t.done();

This comment has been minimized.

@asajeffrey

asajeffrey Apr 12, 2018

Member

Good test! Have you tried on other browsers?

This comment has been minimized.

@cbrewster

cbrewster Apr 12, 2018

Author Member

Works in Firefox; fails in Chrome and Safari.

This comment has been minimized.

@asajeffrey

asajeffrey Apr 12, 2018

Member

Oh that's interesting. I'm pretty sure this is a spec-compliant test.

}
child.src = "joint-session-history-filler.html";
}, 1000);
});
</script>
</body>
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.