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

Replace current session entry when reloading #13167

Merged
merged 1 commit into from Sep 19, 2016

Conversation

cbrewster
Copy link
Contributor

@cbrewster cbrewster commented Sep 3, 2016

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.


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

This change is Reviewable

@highfive
Copy link

highfive commented Sep 3, 2016

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

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 3, 2016
@cbrewster
Copy link
Contributor Author

r? @asajeffrey

@highfive highfive assigned asajeffrey and unassigned SimonSapin Sep 5, 2016
let entry = self.frames.get_mut(&frame_id).map(|frame| {
frame.replace_current(frame_change.new_pipeline_id)
});
if let Some(entry) = entry {
Copy link
Contributor

@paulrouget paulrouget Sep 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@paulrouget
Copy link
Contributor

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

@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Sep 10, 2016
@cbrewster cbrewster removed the S-needs-rebase There are merge conflict errors. label Sep 10, 2016
@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Sep 14, 2016
@cbrewster cbrewster removed the S-needs-rebase There are merge conflict errors. label Sep 17, 2016
@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Sep 19, 2016
@cbrewster cbrewster removed the S-needs-rebase There are merge conflict errors. label Sep 19, 2016
@asajeffrey
Copy link
Member

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
Copy link
Contributor Author

@bors-servo r=asajeffrey

@bors-servo
Copy link
Contributor

📌 Commit e9b2f1b has been approved by asajeffrey

@highfive 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
Copy link
Contributor

⌛ Testing commit e9b2f1b with merge 9876923...

bors-servo pushed a commit that referenced this pull request Sep 19, 2016
…effrey

Replace current session entry when reloading

<!-- Please describe your changes on the following line: -->
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.

---
<!-- 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 #13123 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/13167)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

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

@bors-servo bors-servo merged commit e9b2f1b into servo:master Sep 19, 2016
@highfive 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reloading a page should not create a new pipeline
6 participants