Skip to content

/ servo

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

Jump to bottom

Remove pipeline non-compositing-logic code from compositor #17200

Closed
paulrouget opened this issue Jun 7, 2017 · 2 comments
Closed

Remove pipeline non-compositing-logic code from compositor #17200

paulrouget opened this issue Jun 7, 2017 · 2 comments
Assignees
@paulrouget
Labels
A-embedding A-gfx/compositing C-assigned

Comments

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Jun 7, 2017

Necessary for #15934.

From #15934 (comment):

In the compositor, all the pipeline work that is not related to compositing need to be striped down to its most simple form. The compositor code in these cases should just be used to forward messages to the constellation (we will eventually have a dedicated channel).

For example, the Reload message handler work that way today:

  • Embedder send WindowEvent::Reload to compositor.
  • Compositor find the pipeline to reload and its browsing context.
  • Compositor send ConstellationMsg::Reload(browsing_context) to constellation.

What we want is either:

  • Embedder send WindowEvent::Reload(browsing_context) to compositor.
  • Compositor forward to constellation.

… or:

  • Embedder send WindowEvent::Reload to compositor.
  • Compositor forward to constellation.
  • Constellation find browsing context.

Another example, the ChangePageTitle event that comes from the script thread. This is how it works today:

  • document.rs send SetTitle to constellation
  • constellation send ChangePageTitle to compositor
  • compositor checks if the source pipeline is root pipeline, if so, it sends it to embedder

What we want is:

  • document.rs send SetTitle to constellation
  • constellation checks if the source pipeline is root pipeline, if so, it sends to compositor
  • compositor forwards to embedder
@paulrouget paulrouget self-assigned this Jun 7, 2017
@paulrouget paulrouget mentioned this issue Jun 7, 2017
Closed
@paulrouget paulrouget mentioned this issue Jun 8, 2017
Closed
@gterzian
Copy link
Member

@gterzian gterzian commented Jun 13, 2017

Perhaps in the light of #15934, it is not necessary for the compositor to forward messages from the embedder, rather the embedder(actually it's not the embedder, rather 'Browser' in libservor) could send a message to the constellation, currently grabbing the pipeline id from the compositor(see https://github.com/gterzian/servo/blob/887289e2628f714de8c5787a0c72c527957502ba/components/servo/lib.rs#L253)
@paulrouget paulrouget mentioned this issue Jun 20, 2017
Merged
bors-servo added a commit that referenced this issue Aug 12, 2017
@bors-servo
Auto merge of #17425 - paulrouget:attach-pipeline-2, r=asajeffrey
Loading status checks…
76700a3 
cleanup embedder/compositor/constellation/script messages

Fix: #17226 #17200 #17201

This is work in progress. Some tests still fail.
I'd like to get early feedback as it's a pretty large PR.

There is nothing fundamentally new. Basically, I added TopLevelBrowsingContrextId to the relevant messages between the embedder, the compositor and the constellation, and enforced the PipelineId to be attached to each ScriptMsg (see #17201).

I unaliased all the ScriptMsg. It was getting difficult to understand the nature of the message as ScriptMsg was used aliased CompositorMsg sometimes (CompositorMsg is an actually type of message already). I renamed constellation_chan to script_to_constellation_chan, again, for clarification.

This cleanup code is necessary for #15934 and for tabs support.

/cc @asajeffrey can I ask you to look at this? No need for a formal review, I need feedback at this stage.

<!-- 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/17425)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Aug 15, 2017
@bors-servo
Auto merge of #17425 - paulrouget:attach-pipeline-2, r=asajeffrey
Loading status checks…
7455899 
cleanup embedder/compositor/constellation/script messages

Fix: #17226 #17200 #17201

This is work in progress. Some tests still fail.
I'd like to get early feedback as it's a pretty large PR.

There is nothing fundamentally new. Basically, I added TopLevelBrowsingContrextId to the relevant messages between the embedder, the compositor and the constellation, and enforced the PipelineId to be attached to each ScriptMsg (see #17201).

I unaliased all the ScriptMsg. It was getting difficult to understand the nature of the message as ScriptMsg was used aliased CompositorMsg sometimes (CompositorMsg is an actually type of message already). I renamed constellation_chan to script_to_constellation_chan, again, for clarification.

This cleanup code is necessary for #15934 and for tabs support.

/cc @asajeffrey can I ask you to look at this? No need for a formal review, I need feedback at this stage.

<!-- 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/17425)
<!-- Reviewable:end -->
@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Aug 15, 2017

Fixed by #17425
@paulrouget paulrouget closed this Aug 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@paulrouget paulrouget

Labels
A-embedding A-gfx/compositing C-assigned
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
@jdm @paulrouget @gterzian
You can’t perform that action at this time.