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

Attach PipelineId to all ScriptMsg #17201

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

Attach PipelineId to all ScriptMsg #17201

paulrouget opened this issue Jun 7, 2017 · 4 comments
Assignees

Comments

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Jun 7, 2017

Many ScriptMsg already require a PipelineId field.

Many of these messages are sent to the constellation to be then sent to the embedder via the compositor (status, favicon, loadcomplete, loadstate, etc). And as of now, the decision to send the message to the embedder (which usually is either the pipeline the top level one or not) is done in on of these 3 places: in /script/, in compositor.rs, or in constellation.rs.

I think the logic of forwarding the event or not should all happen in constellation.rs. The constellation has more context than /script/, and we eventually want to get rid of the compositor step (#15934).

For the constellation to make the correct decision of forwarding the event, it needs the PipelineId.

I'm looking for a way to easily attach the PipelineId to all the ScriptMsg.

I was thinking about change the ScriptMsg type to:

struct ScriptMsg {
  source: PipelineId,
  message: ScriptMsgContent,
}

enum ScriptMsgContent {
  SetPageTitle,
  Focus,
  …
}

… but it's kind of cumbersome. And we don't always have an easy access to the pipeline_id. Would there be a better way to do that?

@paulrouget paulrouget self-assigned this Jun 7, 2017
@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jun 7, 2017

I want also thinking about creating a struct CompositorChannel:

struct CompositorChannel {
  sender: IpcSender<ScriptMsg>,
  pipeline_id: PipelineId,
}

impl CompositorChannel {
  fn send(msg) {
    self.sender.send((self.pipeline_id, msg)),
  }
  fn clone_with_new_pipeline(pipeline_id) -> CompositorChannel {
    CompositorChannel {
      pipeline_id: pipeline_id,
      send: self.sender.clone(),
    }
  } 
}

… and instead of calling constellation_chan.clone() we would use constellation_chan.clone_with_new_pipeline(pipeline_id).

@jdm jdm added the A-embedding label Jun 7, 2017
@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jun 7, 2017

Do you mean:

   struct CompositorChannel {
      sender: Sender<(PipelineId, ScriptMsg)>,
      pipeline_id: PipelineId,
   }

If so, apart from the naming, this seems sensible. I used a similar trick in worklets:

/// An executor of worklet tasks

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jun 8, 2017

oops, yes, it's what I meant.

Thanks.

bors-servo added a commit that referenced this issue Aug 12, 2017
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
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
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.