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

Switch to using WebRender hit testing #18704

Merged
merged 1 commit into from Oct 17, 2017
Merged

Conversation

@mrobinson
Copy link
Member

mrobinson commented Oct 2, 2017

This trades quite a bit of complicated code in Servo for few more
messages and a significant performance improvement. In particular,
WebRender can search the entire display list at once instead of
ping-ponging down the pipeline tree. This allows us to send mouse
events to the correct pipeline immediately.


  • ./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 they should not change behavior.

This change is Reviewable

@highfive
Copy link

highfive commented Oct 2, 2017

Heads up! This PR modifies the following files:

  • @jgraham: tests/wpt/grouping_formatter.py
  • @emilio: components/layout/webrender_helpers.rs, components/layout/query.rs
  • @fitzgen: components/script/dom/document.rs, components/script/dom/window.rs, components/script/script_thread.rs, components/script_layout_interface/message.rs, components/script_traits/lib.rs and 2 more
  • @KiChjang: components/script/dom/document.rs, components/script/dom/window.rs, components/script/script_thread.rs, components/script_layout_interface/message.rs, components/script_traits/lib.rs and 2 more
  • @asajeffrey: components/constellation/constellation.rs
  • @paulrouget: components/compositing/compositor.rs, components/constellation/constellation.rs, components/compositing/compositor_thread.rs
@highfive
Copy link

highfive commented Oct 2, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@mrobinson
Copy link
Member Author

mrobinson commented Oct 2, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2017

Trying commit 52d356e with merge 64a4ea8...

bors-servo added a commit that referenced this pull request Oct 2, 2017
Switch to using WebRender hit testing

This trades quite a bit of complicated code in Servo for few more
messages and a significant performance improvement. In particular,
WebRender can search the entire display list at once instead of
ping-ponging down the pipeline tree. This allows us to send mouse
events to the correct pipeline immediately.

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

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

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because they should not change behavior.

<!-- 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. -->
@mrobinson
Copy link
Member Author

mrobinson commented Oct 2, 2017

This PR isn't quite ready to review, as I need to update the code style.

@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2017

💔 Test failed - linux-dev

@mrobinson mrobinson force-pushed the mrobinson:wr-hit-testing branch from 52d356e to 227be2a Oct 2, 2017
@highfive highfive removed the S-tests-failed label Oct 2, 2017
@mrobinson
Copy link
Member Author

mrobinson commented Oct 2, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2017

Trying commit 227be2a with merge e603814...

bors-servo added a commit that referenced this pull request Oct 2, 2017
Switch to using WebRender hit testing

This trades quite a bit of complicated code in Servo for few more
messages and a significant performance improvement. In particular,
WebRender can search the entire display list at once instead of
ping-ponging down the pipeline tree. This allows us to send mouse
events to the correct pipeline immediately.

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

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

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because they should not change behavior.

<!-- 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/18704)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2017

💔 Test failed - mac-rel-wpt4

@jdm
Copy link
Member

jdm commented Oct 2, 2017

  ▶ Unexpected subtest result in /_mozilla/mozilla/scrollBy.html:
  │ FAIL [expected PASS] Ensure that the window.scrollBy function affects scroll position as expected
  │   → assert_equals: expected Element node <body>
    <a id="link" href="http://mozilla.org">This is... but got Element node <a id="link" href="http://mozilla.org">This is some link ...
  │ 
  │ @http://web-platform.test:8000/_mozilla/mozilla/scrollBy.html:23:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  │ test@http://web-platform.test:8000/resources/testharness.js:497:9
  └ @http://web-platform.test:8000/_mozilla/mozilla/scrollBy.html:10:5
@mrobinson mrobinson force-pushed the mrobinson:wr-hit-testing branch from 227be2a to 83a9603 Oct 2, 2017
@highfive highfive removed the S-tests-failed label Oct 2, 2017
@mrobinson
Copy link
Member Author

mrobinson commented Oct 2, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2017

Trying commit 83a9603 with merge 33f85dd8d8ce4b499eb9fd28bd10b9422e018c33...

@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2017

💔 Test failed - linux-rel-css

@mrobinson
Copy link
Member Author

mrobinson commented Oct 2, 2017

  ▶ CRASH [expected OK] /css-transitions-1_dev/html/before-DOMContentLoaded-001.htm
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 17.2.0-devel
  │ called `Option::unwrap()` on a `None` value (thread LayoutThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(NonZero(1)) }, at /checkout/src/libcore/option.rs:335)
  │ stack backtrace:
  │    0:     0x7fb5a595605c - backtrace::backtrace::trace::hc558941d466d8278
  │    1:     0x7fb5a5956092 - backtrace::capture::Backtrace::new::ha6795f2bcb9f5158
  │    2:     0x7fb5a40533b5 - servo::main::{{closure}}::h525ba2edc1f14706
  │    3:     0x7fb5a63e0596 - std::panicking::rust_panic_with_hook
  │                         at /checkout/src/libstd/panicking.rs:578
  │    4:     0x7fb5a63e03b4 - std::panicking::begin_panic<alloc::string::String>
  │                         at /checkout/src/libstd/panicking.rs:538
  │    5:     0x7fb5a63e0329 - std::panicking::begin_panic_fmt
  │                         at /checkout/src/libstd/panicking.rs:522
  │    6:     0x7fb5a63e02ba - std::panicking::rust_begin_panic
  │                         at /checkout/src/libstd/panicking.rs:498
  │    7:     0x7fb5a641a680 - core::panicking::panic_fmt
  │                         at /checkout/src/libcore/panicking.rs:71
  │    8:     0x7fb5a641a5b6 - core::panicking::panic
  │                         at /checkout/src/libcore/panicking.rs:51
  │    9:     0x7fb5a42c5617 - layout_thread::LayoutThread::perform_post_style_recalc_layout_passes::hf58c3db4beedaa2e
  │   10:     0x7fb5a42c2d3d - layout_thread::LayoutThread::tick_all_animations::h27d058b230c3b980
  │   11:     0x7fb5a42b00ad - layout_thread::LayoutThread::handle_request_helper::h25bf1197f760cbdf
  │   12:     0x7fb5a42ab834 - <layout_thread::LayoutThread as layout_traits::LayoutThreadFactory>::create::{{closure}}::hee39b95ba0800712
  │   13:     0x7fb5a420b8c5 - std::sys_common::backtrace::__rust_begin_short_backtrace::h6656dd04e3083ef0
  │   14:     0x7fb5a42144a5 - std::panicking::try::do_call::hf425695f73c12133
  │   15:     0x7fb5a63e772c - panic_unwind::__rust_maybe_catch_panic
  │                         at /checkout/src/libpanic_unwind/lib.rs:99
  │   16:     0x7fb5a423eaf2 - <F as alloc::boxed::FnBox<A>>::call_box::h165d5e26aa343cdd
  │   17:     0x7fb5a63df21b - alloc::boxed::{{impl}}::call_once<(),()>
  │                         at /checkout/src/liballoc/boxed.rs:738
  │                          - std::sys_common::thread::start_thread
  │                         at /checkout/src/libstd/sys_common/thread.rs:24
  │                          - std::sys::imp::thread::{{impl}}::new::thread_start
  │                         at /checkout/src/libstd/sys/unix/thread.rs:90
  │   18:     0x7fb5a1d0c183 - start_thread
  │   19:     0x7fb5a1822ffc - clone
  │   20:                0x0 - <unknown>
  │ ERROR:servo: called `Option::unwrap()` on a `None` value
  └ Pipeline failed in hard-fail mode.  Crashing!
@jdm
Copy link
Member

jdm commented Oct 2, 2017

That's the same panic and backtrace that was observed in #18701.

@mrobinson
Copy link
Member Author

mrobinson commented Oct 3, 2017

Going to retry this in case it is an intermittent.

@jdm
Copy link
Member

jdm commented Oct 3, 2017

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Oct 3, 2017

Trying commit 83a9603 with merge 07a45689bfc1128b7c884ccef2fa78067919cb78...

@bors-servo
Copy link
Contributor

bors-servo commented Oct 17, 2017

Testing commit 7692145 with merge a12ba64...

bors-servo added a commit that referenced this pull request Oct 17, 2017
Switch to using WebRender hit testing

This trades quite a bit of complicated code in Servo for few more
messages and a significant performance improvement. In particular,
WebRender can search the entire display list at once instead of
ping-ponging down the pipeline tree. This allows us to send mouse
events to the correct pipeline immediately.

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

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

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because they should not change behavior.

<!-- 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/18704)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 17, 2017

💔 Test failed - linux-rel-css

@jdm
Copy link
Member

jdm commented Oct 17, 2017

error[E0433]: failed to resolve. Use of undeclared type or module `NonZeroU32`
   --> /home/servo/buildbot/slave/linux-rel-css/build/components/msg/constellation_msg.rs:255:38
    |
255 |                 index: PipelineIndex(NonZeroU32::new_unchecked(index)),
    |                                      ^^^^^^^^^^ Use of undeclared type or module `NonZeroU32`
@jdm
Copy link
Member

jdm commented Oct 17, 2017

I think you can use NonZero::new_unchecked now instead.

This trades quite a bit of complicated code in Servo for few more
messages and a significant performance improvement. In particular,
WebRender can search the entire display list at once instead of
ping-ponging down the pipeline tree. This allows us to send mouse
events to the correct pipeline immediately.
@mrobinson mrobinson force-pushed the mrobinson:wr-hit-testing branch from 7692145 to b5d51dd Oct 17, 2017
@mrobinson
Copy link
Member Author

mrobinson commented Oct 17, 2017

Rebased and ready to go.

@bors-servo: r=jdm,glennw,mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Oct 17, 2017

📌 Commit b5d51dd has been approved by jdm,glennw,mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Oct 17, 2017

Testing commit b5d51dd with merge ca08271...

bors-servo added a commit that referenced this pull request Oct 17, 2017
Switch to using WebRender hit testing

This trades quite a bit of complicated code in Servo for few more
messages and a significant performance improvement. In particular,
WebRender can search the entire display list at once instead of
ping-ponging down the pipeline tree. This allows us to send mouse
events to the correct pipeline immediately.

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

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

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because they should not change behavior.

<!-- 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/18704)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 17, 2017

☀️ 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: jdm,glennw,mbrubeck
Pushing ca08271 to master...

@bors-servo bors-servo merged commit b5d51dd into servo:master Oct 17, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@mrobinson
Copy link
Member Author

mrobinson commented Oct 18, 2017

Thanks for the reviews!

@mrobinson mrobinson deleted the mrobinson:wr-hit-testing branch Oct 18, 2017
@jrmuizel
Copy link

jrmuizel commented Oct 18, 2017

@mrobinson How does this handle hit testing transparent backgrounds?

@mrobinson
Copy link
Member Author

mrobinson commented Oct 18, 2017

@jrmuizel Transparent backgrounds are handled by sending transparent rectangles with the display lit. They are added to the list of hit testing items, but are culled early from rendering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.