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

Separate waking the event loop, from communicating with Compositor #17068

Merged
merged 1 commit into from Jun 7, 2017

Conversation

Projects
None yet
6 participants
@gterzian
Copy link
Collaborator

commented May 28, 2017

@paulrouget first step of #15934, Glutin only for now, please take a look...

If this makes sense, I will also update the code for ports other than Glutin...

One question: one do I add the warn! macro to servolib? Also perhaps we would also want to add box, since I had to switch to using Box::new...


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@gterzian gterzian changed the title Separate waking the event loop, from CompositorProxy [WIP] Separate waking the event loop, from CompositorProxy May 28, 2017

@gterzian gterzian changed the title [WIP] Separate waking the event loop, from CompositorProxy [WIP] Separate waking the event loop, from communicating with Compositor May 28, 2017

@gterzian gterzian force-pushed the gterzian:remove_compositorproxy branch 6 times, most recently from 33227ca to ab7a4d0 May 28, 2017

@jdm

This comment has been minimized.

Copy link
Member

commented May 28, 2017

@highfive highfive assigned paulrouget and unassigned jdm May 28, 2017

@gterzian gterzian force-pushed the gterzian:remove_compositorproxy branch 2 times, most recently from 01170ae to 633ca33 May 30, 2017

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented May 30, 2017

UPDATE: also added the implementation for the cef port...

@paulrouget
Copy link
Contributor

left a comment

Looks good to me. There are some nits to address. Also, I'm not 100% sure you need to use Box.

Once these addressed, I'd like @asajeffrey to take a look as well.

pub trait CompositorReceiver : 'static {
/// Receives the next message inbound for the compositor. This must not block.
fn try_recv_compositor_msg(&mut self) -> Option<Msg>;
/// Synchronously waits for, and returns, the next message inbound for the compositor.
fn recv_compositor_msg(&mut self) -> Msg;
}

/// Used to wake up the event loop, provided by the Servo port.

This comment has been minimized.

Copy link
@paulrouget

paulrouget May 30, 2017

Contributor

s/Servo port/embedder/

/// This is part of the windowing system because its implementation often involves OS-specific
/// magic to wake the up window's event loop.
fn create_compositor_channel(&self) -> (Box<CompositorProxy + Send>, Box<CompositorReceiver>);
/// Creates a thread-safe object to wake the up window's event loop.

This comment has been minimized.

Copy link
@paulrouget

paulrouget May 30, 2017

Contributor

Typo in comment.

@@ -273,6 +273,36 @@ impl<Window> Browser<Window> where Window: WindowMethods + 'static {
}
}

fn create_compositor_channel(event_loop_riser: Box<compositor_thread::EventLoopRiser>)
-> (Box<CompositorProxy + Send>, Box<CompositorReceiver>) {

This comment has been minimized.

Copy link
@paulrouget

paulrouget May 30, 2017

Contributor

(Box<CompositorProxy + Send>, Box<CompositorReceiver>): do we still need boxed objects? Now that the implementation is not in the embedder anymore, we don't need a trait, we could use a regular struct (like BrowserCompositorProxy). It would then have a known size, and I don't think we would then need to box these?

I didn't look too closely, so maybe I'm wrong.

// Send a message and kick the OS event loop awake.
if let Err(err) = self.sender.send(msg) {
// NOTE: how to add the warn! macro?
//warn!("Failed to send response ({}).", err);

This comment has been minimized.

Copy link
@paulrouget

paulrouget May 30, 2017

Contributor

Add #[macro_use] on top of extern crate log;

} as Box<CompositorProxy+Send>,
box receiver as Box<CompositorReceiver>)
fn create_event_loop_riser(&self) -> Box<EventLoopRiser> {
struct CefEventLoopRiser {}

This comment has been minimized.

Copy link
@paulrouget

paulrouget May 30, 2017

Contributor

You can just use struct CefEventLoopRiser;.

box receiver as Box<CompositorReceiver>)
box GlutinEventLoopRiser {
window_proxy: window_proxy,
} as Box<EventLoopRiser + Send>

This comment has been minimized.

Copy link
@paulrouget

paulrouget May 30, 2017

Contributor

Is the as … necessary?

fn clone(&self) -> Box<EventLoopRiser + Send> {
box GlutinEventLoopRiser {
window_proxy: self.window_proxy.clone(),
} as Box<EventLoopRiser + Send>

This comment has been minimized.

Copy link
@paulrouget

paulrouget May 30, 2017

Contributor

Is the as … necessary?

@gterzian gterzian force-pushed the gterzian:remove_compositorproxy branch from 633ca33 to f630c34 Jun 3, 2017

@gterzian gterzian force-pushed the gterzian:remove_compositorproxy branch 3 times, most recently from 03b225d to bab8f3a Jun 4, 2017

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 4, 2017

@paulrouget Ok, I replaced the compositor related traits with structs, and addressed the other comments too...

I'm wondering if we need to add some sort of "clone" method to CompositorReceiver?

} as Box<CompositorProxy+Send>,
box receiver as Box<CompositorReceiver>)
fn create_event_loop_riser(&self) -> Box<EventLoopRiser> {
struct CefEventLoopRiser

This comment has been minimized.

Copy link
@paulrouget

paulrouget Jun 5, 2017

Contributor

Add a ; at the end after CefEventLoopRiser.

}
fn clone(&self) -> Box<EventLoopRiser + Send> {
box CefEventLoopRiser {
} as Box<EventLoopRiser + Send>

This comment has been minimized.

Copy link
@paulrouget

paulrouget Jun 5, 2017

Contributor

I think you can just do:

fn clone() {
  box CefEventLoopRiser
}
@paulrouget

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2017

Thank you Greg. This looks good to me now (beside a nit).

I'd like to get a review from Alan.

@asajeffrey - this is the first step to clarify the Embedder <-> Constellation communication. Can you take a look too?

@asajeffrey
Copy link
Member

left a comment

LGTM, I just have an annoying bikeshedding name change. EventLoopWakeup?

fn clone_compositor_proxy(&self) -> Box<CompositorProxy + 'static + Send>;

/// Used to wake up the event loop, provided by the servo port/embedder.
pub trait EventLoopRiser : 'static + Send {

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Jun 5, 2017

Member

Nit: not sure about the name. A riser is a part of a staircase :)

This comment has been minimized.

Copy link
@gterzian

gterzian Jun 5, 2017

Author Collaborator

How about EventLoopAwaker, so that we have a noun? https://en.wiktionary.org/wiki/awaker

@paulrouget

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2017

ajeffrey> "Waker" would probably be more colloquial English: "Awaker"
sounds like "I am more awake than you".

@gterzian gterzian force-pushed the gterzian:remove_compositorproxy branch from 715e910 to 3a693c7 Jun 6, 2017

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 6, 2017

Ok, EventLoopWaker it is...

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 7, 2017

It looks like approving in Github isn't enough, and somebody needs to do a r+ at the bot...

@asajeffrey

This comment has been minimized.

Copy link
Member

commented Jun 7, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2017

📋 Looks like this PR is still in progress, ignoring approval

@highfive highfive assigned asajeffrey and unassigned paulrouget Jun 7, 2017

@asajeffrey

This comment has been minimized.

Copy link
Member

commented Jun 7, 2017

Hmm, I wonder why bors reckons this is still a WIP. Is it the unchecked boxes in the PR description?

@paulrouget paulrouget changed the title [WIP] Separate waking the event loop, from communicating with Compositor Separate waking the event loop, from communicating with Compositor Jun 7, 2017

@paulrouget

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2017

Probably the title. I changed it. Do you need to re-approve?

@asajeffrey

This comment has been minimized.

Copy link
Member

commented Jun 7, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2017

📌 Commit 3a693c7 has been approved by asajeffrey

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 7, 2017

@paulrouget @asajeffrey thanks for the review.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2017

⌛️ Testing commit 3a693c7 with merge 7e273d6...

bors-servo added a commit that referenced this pull request Jun 7, 2017

Auto merge of #17068 - gterzian:remove_compositorproxy, r=asajeffrey
Separate waking the event loop, from communicating with Compositor

<!-- Please describe your changes on the following line: -->

@paulrouget first step of #15934, Glutin only for now, please take a look...

If this makes sense, I will also update the code for ports other than Glutin...

One question: one do I add the `warn!` macro to `servolib`? Also perhaps we would also want to add `box`, since I had to switch to using `Box::new`...

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/17068)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2017

@bors-servo bors-servo merged commit 3a693c7 into servo:master Jun 7, 2017

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
@paulrouget

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2017

Thank you @gterzian

@gterzian gterzian deleted the gterzian:remove_compositorproxy branch Jun 7, 2017

@asajeffrey

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

@gterzian thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.