Notify embedder when history changes #15794

paulrouget · 2017-03-02T08:21:25Z

WindowMethods::set_page_url is only called when the embedder set the URL. It is not called when the page url is updated. I believe that instead we should just pass the URL to head_parsed.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #15439 #15643 and #15642 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because I'm not sure how to test that

This change is

highfive · 2017-03-02T08:21:28Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs
@fitzgen: components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/dom/htmlbodyelement.rs
@KiChjang: components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/dom/htmlbodyelement.rs

highfive · 2017-03-02T08:21:29Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

paulrouget · 2017-03-08T10:26:06Z

@emilio review ping

emilio

Looks sensible, but I wouldn't think HeadParsed is the message containing the url. Perhaps we should send a message before, when the navigation starts. Probably @asajeffrey wants to take a look?

emilio · 2017-03-08T20:09:38Z

components/compositing/compositor_thread.rs

@@ -105,7 +103,7 @@ pub enum Msg {
    /// A favicon was detected
    NewFavicon(ServoUrl),
    /// <head> tag finished parsing


Please add docs on what the url means.

paulrouget · 2017-03-09T00:36:14Z

We want the final URL, after all the redirections, and once the pipeline is not pending anymore.

paulrouget · 2017-03-09T16:55:01Z

Doc added.

paulrouget · 2017-03-09T16:55:33Z

@asajeffrey r?

asajeffrey · 2017-03-09T17:04:35Z

Do we want the embedder to be notified about intermediate URLs when doing redirection? Or just the final URL?

paulrouget · 2017-03-09T17:12:12Z

Do we want the embedder to be notified about intermediate URLs when doing redirection? Or just the final URL?

That's a good question. Naively I would say no. Just the final URL. This will be used to update the urlbar in the user interface. How does it work in Firefox? Does the urlbar get updated for each redirection?

asajeffrey · 2017-03-09T17:14:03Z

ports/cef/window.rs

+        // FIXME(https://github.com/rust-lang/rust/issues/23338)
+        let mut frame_url = servoframe.url.borrow_mut();
+        *frame_url = url.into_string();
+        let utf16_chars: Vec<u16> = frame_url.encode_utf16().collect();


Can we avoid this conversion to UTF16?

asajeffrey · 2017-03-09T17:14:03Z

ports/cef/window.rs

+        let frame = browser.get_main_frame();
+        let servoframe = frame.downcast();
+        // FIXME(https://github.com/rust-lang/rust/issues/23338)
+        let mut frame_url = servoframe.url.borrow_mut();


This borrow_mut is living a long time, can we reduce its lifetime?

asajeffrey · 2017-03-09T17:15:56Z

I think most browsers just report the final URL, but I can certainly imagine some embedders wanting to display the intermediates. If we give them all the URLs, they can choose whether or not to display them.

paulrouget · 2017-03-09T17:21:38Z

Ok. I can report all the urls. The embedder can assume the latest received url when getting HeadParsed is the final url.

asajeffrey · 2017-03-09T17:40:29Z

Sounds good.

fabricedesre · 2017-03-09T17:43:35Z

I think gecko and others don't report intermediates partly because they all have some protection against endless redirects and that would not make much sense to notify of all the changes.

paulrouget · 2017-03-17T08:58:13Z

I think gecko and others don't report intermediates partly because they all have some protection against endless redirects and that would not make much sense to notify of all the changes.

Not sure it would hurt to report the url anyway?

paulrouget · 2017-03-18T09:03:59Z

I've been thinking a bit more about this.

The reason why I need the url is to update the urlbar.
I also use head_parsed(url) to know when we moved to a new document.
I use load_start(can_go_back, can_go_fwd) and load_end(can_go_back, can_go_fwd) to know when to update the loading indicator and the history state.
I never use set_page_url(url) because it's not correctly implemented.

All of that doesn't make much sense because the history can change without load events (it actually change after load_start and before load_end) and url can change without head_parsed.

So I would rather have this:

load_start, load_end (empty events, no can_go_back/fwd) and load_error (the event exist but is never fired)
head_parsed (doesn't hurt to keep it)
history_changed(HistoryEntries)

With:

struct HistoryEntries {
  current: u32,
  entries: Vec<HistoryEntry>,
}

struct HistoryEntry {
  url: ServoUrl,
  title: Option<String>,
  favicon_url: Option<ServoUrl>
}

I don't see any benefit of being notified every time the pipeline resolve urls. And also, while it's resolving urls, the pipeline is still pending, so there is nothing the embedder can do with this information. What would be more appropriate is to do what other engines do and have a way to be notified of any HTTP activity. Something like this: https://developer.chrome.com/extensions/webRequest

paulrouget · 2017-03-18T09:06:36Z

I'll go ahead and implement this proposal. @asajeffrey @fabricedesre let me know if any of this doesn't make sense to you.

paulrouget · 2017-03-18T18:38:02Z

@asajeffrey I've updated the PR. I'd appreciate some feedback.

asajeffrey · 2017-03-20T21:08:08Z

components/compositing/compositor_thread.rs

+    /// The load of a page has completed
+    LoadComplete,
+    /// The history of a top level browser has changed.
+    HistoryChanged(HistoryEntries),


Is this sending the joint session history?

Sending the whole history every time seems a bit wasteful, most of it won't have changed.

asajeffrey · 2017-03-20T21:08:08Z

components/constellation/constellation.rs

@@ -2141,6 +2135,35 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
        }
    }

+    fn notify_history_changed(&self, frame_id: FrameId) {
+        if let Some(frame) = self.frames.get(&frame_id) {


Can we not use one of the existing iterators for this? This appears to be only sending the session history for the frame that traversed, not the joint session history.

asajeffrey · 2017-03-20T21:08:08Z

ports/cef/window.rs

+
+        let current = history.current;
+        let can_go_back = current > 0;
+        let can_go_forward = current < history.entries.len() - 1;


These are the forward/back settings for the session history of the frame that just navigated, not the joint session history.

paulrouget · 2017-03-21T10:22:31Z

I've updated the PR and now send the whole joint session.

Sending the whole history every time seems a bit wasteful, most of it won't have changed.

It's easier for the embedder though as we don't have to maintain a state and duplicate the navigation logic.

bors-servo · 2017-03-23T12:12:23Z

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

asajeffrey · 2017-04-04T14:26:58Z

@paulrouget should we merge this as-is, or are there any more changes you're wanting to implement?

paulrouget · 2017-04-05T01:01:17Z

should we merge this as-is, or are there any more changes you're wanting to implement?

I need to do some further testing. Your previous comment made me realize that I might have not correctly exposed the flatten history. I think this approach will show the load_data of an iframe when the embedder need the load_data of its parent.

Scenario:

user loads a.html.
a.html has an iframe called 1.html
uses clicked on 1.html and the iframe navigates to 2.html
user clicks on a.html navigate to b.html

The history is:

(a.html + 1.html) > (a.html + 2.html) > (b.html)

We want an array of a.html load_data > a.html load_data > b.html' load_data.

If I'm not mistaken, my PR will include the load_data of the iframe.

I'll test that later today.

asajeffrey · 2017-04-05T14:44:40Z

In the example, shouldn't the session history include the frame id, so it would be:

  (parent: a.html) > (child: 1.html) > (child: 2.html) > (parent: b.html)

child frames can traverse independently from their parent, e.g going forward from 1.html will traverse the child, but not the parent.

cc @cbrewster

cbrewster · 2017-04-05T14:50:42Z

I believe you would end up with something like:

(parent: a.html) > (child: 1.html) > (parent: b.html)

Its important to note that the current entry is actually a group of current entries. Each frame has its own current entry, so for the child 2.html is the current entry which means it will not be present with this implementation. I am not sure whether the embedder would care about that or not.

paulrouget · 2017-04-06T04:09:17Z

The event and its payload are used to update the current url, to know if the user can go back/forward, and to display a list of entries where he can jump to (long press on the back button in Firefox).

So we need to have an array of LoadData with an index of the current position in the history.

When the user navigates within iframes, the user can click back and the iframe will go back. But the LoadData we want are not the LoadData of the iframe being navigated, but of the parent document.

Back to the above scenario:

user loads a.html.
a.html has an iframe called 1.html
user clicked on 1.html and the iframe navigates to 2.html
user clicks on a.html navigate to b.html
user clicks on b.html navigate to c.html

When the user long press the back button, we want to show:

b.html's url and title
a.html's url and title
a.html's url and title

We don't want to show 2.html or 1.html in the drop down menu.

Does it make sense?

The current PR is wrong because it collects the LoadData of the inner documents.

After getting joint_session_past and joint_session_furture, I don't know how to get the LoadData of the parent. Can you help?

asajeffrey · 2017-04-06T15:52:44Z

I think you're wanting the frame id, and the frame tree, so you can present the url, etc. for the top-level frame, not the child frame.

paulrouget · 2017-04-07T05:22:14Z

I've updated the PR.

I think you're wanting the frame id, and the frame tree, so you can present the url, etc. for the top-level frame, not the child frame.

We can't go "up" the frame tree to get the proper FrameState in this case. Reaching the parent pipeline and then getting the frame_id won't work as it will give me a frame when I need a framestate. Instead, when the FrameState is for an iframe, I use the previous FrameState in the history to find the relevant LoadData. See the comment in the PR.

I've tested this locally, and it appears to work as expected.

asajeffrey

Back from vacation, sorry about the delay. Mostly this looks fine, just a few niggles.

asajeffrey · 2017-04-17T15:52:13Z

components/constellation/constellation.rs

+        };
+
+        let mut entries: Vec<LoadData> = self.joint_session_past(top_level_frame_id)
+            .map(&keep_load_data_if_top_frame)


Do we need this &? It makes the function call a dynamic rather than static dispatch.

This callback is called twice. Moved in the first map. How can I use it twice then?

Also, in general, I'd be interested to understand why it's bad to use &.

Oh, perhaps you're okay in this case, since it's a pointer to a closure, so Rust will probably generate a new type for it. In the case of a declared fn foo, .map(&foo) will perform dynamic dispatch, since &foo has type &fn(S) -> T, which blocks other code optimizations.

asajeffrey · 2017-04-17T15:52:13Z

components/constellation/constellation.rs

+        entries.push(current_load_data.clone());
+
+        let mut future_entries = self.joint_session_future(top_level_frame_id)
+            .map(&keep_load_data_if_top_frame)


asajeffrey · 2017-04-17T15:52:13Z

components/constellation/constellation.rs

+            .scan(current_load_data.clone(), &resolve_load_data)
+            .collect();
+
+        entries.append(&mut future_entries);


Do we need to collect then append? Wouldn't it be easier to chain the iterators, then do one collect?

Maybe. How do I append the current entry (using chain(once())?) and also get the size of the past entries (to get the current index)?

Oh rats, you need the length of the session past? Oops, forgot about that. You can still avold doing the unnecessary collect by using entries.extend with an iterator.


        Notify embedder when history changes

paulrouget · 2017-04-18T05:41:28Z

Updated the PR. I didn't change &keep_load_data_if_top_frame. If you know a better solution, let me know.

I used entries.extend.

asajeffrey · 2017-04-25T19:11:50Z

@bors-servo r+

bors-servo · 2017-04-25T19:11:51Z

📌 Commit e493548 has been approved by asajeffrey

bors-servo · 2017-04-25T19:14:32Z

⌛ Testing commit e493548 with merge dc594fa...


        Auto merge of #15794 - paulrouget:head_parsed_url, r=asajeffrey

Notify embedder when history changes `WindowMethods::set_page_url` is only called when the embedder set the URL. It is not called when the page url is updated. I believe that instead we should just pass the URL to `head_parsed`. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #15439 #15643 and #15642 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because I'm not sure how to test that   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15794)

bors-servo · 2017-04-25T19:49:35Z

☀️ 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-msvc-dev
Approved by: asajeffrey
Pushing dc594fa to master...

highfive assigned emilio Mar 2, 2017

highfive added the S-awaiting-review label Mar 2, 2017

emilio reviewed Mar 8, 2017

View changes

components/compositing/compositor_thread.rs Outdated

@@ -105,7 +103,7 @@ pub enum Msg {

/// A favicon was detected

NewFavicon(ServoUrl),

/// <head> tag finished parsing

This comment has been minimized.

Sign in to view

emilio Mar 8, 2017
Member

Please add docs on what the url means.

paulrouget force-pushed the paulrouget:head_parsed_url branch from 9490cde to d97b74e Mar 9, 2017

asajeffrey reviewed Mar 9, 2017

View changes

paulrouget force-pushed the paulrouget:head_parsed_url branch 2 times, most recently from 25a5372 to aac02df Mar 18, 2017

paulrouget changed the title ~~update URL via head_parsed~~ Notify embedder when history changes Mar 18, 2017

paulrouget force-pushed the paulrouget:head_parsed_url branch from aac02df to 75f70c4 Mar 18, 2017

paulrouget force-pushed the paulrouget:head_parsed_url branch from 75f70c4 to 165e129 Mar 18, 2017

asajeffrey reviewed Mar 20, 2017

View changes

paulrouget force-pushed the paulrouget:head_parsed_url branch from 165e129 to 85e38f7 Mar 21, 2017

asajeffrey removed the S-awaiting-review label Mar 31, 2017

asajeffrey added S-awaiting-answer and removed S-needs-rebase labels Apr 4, 2017

paulrouget force-pushed the paulrouget:head_parsed_url branch from 86f5fd3 to 865a6f3 Apr 7, 2017

highfive added the S-awaiting-review label Apr 7, 2017

paulrouget force-pushed the paulrouget:head_parsed_url branch from 865a6f3 to dedef1b Apr 7, 2017

asajeffrey reviewed Apr 17, 2017

View changes

asajeffrey removed the S-awaiting-review label Apr 17, 2017

Notify embedder when history changes

Loading status checks…

e493548

paulrouget force-pushed the paulrouget:head_parsed_url branch from dedef1b to e493548 Apr 18, 2017

highfive added the S-awaiting-review label Apr 18, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-awaiting-answer labels Apr 25, 2017

bors-servo merged commit e493548 into servo:master Apr 25, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Apr 25, 2017

bors-servo mentioned this pull request Apr 25, 2017

Avoid a constellation roundtrip when a same-origin iframe finishes loading #16109

Closed

4 of 4 tasks complete

servo / servo

Join GitHub today

Notify embedder when history changes #15794

Notify embedder when history changes #15794

Conversation

paulrouget commented Mar 2, 2017 • edited

highfive commented Mar 2, 2017

highfive commented Mar 2, 2017

paulrouget commented Mar 8, 2017

emilio left a comment

This comment has been minimized.

paulrouget commented Mar 9, 2017

paulrouget commented Mar 9, 2017

paulrouget commented Mar 9, 2017

asajeffrey commented Mar 9, 2017

paulrouget commented Mar 9, 2017

This comment has been minimized.

This comment has been minimized.

asajeffrey commented Mar 9, 2017

paulrouget commented Mar 9, 2017

asajeffrey commented Mar 9, 2017

fabricedesre commented Mar 9, 2017

paulrouget commented Mar 17, 2017

paulrouget commented Mar 18, 2017

paulrouget commented Mar 18, 2017

paulrouget commented Mar 18, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

paulrouget commented Mar 21, 2017

bors-servo commented Mar 23, 2017

asajeffrey commented Apr 4, 2017

paulrouget commented Apr 5, 2017

asajeffrey commented Apr 5, 2017

cbrewster commented Apr 5, 2017

paulrouget commented Apr 6, 2017

asajeffrey commented Apr 6, 2017

paulrouget commented Apr 7, 2017

asajeffrey left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

paulrouget commented Apr 18, 2017

asajeffrey commented Apr 25, 2017

bors-servo commented Apr 25, 2017

bors-servo commented Apr 25, 2017

bors-servo commented Apr 25, 2017

paulrouget commented Mar 2, 2017 •

edited