Separate waking the event loop, from communicating with Compositor by gterzian · Pull Request #17068 · servo/servo

gterzian · 2017-05-28T03:58:58Z

@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

jdm · 2017-05-28T13:22:23Z

r? @paulrouget

gterzian · 2017-05-30T01:55:22Z

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

paulrouget

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.

paulrouget · 2017-05-30T08:36:03Z

components/compositing/compositor_thread.rs

    fn recv_compositor_msg(&mut self) -> Msg;
 }

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


s/Servo port/embedder/

paulrouget · 2017-05-30T08:37:02Z

components/compositing/windowing.rs

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


Typo in comment.

paulrouget · 2017-05-30T08:50:15Z

components/servo/lib.rs

 }

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


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

paulrouget · 2017-05-30T08:52:10Z

components/servo/lib.rs

+            // 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);


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

paulrouget · 2017-05-30T08:54:33Z

ports/cef/window.rs

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


You can just use struct CefEventLoopRiser;.

paulrouget · 2017-05-30T09:06:04Z

ports/glutin/window.rs

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


Is the as … necessary?

paulrouget · 2017-05-30T09:10:30Z

ports/glutin/window.rs

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


Is the as … necessary?

gterzian · 2017-06-04T01:58:59Z

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

paulrouget · 2017-06-05T08:31:38Z

ports/cef/window.rs

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


Add a ; at the end after CefEventLoopRiser.

paulrouget · 2017-06-05T08:32:09Z

ports/cef/window.rs

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


I think you can just do:

fn clone() { box CefEventLoopRiser }

paulrouget · 2017-06-05T08:39:01Z

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

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

asajeffrey · 2017-06-05T14:10:13Z

components/compositing/compositor_thread.rs

-    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 {


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

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

paulrouget · 2017-06-06T03:39:28Z

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

gterzian · 2017-06-06T07:47:21Z

Ok, EventLoopWaker it is...

gterzian · 2017-06-07T00:53:27Z

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

asajeffrey · 2017-06-07T01:13:16Z

@bors-servo r+

bors-servo · 2017-06-07T01:13:17Z

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

asajeffrey · 2017-06-07T01:22:49Z

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

paulrouget · 2017-06-07T03:13:13Z

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

paulrouget · 2017-06-07T03:18:08Z

https://github.com/bundler/homu/blob/288bda90d87f6500f7dd9dd779b633f7615f72e4/homu/main.py#L352

asajeffrey · 2017-06-07T12:18:26Z

@bors-servo r+

bors-servo · 2017-06-07T12:18:26Z

📌 Commit 3a693c7 has been approved by asajeffrey

gterzian · 2017-06-07T12:24:10Z

@paulrouget @asajeffrey thanks for the review.

bors-servo · 2017-06-07T12:52:37Z

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

@paulrouget

Separate waking the event loop, from communicating with Compositor  @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 [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17068)

bors-servo · 2017-06-07T13:49:06Z

☀️ Test successful - android, 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: asajeffrey
Pushing 7e273d6 to master...

paulrouget · 2017-06-07T18:46:17Z

Thank you @gterzian

asajeffrey · 2017-06-08T04:01:01Z

@gterzian thanks!

highfive assigned jdm May 28, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 28, 2017

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

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 force-pushed the remove_compositorproxy branch 6 times, most recently from 33227ca to ab7a4d0 Compare May 28, 2017 04:46

highfive assigned paulrouget and unassigned jdm May 28, 2017

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

paulrouget reviewed May 30, 2017

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 30, 2017

gterzian force-pushed the remove_compositorproxy branch from 633ca33 to f630c34 Compare June 3, 2017 06:49

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jun 3, 2017

gterzian force-pushed the remove_compositorproxy branch 3 times, most recently from 03b225d to bab8f3a Compare June 4, 2017 01:55

paulrouget mentioned this pull request Jun 5, 2017

Create a dedicated channel for constellation -> embedder communication #15934

Closed

paulrouget suggested changes Jun 5, 2017

View reviewed changes

paulrouget approved these changes Jun 5, 2017

View reviewed changes

asajeffrey approved these changes Jun 5, 2017

View reviewed changes

separate waking the event loop, from communicating with a compositor

3a693c7

gterzian force-pushed the remove_compositorproxy branch from 715e910 to 3a693c7 Compare June 6, 2017 07:46

highfive assigned asajeffrey and unassigned paulrouget Jun 7, 2017

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

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 7, 2017

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

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jun 7, 2017

This was referenced Jun 7, 2017

Pass URL to Browser::new(), delegate url checking logic to 3rd party #16477

Merged

[wip] Introduce ViewGeometry #17091

Closed

gterzian deleted the remove_compositorproxy branch June 7, 2017 23:36

Uh oh!

Conversation

gterzian commented May 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdm commented May 28, 2017

Uh oh!

gterzian commented May 30, 2017

Uh oh!

paulrouget left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gterzian commented Jun 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulrouget commented Jun 5, 2017

Uh oh!

asajeffrey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulrouget commented Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gterzian commented Jun 6, 2017

Uh oh!

gterzian commented Jun 7, 2017

Uh oh!

asajeffrey commented Jun 7, 2017

Uh oh!

bors-servo commented Jun 7, 2017

Uh oh!

asajeffrey commented Jun 7, 2017

Uh oh!

paulrouget commented Jun 7, 2017

Uh oh!

paulrouget commented Jun 7, 2017

Uh oh!

asajeffrey commented Jun 7, 2017

Uh oh!

bors-servo commented Jun 7, 2017

Uh oh!

gterzian commented Jun 7, 2017

Uh oh!

bors-servo commented Jun 7, 2017

Uh oh!

bors-servo commented Jun 7, 2017

Uh oh!

paulrouget commented Jun 7, 2017

Uh oh!

asajeffrey commented Jun 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

gterzian commented May 28, 2017 •

edited

Loading

gterzian commented Jun 4, 2017 •

edited

Loading

paulrouget commented Jun 6, 2017 •

edited

Loading