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

asajeffrey · 2016-12-23T15:35:45Z

There is some data we are currently storing in Pipeline that should really be in Frame: parent_info, size, is_private and visible.

nox · 2017-10-07T10:59:21Z

If you wrote some more details, this issue could probably be E-Easy or E-Mentored.

asajeffrey · 2017-10-09T16:04:46Z

Not sure about E-easy, this refactoring touches a lot of code. Might be doable as E-Mentored. cc @cbrewster

mandreyel · 2018-08-15T20:26:52Z

I tried to look for Frame but didn't find it. Has it been renamed, removed, or did I just miss it? In case Frame no longer exists, is this issue still relevant (the mentioned fields are still in Pipeline)?

jdm · 2018-08-15T21:30:01Z

According to 607e011#diff-7397386a198b1fdaf83ca2eb01eab35b Frame was renamed to BrowsingContext. Presumably this is still relevant!

mandreyel · 2018-08-16T05:03:12Z

Ah, that makes sense, I recall seeing a few TODOs sprinkled in Pipeline to move some fields to BrowsingContext.

In that case I'd like to try my hand at this! I can probably start working on it Friday!

jdm · 2018-08-16T05:34:54Z

Please do!

mandreyel · 2018-08-20T10:58:15Z

Okay, so I had some time to work on this and I'm mostly done, but there is one pesky bit that I cannot seem to figure out on which I need your input.

After creating an iframe and loading a document in it for the first time, Constellation::change_session_history is eventually called, which in this case needs to set up a new BrowsingContext for the iframe as it does not have one yet, and insert this new browsing context into its containing parent Pipeline's children: Vec<BrowsingContext> field.

Previously it was solved like this, but now that BrowsingContext instead of Pipeline keeps track of its containing parent Pipeline, a conundrum arises in that we cannot add this new browsing context to its would-be parent pipeline, since the context has just been created and thus its (optional) field for keeping track of its parent pipeline has not yet been set, nor does its parent pipeline's children field contain this browsing context (and thus we cannot find the pertaining pipeline starting from iframe's top level browsing context). So I'm not sure what the best way would be to "attach" a new iframe's browsing context to its parent pipeline.

I see one possible solution: Constellation::handle_script_new_iframe is invoked when script notifies constellation of a new iframe with an IFrameLoadInfo object, which does contain the ID of its parent pipeline. So the new iframe's browsing_context_id could simply be inserted in its parent pipeline's children field in this function?
However, I'm not sure if this were to break any behaviour since this change would mean that the parent pipeline's children field would contain a browsing context whose document hasn't been loaded yet, while previously it was only inserted once the document had become active.

What do you think? Do you see another solution?

Please note, though, that I may just have misunderstood some (or many) things since I'm not yet fully familiar with Servo's code :)

mandreyel · 2018-08-20T15:14:34Z

Another option would be to store a parent_pipeline_id: Option<PipelineId> field in SessionHistoryChange, so that when the iframe's document is done loading and becomes active we can access the parent pipeline and attach iframe's browsing context to it. I think either could work, though perhaps the previously proposed solution is a bit cleaner--unless that won't work due to the described slight change in behaviour.

cbrewster · 2018-08-20T17:46:13Z

The second approach (adding a new field to SessionHistoryChange) sounds like the right way to go about this. The first approach would end up being a behavior change which may cause some test failures (I believe I tried a change similar to that a while back with that result).

mandreyel · 2018-08-20T20:24:50Z

@cbrewster Thanks for chiming in! Having thought about it some more, I would now go with the second approach as well.

In fact, I had initially overlooked that the same difficulty exists when passing is_private to a new BrowsingContext and probably when passing visible--though I don't fully understand the role of the latter yet, so I'm not sure if this could be set at a later point. [edit] Are new browsing contexts visible by default? Or how does this work? Knowing this could simplify the code a bit.

I suppose I could just stuff all fields in SesssionHistoryChange for now, and see if there is a better solution.

mandreyel · 2018-08-31T19:21:21Z

Landed PR #21559.


        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)


        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)


        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)


        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)


        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)


        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)


        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)


        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)

asajeffrey added A-constellation I-refactor labels Dec 23, 2016

asajeffrey self-assigned this Dec 23, 2016

asajeffrey mentioned this issue Dec 23, 2016

Constellation documentation comments #14698

Merged

3 of 3 tasks complete

asajeffrey mentioned this issue Jan 4, 2017

Implement discarding Document objects to reclaim space. #14312

Merged

4 of 4 tasks complete

asajeffrey mentioned this issue Feb 16, 2017

Track focused Frame instead of Pipeline #15507

Closed

asajeffrey removed their assignment Oct 9, 2017

jdm added the C-assigned label Aug 16, 2018

mandreyel mentioned this issue Aug 31, 2018

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

Merged

4 of 4 tasks complete

bors-servo closed this in #21559 Sep 13, 2018

servo / servo

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

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

asajeffrey commented Dec 23, 2016

nox commented Oct 7, 2017

asajeffrey commented Oct 9, 2017

mandreyel commented Aug 15, 2018 •

edited

jdm commented Aug 15, 2018

mandreyel commented Aug 16, 2018

jdm commented Aug 16, 2018

mandreyel commented Aug 20, 2018 •

edited

mandreyel commented Aug 20, 2018

cbrewster commented Aug 20, 2018

mandreyel commented Aug 20, 2018 •

edited

mandreyel commented Aug 31, 2018

servo / servo

Join GitHub today

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

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

Comments

asajeffrey commented Dec 23, 2016

nox commented Oct 7, 2017

asajeffrey commented Oct 9, 2017

mandreyel commented Aug 15, 2018 • edited

jdm commented Aug 15, 2018

mandreyel commented Aug 16, 2018

jdm commented Aug 16, 2018

mandreyel commented Aug 20, 2018 • edited

mandreyel commented Aug 20, 2018

cbrewster commented Aug 20, 2018

mandreyel commented Aug 20, 2018 • edited

mandreyel commented Aug 31, 2018

mandreyel commented Aug 15, 2018 •

edited

mandreyel commented Aug 20, 2018 •

edited

mandreyel commented Aug 20, 2018 •

edited