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
Frames in the constellation store LoadData rather than just URLs. #15878
Frames in the constellation store LoadData rather than just URLs. #15878
Conversation
r? @cbrewster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good other than a couple nits
components/constellation/frame.rs
Outdated
@@ -81,7 +80,7 @@ impl Frame { | |||
pub fn update_current(&mut self, pipeline_id: PipelineId, entry: &FrameState) { | |||
self.pipeline_id = pipeline_id; | |||
self.instant = entry.instant; | |||
self.url = entry.url.clone(); | |||
self.load_data = entry.load_data.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make update_current
take ownership of entry
to remove this clone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
self.traverse_to_entry(entry); | ||
(evicted_id, false, None, false) | ||
(None, false, None, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that only one branch returns something other than None
for evicted_id
can we just move close_pipeline
inside the only branch that uses it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afraid not:
error[E0499]: cannot borrow `*self` as mutable more than once at a time
--> /home/ajeffrey/github/asajeffrey/servo/components/constellation/constellation.rs:2165:17
|
2156 | } else if let Some(frame) = self.frames.get_mut(&frame_change.frame_id) {
| ----------- first mutable borrow occurs here
...
2165 | self.close_pipeline(evicted_id, DiscardBrowsingContext::No, ExitPipelineMode::Normal);
| ^^^^ second mutable borrow occurs here
...
2171 | };
| - first borrow ends here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Is it still r=you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes me want to have 2-phase borrowing here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed.
7cbd54c
to
90650da
Compare
components/constellation/frame.rs
Outdated
|
||
/// Is the new document replacing the current document (e.g. a reload) | ||
/// or pushing it into the session history (e.g. a navigation)? | ||
pub replace: Option<FrameState>, | ||
/// It it replacing an existing entry, we store its timestamp. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/It it/If it is/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
90650da
to
1a44a76
Compare
@bors-servo r=cbrewster |
📌 Commit 1a44a76 has been approved by |
…, r=cbrewster Frames in the constellation store LoadData rather than just URLs. <!-- Please describe your changes on the following line: --> Include LoadData in each session history entry, so that when we reload discarded documents we can include POST data etc. --- <!-- 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` does not report any errors - [X] These changes fix #14888 - [X] These changes do not require tests because we don't test document discarding. <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15878) <!-- Reviewable:end -->
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev |
Include LoadData in each session history entry, so that when we reload discarded documents we can include POST data etc.
./mach build -d
does not report any errors./mach test-tidy
does not report any errorsThis change is