Frame should store some of the data that is currently in Pipeline #14692 #21559

mandreyel · 2018-08-31T19:16:39Z

Apologies, meant to land it sooner but deadline at work got hectic.

So I moved the Pipeline::{visible, is_private, parent_info} fields (size was moved earlier) to BrowsingContext, and renamed them where appropriate (and did some minor refactoring on the side, hope that's alright).

This introduced some complications, because when a pipeline is spawned for a browsing context that does not yet exist, the browsing context won't be constructed until after pipeline has made its document active. Thus, values for the fields that used to be in Pipeline and are now in BrowsingContext could not be easily retrieved when constructing the BrowsingContext (since in most cases they were only available when spawning a pipeline).
For this reason, I've put these fields in SessionHistoryChange since one is always created and added to Constellation::pending_changes when a new pipeline is created, so it provides an easy way to forward the necessary values to new BrowsingContexts.

Though frankly I'm not sure I like expanding SessionHistoryChange's purpose to serve as a crutch to construct browsing contexts, so a way to uncouple purposes would be to separately store the values for a to-be-created BrowsingContext in a collection of structs in Constellation and consume them when a new BrowsingContext is created. Here's a PoC: mandreyel@6fa2160.
I didn't include this by default because it introduces a little overhead. Perhaps PendingBrowsingContextInfo could be stored as an Option<> next to a SessionHistoryChange in Constellation::pending_changes? That'd uncouple the two structs but not incur any overhead.

I don't think it's finished, so I've marked some areas where I need input on small matters with TODO(mandreyel), but the general idea is done. I'll be sure to squash commits when no further changes need be done!

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14692.

These changes do not require tests because no new features or behaviour were introduced.

This change is

highfive · 2018-08-31T19:16:46Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @gw3583 (or someone else) soon.

highfive · 2018-08-31T19:16:46Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/session_history.rs, components/constellation/browsingcontext.rs, components/constellation/pipeline.rs, components/constellation/constellation.rs
@cbrewster: components/constellation/session_history.rs, components/constellation/browsingcontext.rs, components/constellation/pipeline.rs, components/constellation/constellation.rs
@paulrouget: components/constellation/session_history.rs, components/constellation/browsingcontext.rs, components/constellation/pipeline.rs, components/constellation/constellation.rs

jdm · 2018-08-31T20:23:47Z

r? @cbrewster
Connor, if you don't have time to review this, we can find someone else.

cbrewster

Thank you for working on this, I think this is a good improvement.

There are quite a few times where we are now nesting lookups. pipeline ID -> pipeline -> BC ID -> BC.

I would prefer trying to keep things more flat that nesting.

Ex:

let browsing_context_id = self.pipelines.get(&pipeline_id) {
    Some(pipeline) => pipeline.browsing_context_id,
    None => warn!(...),
};

let (...) = match self.browsing_contexts.get(&browsing_context_id) {
    Some(browsing_context) => (...),
    None => warn!(...),
};

cbrewster · 2018-08-31T22:02:03Z

components/constellation/constellation.rs

@@ -1244,13 +1239,17 @@ where
                    warn!("Sending reply to get browsing context failed ({:?}).", e);
                }
            },
+            // TODO(mandreyel): maybe change semantics of this message to


This would be a good change and could remove some extra messages sent to the constellation.
See:

servo/components/script/script_thread.rs

Lines 2115 to 2136 in 2351872

fn remote_window_proxy(&self,

global_to_clone: &GlobalScope,

top_level_browsing_context_id: TopLevelBrowsingContextId,

pipeline_id: PipelineId,

opener: Option<BrowsingContextId>)

-> Option<DomRoot<WindowProxy>>

{

let browsing_context_id = self.ask_constellation_for_browsing_context_id(pipeline_id)?;

if let Some(window_proxy) = self.window_proxies.borrow().get(&browsing_context_id) {

return Some(DomRoot::from_ref(window_proxy));

}

let parent = self.ask_constellation_for_parent_info(pipeline_id).and_then(|parent_id| {

self.remote_window_proxy(global_to_clone, top_level_browsing_context_id, parent_id, opener)

});

let window_proxy = WindowProxy::new_dissimilar_origin(global_to_clone,

browsing_context_id,

top_level_browsing_context_id,

parent.r(),

opener);

self.window_proxies.borrow_mut().insert(browsing_context_id, Dom::from_ref(&*window_proxy));

Some(window_proxy)

}

Specifically, we shouldn't need to have this line:

servo/components/script/script_thread.rs

Line 2122 in 2351872

let browsing_context_id = self.ask_constellation_for_browsing_context_id(pipeline_id)?;

I'm not sure how involved this change would be, so it may be a good followup.

I think I'll leave this to another PR (I'd like to do it!) to keep things neatly isolated, if that's alright.

Hmm, yeah, this could actually be coalesced into a single message to constellation. Something like ScriptMsg::GetBrowsingContextInfo(PipelineId) that returns the most relevant fields (in this case browsing_context_id and parent_pipeline_id).

Exactly, this would be great as a followup.

cbrewster · 2018-08-31T22:02:03Z

components/constellation/constellation.rs

-            let pipeline_id = browsing_context.map(|browsing_context| browsing_context.pipeline_id);
-            (window_size, pipeline_id)
+            let window_size = browsing_context.and_then(|context| context.size);
+            // TODO(mandreyel): browsing_context_id was originally


You are correct, the parent should always be None since we get the top level browsing context. You can remove the code that gets the parent pipeline id and change the call to self.new_pipeline to pass None as the parent info.

cbrewster · 2018-08-31T22:02:03Z

components/constellation/constellation.rs

-                None => return debug!("Pipeline {} has no parent.", pipeline_id),
+            Some(pipeline) => match self.browsing_contexts.get(&pipeline.browsing_context_id) {
+                Some(browsing_context) => (browsing_context.id, browsing_context.parent_pipeline_id),
+                // TODO(mandreyel): message?


This message looks good.

cbrewster · 2018-08-31T22:02:03Z

components/constellation/constellation.rs

+                            ),
+                            None => {
+                                return warn!(
+                                    // TODO(mandreyel): message?


This message looks good.

cbrewster · 2018-08-31T22:02:03Z

components/constellation/constellation.rs

+                match self.browsing_contexts.get(&parent_browsing_context_id) {
+                    Some(browsing_context) => (browsing_context.is_private, browsing_context.is_visible),
+                    None => {
+                        // TODO(mandreyel): message?


"New iframe loaded in closed browsing context {}.", ...

cbrewster · 2018-08-31T22:02:03Z

components/constellation/constellation.rs

+                    pipeline.browsing_context_id,
+                    browsing_context.parent_pipeline_id,
+                ),
+                // TODO(mandreyel): message?


Pipeline {} focused after browsing context {:?} was closed

cbrewster · 2018-08-31T22:02:03Z

components/constellation/constellation.rs

@@ -2664,12 +2774,26 @@ where
        }
    }

+    fn handle_focus_msg(&mut self, pipeline_id: PipelineId) {


Would you be willing to also change the constellation to track a focused BrowsingContext rather than a focused Pipeline? If not, that can be done as a followup.

Yes, in fact that's the next issue I want to be working on but I thought I'd keep that as a separate PR :)

Sounds good!

cbrewster · 2018-08-31T22:02:03Z

components/constellation/constellation.rs

@@ -2717,24 +2834,41 @@ where
            },
        };

-        let child_pipeline_ids: Vec<PipelineId> = self
+        // TODO(mandreyel): can we make a mutable version of


Yes, this would be a good solution

I think this could be a separate PR since it's a minor optimization and not directly related to this PR. What do you think?

That would be fine.

cbrewster · 2018-08-31T22:02:03Z

components/constellation/constellation.rs

+                    browsing_context.id,
+                    browsing_context.parent_pipeline_id,
+                ),
+                // TODO(mandreyel): is this warning message ok?


I would have it be:

warn!("Visibility change for closed browsing context {:?}.", pipeline.browsing_context_id)

It's good to include a little context in the warning message (like "visibility change") so we can quickly figure out where the warning was triggered.

cbrewster · 2018-08-31T22:02:03Z

components/constellation/pipeline.rs

            load_data: load_data,
            history_state_id: None,
            history_states: HashSet::new(),
        };

-        pipeline.notify_visibility();


It may be good to keep this here and pass in the visibility to this function, I am worried that there may be times where Pipeline::new is called but notify_visibility is not.

Yes, I was a bit torn on this. On the one hand, it seems to me that the "constructor" shoudn't do a lot of extra work apart from setting up the object, but on the other hand it is indeed error prone to let the user take care of always calling notify_visibility. I guess avoiding potential errors is a higher priority, so I'll change it back.

Agreed, Ideally this would be inside of Constellation::new_pipeline and that would be the only way pipelines are constructed in the constellation, but multiple edge cases caused us to use Pipeline::new directly in multiple placed

cbrewster · 2018-08-31T22:03:58Z

I would prefer adding a PendingBrowsingContextInfo struct that is an optional on the SessionHistoryChange struct rather than adding another HashMap to the constellation.

mandreyel · 2018-08-31T23:43:21Z

Thanks a lot for taking time to look at this! I'll try to implement the fixes asap.

mandreyel · 2018-09-01T01:31:46Z

Okay, I did a few changes. Looking at it now, it does not seem like encapsulating the fields in SessionHistoryChange for new browsing contexts is as revolutionary an idea as I had thought, but I guess it adds some explicitness, which is always good! 🍻

Can you please double-check that the places I've added Some(NewBrowsingContextInfo) when creating a new SessionHistoryChange are correct? More specifically, I'm asking whether I'm correct that that's when new BrowsingContexts don't yet exist and are expected to be constructed. I think there are a few more TODO(mandreyel)s in there, too.

Thanks!

mandreyel · 2018-09-01T09:06:53Z

Oh, damn, I see I messed up the commit history and introduced a bug. Let me fix that real quick.

EDIT: OK, done.

cbrewster

Nearly there! Make sure to remove the leftover TODO: message comments and then I think this should be ready.

It would be great if you could create issues for the followups you are planning to do (if one does not already exist).

cbrewster · 2018-09-01T15:02:08Z

components/constellation/pipeline.rs

            load_data: load_data,
            history_state_id: None,
            history_states: HashSet::new(),
        };

-        pipeline.notify_visibility();


Agreed, Ideally this would be inside of Constellation::new_pipeline and that would be the only way pipelines are constructed in the constellation, but multiple edge cases caused us to use Pipeline::new directly in multiple placed

cbrewster · 2018-09-01T15:02:08Z

components/constellation/constellation.rs

@@ -1244,13 +1239,17 @@ where
                    warn!("Sending reply to get browsing context failed ({:?}).", e);
                }
            },
+            // TODO(mandreyel): maybe change semantics of this message to


Exactly, this would be great as a followup.

cbrewster · 2018-09-01T15:02:08Z

components/constellation/constellation.rs

+                        browsing_context.is_visible,
+                    ),
+                    None => {
+                        // TODO(mandreyel): message?


Please remove the TODO: message comments

cbrewster · 2018-09-01T15:02:08Z

components/constellation/constellation.rs

@@ -2664,12 +2774,26 @@ where
        }
    }

+    fn handle_focus_msg(&mut self, pipeline_id: PipelineId) {


Sounds good!

cbrewster · 2018-09-01T15:02:08Z

components/constellation/constellation.rs

@@ -2717,24 +2834,41 @@ where
            },
        };

-        let child_pipeline_ids: Vec<PipelineId> = self
+        // TODO(mandreyel): can we make a mutable version of


That would be fine.

cbrewster

Sorry I missed the changes for the NewBrowsingContextInfo. This looks much nicer.

cbrewster · 2018-09-01T15:12:59Z

components/constellation/constellation.rs

+            //
+            // If this browsing context didn't exist, insert a pending info
+            // entry so that we can construct it later.
+            new_browsing_context_info: Some(NewBrowsingContextInfo {


This can be None since the browsing_context_id is from an already existing browsing context.

cbrewster · 2018-09-01T15:12:59Z

components/constellation/constellation.rs

                    replace: Some(NeedsToReload::Yes(pipeline_id, load_data.clone())),
-                    is_private: is_private,
-                    is_visible: is_visible,
+                    // TODO(mandreyel): browsing context must exist at this


Yes, it must exist here.

cbrewster · 2018-09-01T15:12:59Z

components/constellation/browsingcontext.rs

@@ -8,6 +8,27 @@ use pipeline::Pipeline;
 use std::collections::{HashMap, HashSet};
 use style_traits::CSSPixel;

+// TODO(mandreyel): is this explanation lucid enough?


Sounds good.

cbrewster · 2018-09-01T15:12:59Z

components/constellation/constellation.rs

+                let new_context_info = match change.new_browsing_context_info {
+                    Some(new_context_info) => new_context_info,
+                    None => {
+                        return warn!(


It's unfortunate that we have to add this check here, but I'm not sure of a way around it at the moment.

Yeah, I agree. While overall the introduction of NewBrowsingContextInfo makes the code cleaner and more explicit, this is an unfortunate side effect.
It could be solved by moving parent_pipeline_id back to SessionHistoryChange and always setting it so that it's available in this function, but that kind of muddies the semantics, since most of the time the field isn't actually needed by whoever uses SessionHistoryChange.

cbrewster · 2018-09-01T15:13:12Z

@bors-servo try

bors-servo · 2018-09-01T15:13:17Z

⌛ Trying commit 2acc818 with merge 511004c...


        Auto merge of #21559 - mandreyel:pipeline-fields-to-browsingcontext, …

…r=<try> Frame should store some of the data that is currently in Pipeline #14692  Apologies, meant to land it sooner but deadline at work got hectic. So I moved the `Pipeline::{visible, is_private, parent_info}` fields (`size` was moved earlier) to `BrowsingContext`, and renamed them where appropriate (and did some minor refactoring on the side, hope that's alright). This introduced some complications, because when a pipeline is spawned for a browsing context that does not yet exist, the browsing context won't be constructed until after pipeline has made its document active. Thus, values for the fields that used to be in `Pipeline` and are now in `BrowsingContext` could not be easily retrieved when constructing the `BrowsingContext` (since in most cases they were only available when spawning a pipeline). For this reason, I've put these fields in `SessionHistoryChange` since one is always created and added to `Constellation::pending_changes` when a new pipeline is created, so it provides an easy way to forward the necessary values to new `BrowsingContext`s. Though frankly I'm not sure I like expanding `SessionHistoryChange`'s purpose to serve as a crutch to construct browsing contexts, so a way to uncouple purposes would be to separately store the values for a to-be-created `BrowsingContext` in a collection of structs in `Constellation` and consume them when a new `BrowsingContext` is created. Here's a PoC: mandreyel@6fa2160. I didn't include this by default because it introduces a little overhead. Perhaps `PendingBrowsingContextInfo` could be stored as an `Option<>` next to a `SessionHistoryChange` in `Constellation::pending_changes`? That'd uncouple the two structs but not incur any overhead. I don't think it's finished, so I've marked some areas where I need input on small matters with `TODO(mandreyel)`, but the general idea is done. I'll be sure to squash commits when no further changes need be done! ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14692.  - [x] These changes do not require tests because no new features or behaviour were introduced.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21559)

bors-servo · 2018-09-01T15:45:48Z

💔 Test failed - linux-rel-css

bors-servo · 2018-09-13T05:13:14Z

⌛ Testing commit b051df7 with merge 7b93d08...


        Auto merge of #21559 - mandreyel:pipeline-fields-to-browsingcontext, …

…r=cbrewster Frame should store some of the data that is currently in Pipeline #14692  Apologies, meant to land it sooner but deadline at work got hectic. So I moved the `Pipeline::{visible, is_private, parent_info}` fields (`size` was moved earlier) to `BrowsingContext`, and renamed them where appropriate (and did some minor refactoring on the side, hope that's alright). This introduced some complications, because when a pipeline is spawned for a browsing context that does not yet exist, the browsing context won't be constructed until after pipeline has made its document active. Thus, values for the fields that used to be in `Pipeline` and are now in `BrowsingContext` could not be easily retrieved when constructing the `BrowsingContext` (since in most cases they were only available when spawning a pipeline). For this reason, I've put these fields in `SessionHistoryChange` since one is always created and added to `Constellation::pending_changes` when a new pipeline is created, so it provides an easy way to forward the necessary values to new `BrowsingContext`s. Though frankly I'm not sure I like expanding `SessionHistoryChange`'s purpose to serve as a crutch to construct browsing contexts, so a way to uncouple purposes would be to separately store the values for a to-be-created `BrowsingContext` in a collection of structs in `Constellation` and consume them when a new `BrowsingContext` is created. Here's a PoC: mandreyel@6fa2160. I didn't include this by default because it introduces a little overhead. Perhaps `PendingBrowsingContextInfo` could be stored as an `Option<>` next to a `SessionHistoryChange` in `Constellation::pending_changes`? That'd uncouple the two structs but not incur any overhead. I don't think it's finished, so I've marked some areas where I need input on small matters with `TODO(mandreyel)`, but the general idea is done. I'll be sure to squash commits when no further changes need be done! ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14692.  - [x] These changes do not require tests because no new features or behaviour were introduced.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21559)

bors-servo · 2018-09-13T05:47:49Z

💔 Test failed - mac-rel-wpt2

jdm · 2018-09-13T06:13:32Z

I am not sure whether to be worried about this:

  ▶ OK [expected TIMEOUT] /html/browsers/history/the-history-interface/traverse_the_history_write_after_load_1.html

  ▶ Unexpected subtest result in /html/browsers/history/the-history-interface/traverse_the_history_write_after_load_1.html:
  │ FAIL [expected TIMEOUT] Traverse the history after document.write after the load event
  │   → assert_array_equals: Pages opened during history navigation property 2, expected 2 but got 3
  │ 
  │ start_test_wait</check_result<@http://web-platform.test:8000/html/browsers/history/the-history-interface/traverse_the_history_write_after_load_1.html:19:11
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1558:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1582:20

mandreyel · 2018-09-13T08:29:21Z

@jdm Hmm :/ I ran the /html/browsers test suite tens of times on both master (to be more precise, the last upstream commit before mine) and my branch (after the above fix), and there were a few history traversal tests (along with a few others) that failed pretty consistently on both branches for me (/html/browsers/history/the-history-interface/traverse_the_history_{1,2,3,4,5}.html) and it was always the same tests on both (they didn't all fail at the same time though).
I must admit I have never seen this one fail on either branch and am trying to reproduce locally now.
Were there problems with history traversal before or is this new to you?

mandreyel · 2018-09-13T09:00:22Z

E.g. this is what I get on master (2351872) on a debug build:

./mach test-wpt tests/wpt/web-platform-tests/html/browsers/history
Running 92 tests in web-platform-tests

  ▶ Unexpected subtest result in /html/browsers/history/the-history-interface/traverse_the_history_3.html:
  └ PASS [expected FAIL] Multiple history traversals, last would be aborted

  ▶ Unexpected subtest result in /html/browsers/history/the-history-interface/traverse_the_history_5.html:
  └ PASS [expected FAIL] Multiple history traversals, last would be aborted

  ▶ Unexpected subtest result in /html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html:
  │ FAIL [expected PASS] Set HTTP URL frame location.protocol to x
  │   → assert_equals: expected "http:" but got "x"
  │ 
  │ self.onload/</</frame.onload</<@http://web-platform.test:8000/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html:25:11
  │ Test.prototype.step_timeout/<@http://web-platform.test:8000/resources/testharness.js:1611:20
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1553:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1577:20

Ran 92 tests finished in 36.0 seconds.
  • 89 ran as expected. 2 tests skipped.
  • 3 tests had unexpected subtest results

jdm · 2018-09-13T13:18:42Z

I'm willing to believe it's a latest intermittent failure that just hasn't hit us before.
@bors-servo retry

bors-servo · 2018-09-13T13:18:49Z

⌛ Testing commit b051df7 with merge fcdcd3d...


        Auto merge of #21559 - mandreyel:pipeline-fields-to-browsingcontext, …

…r=cbrewster Frame should store some of the data that is currently in Pipeline #14692  Apologies, meant to land it sooner but deadline at work got hectic. So I moved the `Pipeline::{visible, is_private, parent_info}` fields (`size` was moved earlier) to `BrowsingContext`, and renamed them where appropriate (and did some minor refactoring on the side, hope that's alright). This introduced some complications, because when a pipeline is spawned for a browsing context that does not yet exist, the browsing context won't be constructed until after pipeline has made its document active. Thus, values for the fields that used to be in `Pipeline` and are now in `BrowsingContext` could not be easily retrieved when constructing the `BrowsingContext` (since in most cases they were only available when spawning a pipeline). For this reason, I've put these fields in `SessionHistoryChange` since one is always created and added to `Constellation::pending_changes` when a new pipeline is created, so it provides an easy way to forward the necessary values to new `BrowsingContext`s. Though frankly I'm not sure I like expanding `SessionHistoryChange`'s purpose to serve as a crutch to construct browsing contexts, so a way to uncouple purposes would be to separately store the values for a to-be-created `BrowsingContext` in a collection of structs in `Constellation` and consume them when a new `BrowsingContext` is created. Here's a PoC: mandreyel@6fa2160. I didn't include this by default because it introduces a little overhead. Perhaps `PendingBrowsingContextInfo` could be stored as an `Option<>` next to a `SessionHistoryChange` in `Constellation::pending_changes`? That'd uncouple the two structs but not incur any overhead. I don't think it's finished, so I've marked some areas where I need input on small matters with `TODO(mandreyel)`, but the general idea is done. I'll be sure to squash commits when no further changes need be done! ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14692.  - [x] These changes do not require tests because no new features or behaviour were introduced.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21559)

nox · 2018-09-13T13:20:36Z

@jdm There was a different PR which failed because of spurious failures (#21687) and I was waiting for its remaining builds to finish on Buildbot to retry it. :(

bors-servo · 2018-09-13T13:38:19Z

💔 Test failed - linux-rel-css

jdm · 2018-09-13T13:50:47Z

@bors-servo retry


        Auto merge of #21559 - mandreyel:pipeline-fields-to-browsingcontext, …

…r=cbrewster Frame should store some of the data that is currently in Pipeline #14692  Apologies, meant to land it sooner but deadline at work got hectic. So I moved the `Pipeline::{visible, is_private, parent_info}` fields (`size` was moved earlier) to `BrowsingContext`, and renamed them where appropriate (and did some minor refactoring on the side, hope that's alright). This introduced some complications, because when a pipeline is spawned for a browsing context that does not yet exist, the browsing context won't be constructed until after pipeline has made its document active. Thus, values for the fields that used to be in `Pipeline` and are now in `BrowsingContext` could not be easily retrieved when constructing the `BrowsingContext` (since in most cases they were only available when spawning a pipeline). For this reason, I've put these fields in `SessionHistoryChange` since one is always created and added to `Constellation::pending_changes` when a new pipeline is created, so it provides an easy way to forward the necessary values to new `BrowsingContext`s. Though frankly I'm not sure I like expanding `SessionHistoryChange`'s purpose to serve as a crutch to construct browsing contexts, so a way to uncouple purposes would be to separately store the values for a to-be-created `BrowsingContext` in a collection of structs in `Constellation` and consume them when a new `BrowsingContext` is created. Here's a PoC: mandreyel@6fa2160. I didn't include this by default because it introduces a little overhead. Perhaps `PendingBrowsingContextInfo` could be stored as an `Option<>` next to a `SessionHistoryChange` in `Constellation::pending_changes`? That'd uncouple the two structs but not incur any overhead. I don't think it's finished, so I've marked some areas where I need input on small matters with `TODO(mandreyel)`, but the general idea is done. I'll be sure to squash commits when no further changes need be done! ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14692.  - [x] These changes do not require tests because no new features or behaviour were introduced.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21559)

bors-servo · 2018-09-13T14:28:48Z

⌛ Testing commit b051df7 with merge 7b3feb7...

bors-servo · 2018-09-13T15:26:21Z

☀️ Test successful - android, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: cbrewster
Pushing 7b3feb7 to master...


        Auto merge of #21705 - mandreyel:coalesce-script-to-constellation-msg…

…s, r=paulrouget Create ScriptMsg::GetBrowsingContextInfo Script used to send two messages to constellation to first retrieve the id of the browsing context in which a pipeline resides and then its parent pipeline's id. This patch introduces a minor optimization to instead retrieve those fields in a single message. Also, fixed a potential bug introduced in #21559 where if a browsing context wasn't found for a pipeline in response to `GetParentInfo` (which is now deleted), constellation sent nothing back, presumably causing the script thread to hang on the receiving channel. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #21703 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because no new behaviour was introduced.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21705)

highfive assigned gw3583 Aug 31, 2018

highfive added the S-awaiting-review label Aug 31, 2018

mandreyel mentioned this pull request Aug 31, 2018

Frame should store some of the data that is currently in Pipeline #14692

Closed

highfive assigned cbrewster and unassigned gw3583 Aug 31, 2018

cbrewster requested changes Aug 31, 2018

View changes

cbrewster added S-needs-code-changes and removed S-awaiting-review labels Aug 31, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 1, 2018

mandreyel mentioned this pull request Sep 1, 2018

Track focused Frame instead of Pipeline #15507

Closed

mandreyel force-pushed the mandreyel:pipeline-fields-to-browsingcontext branch from 195f2ba to 2acc818 Sep 1, 2018

cbrewster requested changes Sep 1, 2018

View changes

cbrewster added S-needs-code-changes and removed S-awaiting-review labels Sep 1, 2018

cbrewster reviewed Sep 1, 2018

View changes

highfive added S-tests-failed and removed S-tests-failed S-needs-code-changes labels Sep 1, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Sep 13, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 13, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Sep 13, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 13, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Sep 13, 2018

highfive removed the S-awaiting-merge label Sep 13, 2018

mandreyel mentioned this pull request Sep 13, 2018

Create ScriptMsg::GetBrowsingContextInfo #21705

Merged

4 of 5 tasks complete

mandreyel mentioned this pull request Sep 14, 2018

Constellation::load_url private browsing mode propagation to new pipeline #21706

Closed

	fn remote_window_proxy(&self,
	global_to_clone: &GlobalScope,
	top_level_browsing_context_id: TopLevelBrowsingContextId,
	pipeline_id: PipelineId,
	opener: Option<BrowsingContextId>)
	-> Option<DomRoot<WindowProxy>>
	{
	let browsing_context_id = self.ask_constellation_for_browsing_context_id(pipeline_id)?;
	if let Some(window_proxy) = self.window_proxies.borrow().get(&browsing_context_id) {
	return Some(DomRoot::from_ref(window_proxy));
	}
	let parent = self.ask_constellation_for_parent_info(pipeline_id).and_then(\|parent_id\| {
	self.remote_window_proxy(global_to_clone, top_level_browsing_context_id, parent_id, opener)
	});
	let window_proxy = WindowProxy::new_dissimilar_origin(global_to_clone,
	browsing_context_id,
	top_level_browsing_context_id,
	parent.r(),
	opener);
	self.window_proxies.borrow_mut().insert(browsing_context_id, Dom::from_ref(&*window_proxy));
	Some(window_proxy)
	}

servo / servo

Join GitHub today

Frame should store some of the data that is currently in Pipeline #14692 #21559

Frame should store some of the data that is currently in Pipeline #14692 #21559

Conversation

mandreyel commented Aug 31, 2018 • edited

highfive commented Aug 31, 2018

highfive commented Aug 31, 2018

jdm commented Aug 31, 2018

cbrewster 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.

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.

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.

cbrewster commented Aug 31, 2018

mandreyel commented Aug 31, 2018

mandreyel commented Sep 1, 2018 • edited

mandreyel commented Sep 1, 2018 • edited

cbrewster 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.

cbrewster 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.

cbrewster commented Sep 1, 2018

bors-servo commented Sep 1, 2018

bors-servo commented Sep 1, 2018

bors-servo commented Sep 13, 2018

bors-servo commented Sep 13, 2018

jdm commented Sep 13, 2018

mandreyel commented Sep 13, 2018 • edited

mandreyel commented Sep 13, 2018

jdm commented Sep 13, 2018

bors-servo commented Sep 13, 2018

nox commented Sep 13, 2018

bors-servo commented Sep 13, 2018

jdm commented Sep 13, 2018

bors-servo commented Sep 13, 2018

bors-servo commented Sep 13, 2018

mandreyel commented Aug 31, 2018 •

edited

mandreyel commented Sep 1, 2018 •

edited

mandreyel commented Sep 1, 2018 •

edited

mandreyel commented Sep 13, 2018 •

edited