Remove CompositorEventListener trait #11443

kyleheadley · 2016-05-26T16:49:33Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #11339 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because refactoring

This change is

highfive · 2016-05-26T16:49:39Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @larsbergstrom (or someone else) soon.

asajeffrey · 2016-05-26T18:19:37Z

Hi Kyle. Now you get to see reviewable in action, lucky you.

asajeffrey · 2016-05-26T18:35:08Z

Reviewed 5 of 5 files at r1.
Review status: 3 of 5 files reviewed at latest revision, 4 unresolved discussions.

components/compositing/compositor_thread.rs, line 8 [r1] (raw file):

use SendableFrameTree;
use compositor::{self, CompositingReason};

Add IOCompositor to the use list, so it can be used below?

components/compositing/compositor_thread.rs, line 289 [r1] (raw file):

    pub fn create<Window>(window: Rc<Window>,
                          state: InitialCompositorState)
                          -> Box<compositor::IOCompositor<Window>>

If we're returning ownership of the IOCompositor<Window>, can we get rid of the Box? I think it was only needed because CompoositorEventListener was a trait.

components/servo/lib.rs, line 100 [r1] (raw file):

/// loop to pump messages between the embedding application and
/// various browser components.
pub struct Browser<Window: WindowMethods + 'static> {

Ah, that's interesting, Browser is now parametric in Window.

components/servo/lib.rs, line 101 [r1] (raw file):

/// various browser components.
pub struct Browser<Window: WindowMethods + 'static> {
    compositor: Box<IOCompositor<Window>>,

Do we need this Box?

Comments from Reviewable

asajeffrey · 2016-05-26T18:36:43Z

Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

highfive · 2016-05-26T19:10:36Z

New code was committed to pull request.

kyleheadley · 2016-05-26T19:16:54Z

Thanks for the quick review! Do I comment here?

I adjusted the use case above. The boxes don't have to be there, but E-Easy Issue #10261 deals with that, so I left it for another contributor.

The modification in main.rs and lib.rs was scary, but I think it conforms better to the comments in those files, linking the Browser and WindowMethods.

asajeffrey · 2016-05-26T20:47:49Z

You reply back on reviewable. I think it's worth removing the box while you're there, as it really only makes sense for trait objects.

highfive · 2016-05-26T21:23:28Z

New code was committed to pull request.

asajeffrey · 2016-05-26T22:38:43Z

Looks good, can you squash the commits now?

Previously, highfive wrote…

New code was committed to pull request.

Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

highfive · 2016-05-26T22:54:44Z

New code was committed to pull request.

asajeffrey · 2016-05-26T23:39:45Z

@bors-servo: r+

bors-servo · 2016-05-26T23:39:45Z

📌 Commit b0d20d8 has been approved by asajeffrey

asajeffrey · 2016-05-26T23:40:17Z

They're you go, that's the process!

bors-servo · 2016-05-27T00:59:17Z

⌛ Testing commit b0d20d8 with merge c9dca59...


        Auto merge of #11443 - kyleheadley:remove_trait_11339, r=asajeffrey

Remove CompositorEventListener trait  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11339 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11443)

bors-servo · 2016-05-27T01:07:41Z

💔 Test failed - linux-dev

Ms2ger · 2016-05-27T08:25:01Z

./mach build-cef is failing.

jdm · 2016-05-27T11:59:59Z

browser.rs:30:14: 30:21 error: wrong number of type arguments: expected 1, found 0 [E0243]
browser.rs:30     OnScreen(Browser),
                           ^~~~~~~
browser.rs:30:14: 30:21 help: run `rustc --explain E0243` to see a detailed explanation
browser.rs:31:15: 31:22 error: wrong number of type arguments: expected 1, found 0 [E0243]
browser.rs:31     OffScreen(Browser),
                            ^~~~~~~

asajeffrey · 2016-05-27T13:34:55Z

@kyleheadley oops, ports/cef needs updated.

asajeffrey · 2016-05-27T18:28:48Z

I think the fix is to replace Browser by Browser<glutin_app::window::Window>.

kyleheadley · 2016-05-27T18:36:42Z

yes, and the other Browser by Browser<window::Window>. I've got it working but was about to try to rebase off the current master because it's been a few days.

Is there another build I should try? Or leave the others to test on the proper system?

highfive · 2016-05-27T18:40:31Z

New code was committed to pull request.

jdm · 2016-05-27T18:41:18Z

You'll need to change ports/gonk/src/main.rs as well, since it refers to the Browser type. It's harder to build locally, so those changes will probably need to be validated by the builders.

highfive · 2016-05-27T18:45:10Z

New code was committed to pull request.

asajeffrey · 2016-05-27T18:48:02Z

OK, let's see what happens now. @bors-servo r+

bors-servo · 2016-05-27T18:48:03Z

📌 Commit 021fa97 has been approved by asajeffrey

bors-servo · 2016-05-27T19:04:57Z

⌛ Testing commit 021fa97 with merge 8982568...


        Auto merge of #11443 - kyleheadley:remove_trait_11339, r=asajeffrey

Remove CompositorEventListener trait  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11339 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11443)

bors-servo · 2016-05-27T19:58:58Z

💔 Test failed - mac-rel-wpt

highfive · 2016-05-27T19:59:01Z

  ▶ Unexpected subtest result in /cssom-view/elementScroll.html:
  │ FAIL [expected PASS] Element scrollTop/Left getter/setter test
  │   → assert_equals: changed scrollTop should be 40 expected 30 but got 0
  │ 
  │ window.onload/&lt;@http://web-platform.test:8000/cssom-view/elementScroll.html:12:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  └ window.onload@http://web-platform.test:8000/cssom-view/elementScroll.html:5:9

  ▶ Unexpected subtest result in /cssom-view/elementScroll.html:
  │ FAIL [expected PASS] Element scrollTo test
  │   → assert_equals: changed scrollLeft should be 70 expected 80 but got 0
  │ 
  │ window.onload/&lt;@http://web-platform.test:8000/cssom-view/elementScroll.html:26:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  └ window.onload@http://web-platform.test:8000/cssom-view/elementScroll.html:23:9

  ▶ Unexpected subtest result in /cssom-view/elementScroll.html:
  │ FAIL [expected PASS] cssom-view - elementScroll
  │   → assert_equals: increment of scrollLeft should be 10 expected 10 but got 0
  │ 
  │ window.onload/&lt;@http://web-platform.test:8000/cssom-view/elementScroll.html:36:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  └ window.onload@http://web-platform.test:8000/cssom-view/elementScroll.html:30:9

  ▶ Unexpected subtest result in /cssom-view/elementScroll.html:
  │ FAIL [expected PASS] Element scroll test
  │   → assert_equals: changed scrollLeft should be 60 expected 50 but got 0
  │ 
  │ window.onload/&lt;@http://web-platform.test:8000/cssom-view/elementScroll.html:19:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  └ window.onload@http://web-platform.test:8000/cssom-view/elementScroll.html:16:9

highfive · 2016-05-27T20:52:14Z

New code was committed to pull request.

bors-servo · 2016-05-28T01:25:27Z

☔ The latest upstream changes (presumably #11474) made this pull request unmergeable. Please resolve the merge conflicts.

asajeffrey · 2016-05-31T16:13:47Z

@kyleheadley The appveyor build failure is spurious, try rebasing against the upstream master, and see if that fixes things.


        Removed CompositorEventListener trait


        update cef build with proper Window params

asajeffrey · 2016-06-01T17:51:12Z

Hmm, more spurious appveyor failures. @bors-servo r+

bors-servo · 2016-06-01T17:51:14Z

📌 Commit f6682c2 has been approved by asajeffrey

bors-servo · 2016-06-01T19:06:42Z

⌛ Testing commit f6682c2 with merge c641ec9...


        Auto merge of #11443 - kyleheadley:remove_trait_11339, r=asajeffrey

Remove CompositorEventListener trait  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11339 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11443)

bors-servo · 2016-06-01T19:56:02Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

asajeffrey · 2016-06-01T20:45:09Z

Yay!

highfive assigned larsbergstrom May 26, 2016

asajeffrey assigned asajeffrey and unassigned larsbergstrom May 26, 2016

kyleheadley force-pushed the kyleheadley:remove_trait_11339 branch from 1997cb2 to b0d20d8 May 26, 2016

kyleheadley force-pushed the kyleheadley:remove_trait_11339 branch from 021fa97 to a8155cc May 27, 2016

jdm removed the S-awaiting-review label May 31, 2016

kyleheadley added 2 commits May 26, 2016

Removed CompositorEventListener trait

2bb109b

update cef build with proper Window params

Loading status checks…

f6682c2

kyleheadley force-pushed the kyleheadley:remove_trait_11339 branch from a8155cc to f6682c2 May 31, 2016

highfive added the S-awaiting-review label May 31, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jun 1, 2016

bors-servo merged commit f6682c2 into servo:master Jun 1, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 1, 2016

cbrewster mentioned this pull request Jun 3, 2016

Remove CompositorThread struct? #10261

Closed

servo / servo

Remove CompositorEventListener trait #11443

Remove CompositorEventListener trait #11443

kyleheadley commented May 26, 2016 •

edited by larsbergstrom

highfive commented May 26, 2016

asajeffrey commented May 26, 2016

asajeffrey commented May 26, 2016

asajeffrey commented May 26, 2016

highfive commented May 26, 2016

kyleheadley commented May 26, 2016 •

edited

asajeffrey commented May 26, 2016

highfive commented May 26, 2016

asajeffrey commented May 26, 2016

highfive commented May 26, 2016

asajeffrey commented May 26, 2016

bors-servo commented May 26, 2016

asajeffrey commented May 26, 2016

bors-servo commented May 27, 2016

bors-servo commented May 27, 2016

Ms2ger commented May 27, 2016

jdm commented May 27, 2016

asajeffrey commented May 27, 2016

asajeffrey commented May 27, 2016

kyleheadley commented May 27, 2016

highfive commented May 27, 2016

jdm commented May 27, 2016

highfive commented May 27, 2016

asajeffrey commented May 27, 2016

bors-servo commented May 27, 2016

bors-servo commented May 27, 2016

bors-servo commented May 27, 2016

highfive commented May 27, 2016

highfive commented May 27, 2016

bors-servo commented May 28, 2016

asajeffrey commented May 31, 2016

asajeffrey commented Jun 1, 2016

bors-servo commented Jun 1, 2016

bors-servo commented Jun 1, 2016

bors-servo commented Jun 1, 2016

asajeffrey commented Jun 1, 2016

servo / servo

Join GitHub today

Remove CompositorEventListener trait #11443

Remove CompositorEventListener trait #11443

Conversation

kyleheadley commented May 26, 2016 • edited by larsbergstrom

highfive commented May 26, 2016

asajeffrey commented May 26, 2016

asajeffrey commented May 26, 2016

asajeffrey commented May 26, 2016

highfive commented May 26, 2016

kyleheadley commented May 26, 2016 • edited

asajeffrey commented May 26, 2016

highfive commented May 26, 2016

asajeffrey commented May 26, 2016

highfive commented May 26, 2016

asajeffrey commented May 26, 2016

bors-servo commented May 26, 2016

asajeffrey commented May 26, 2016

bors-servo commented May 27, 2016

bors-servo commented May 27, 2016

Ms2ger commented May 27, 2016

jdm commented May 27, 2016

asajeffrey commented May 27, 2016

asajeffrey commented May 27, 2016

kyleheadley commented May 27, 2016

highfive commented May 27, 2016

jdm commented May 27, 2016

highfive commented May 27, 2016

asajeffrey commented May 27, 2016

bors-servo commented May 27, 2016

bors-servo commented May 27, 2016

bors-servo commented May 27, 2016

highfive commented May 27, 2016

highfive commented May 27, 2016

bors-servo commented May 28, 2016

asajeffrey commented May 31, 2016

asajeffrey commented Jun 1, 2016

bors-servo commented Jun 1, 2016

bors-servo commented Jun 1, 2016

bors-servo commented Jun 1, 2016

asajeffrey commented Jun 1, 2016

kyleheadley commented May 26, 2016 •

edited by larsbergstrom

kyleheadley commented May 26, 2016 •

edited