Replace current session entry when reloading by cbrewster · Pull Request #13167 · servo/servo

cbrewster · 2016-09-03T04:31:25Z

This PR adds a replacement option when navigating. It replaces the current session history entry after a new page has been loaded. This will prevent reloading from adding a new entry to the session history.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix reloading a page should not create a new pipeline #13123 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-09-03T04:31:29Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs
@KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/htmlanchorelement.rs, components/script/dom/htmlformelement.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/script_thread.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/location.rs

cbrewster · 2016-09-05T18:32:06Z

r? @asajeffrey

paulrouget · 2016-09-06T05:27:27Z

components/constellation/constellation.rs

+                let entry = self.frames.get_mut(&frame_id).map(|frame| {
+                    frame.replace_current(frame_change.new_pipeline_id)
+                });
+                if let Some(entry) = entry {


I'm not sure, but is entry ever supposed to be none? unwrap or warn?

I should probably warn, the None case will only happen if the frame lookup fails.

paulrouget · 2016-09-06T05:28:22Z

This looks good to me, and pass my local tests.

bors-servo · 2016-09-10T02:15:13Z

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

bors-servo · 2016-09-14T07:43:44Z

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

bors-servo · 2016-09-19T03:59:23Z

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

asajeffrey · 2016-09-19T19:01:03Z

Nitpicking about adding some comments, apart from that lgtm! You don't have to add the comments, but I think they would help. You can r=me.

Reviewed 2 of 10 files at r1, 8 of 8 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

components/constellation/constellation.rs, line 1389 at r2 (raw file):

    }

    fn load_url(&mut self, source_id: PipelineId, load_data: LoadData, replace: bool) -> Option<PipelineId> {

A comment somewhere in this file about what replace is for would be nice. Perhaps on this function?

components/script/script_thread.rs, line 232 at r2 (raw file):

    /// Begins a content-initiated load on the specified pipeline (only
    /// dispatched to ScriptThread).
    Navigate(PipelineId, LoadData, bool),

A comment saying what the bool is for would be nice.

components/script_traits/lib.rs, line 462 at r2 (raw file):

    /// Whether this iframe is a mozbrowser iframe
    pub frame_type: FrameType,
    /// Wether this load should replace the current entry (reload)

A comment about reload not adding an entry to the session history would be nice.

components/script_traits/script_msg.rs, line 86 at r2 (raw file):

    /// has been dispatched.
    LoadComplete(PipelineId),
    /// A new load has been requested, with an option to replace the current entry once loaded.

A comment about reload not adding an entry to the session history would be nice.

Comments from Reviewable

cbrewster · 2016-09-19T19:23:25Z

@bors-servo r=asajeffrey

bors-servo · 2016-09-19T19:23:26Z

📌 Commit e9b2f1b has been approved by asajeffrey

bors-servo · 2016-09-19T19:32:45Z

⌛ Testing commit e9b2f1b with merge 9876923...

…effrey Replace current session entry when reloading  This PR adds a replacement option when navigating. It replaces the current session history entry after a new page has been loaded. This will prevent reloading from adding a new entry to the session history. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13123 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13167)

bors-servo · 2016-09-19T20:37:14Z

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

highfive assigned SimonSapin Sep 3, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 3, 2016

highfive assigned asajeffrey and unassigned SimonSapin Sep 5, 2016

paulrouget reviewed Sep 6, 2016
View reviewed changes

cbrewster force-pushed the reload_replace_current branch from 89767c1 to abeff2c Compare September 7, 2016 20:32

highfive added the S-needs-rebase There are merge conflict errors. label Sep 10, 2016

cbrewster force-pushed the reload_replace_current branch from abeff2c to 2f7b72e Compare September 10, 2016 23:03

cbrewster removed the S-needs-rebase There are merge conflict errors. label Sep 10, 2016

paulrouget mentioned this pull request Sep 12, 2016

Pipeline visibility logic change #13227

Merged

5 tasks

highfive added the S-needs-rebase There are merge conflict errors. label Sep 14, 2016

cbrewster force-pushed the reload_replace_current branch from 2f7b72e to 4bfff66 Compare September 17, 2016 21:26

cbrewster removed the S-needs-rebase There are merge conflict errors. label Sep 17, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Sep 19, 2016

cbrewster force-pushed the reload_replace_current branch from 4bfff66 to 0520ee4 Compare September 19, 2016 18:43

cbrewster removed the S-needs-rebase There are merge conflict errors. label Sep 19, 2016

Replace current session entry for reloads

e9b2f1b

cbrewster force-pushed the reload_replace_current branch from 0520ee4 to e9b2f1b Compare September 19, 2016 19:23

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 19, 2016

bors-servo merged commit e9b2f1b into servo:master Sep 19, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Sep 19, 2016

bors-servo mentioned this pull request Sep 19, 2016

Implement referrer policy delivery via noreferrer link relation #12493

Merged

4 tasks

Uh oh!

Conversation

cbrewster commented Sep 3, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Sep 3, 2016

Uh oh!

cbrewster commented Sep 5, 2016

Uh oh!

paulrouget Sep 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbrewster Sep 6, 2016

Choose a reason for hiding this comment

Uh oh!

cbrewster Sep 7, 2016

Choose a reason for hiding this comment

Uh oh!

paulrouget commented Sep 6, 2016

Uh oh!

bors-servo commented Sep 10, 2016

Uh oh!

bors-servo commented Sep 14, 2016

Uh oh!

bors-servo commented Sep 19, 2016

Uh oh!

asajeffrey commented Sep 19, 2016

Uh oh!

cbrewster commented Sep 19, 2016

Uh oh!

bors-servo commented Sep 19, 2016

Uh oh!

bors-servo commented Sep 19, 2016

Uh oh!

bors-servo commented Sep 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cbrewster commented Sep 3, 2016 •

edited by larsbergstrom

Loading

paulrouget Sep 6, 2016 •

edited

Loading