Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd an explicit API to add reference frames #2756
Conversation
|
cc @staktrace This change was made with ease of transition in mind. The only change necessary for clients will be to push a reference frame and its new ClipId right before the place where the stacking context is pushed. All coordinates are still processed as if they are relative to their parent stacking context. |
|
Reviewed 27 of 27 files at r1. Comments from Reviewable |
|
This looks good to me! It would be good to have a gecko / servo patch ready for this before landing, if possible? |
|
(And also to get sign-off from @staktrace that the API change looks fine). |
|
The API change looks straightforward enough, but I'll do a gecko try push to make sure it's green. |
|
Seems like it's causing a lot of failures. https://hg.mozilla.org/try/rev/9c09292b0d808c6579cd41fb782dee425a0e5f3e is the gecko-side patch I wrote. @mrobinson can you take a look, maybe there's something obvious I missed in the patch? |
|
@staktrace Thanks for writing the Gecko side patch. I'm happy to take a look at this tomorrow. The first thing I'd probably try is to set the stacking context origin to zero. If that's the issue it definitely raises some interesting questions about how to best take this intermediate API step. |
|
Yeah, looks like there is a deeper issue here. I'm on vacation this week, but should be able to look at the Gecko issues on Tuesday of next week. |
Previously reference frames were inserted into the ClipScrollTree when a stacking context had a transformation or a perspective. This changes makes the API for adding reference frames explicit in order to give clients better control over when and how they are created. This change also opens up future opportunities for shrinking the size of the display list (by separating out the ClipScrollTree from the list of display items) and for concurrency during the flattening process.
mrobinson
force-pushed the
mrobinson:separate-reference-frames
branch
from
fbb1f3e
to
f5895e8
|
Sorry for the delay attacking these issues, which were due to a planned vacation. I'm back now and think I have solved the issue in the Gecko side changes. I have a new Gecko try job running here: @staktrace Perhaps you could take a look at the failures and confirm that they aren't related to this PR. They don't look like they are related to me. Thanks! |
|
The failures so far don't look related to this PR and are all pre-existing intermittents. Probably worth waiting for at least the OS X and Windows reftests to finish before merging though. |
|
@staktrace Thanks for looking at the results. The job has finished now. There is one failure in the Windows results and none for OS X. The Windows failure look unrelated to me, though I'm having trouble interpreting the log and I don't see a bugzilla bug open for it. |
|
Yeah the windows xperf talos test failure is expected. That test doesn't run on m-c but for some reason still runs in try pushes. |
|
@gw3583 This should be good to review whenever you have a free moment. |
|
@mrobinson Ah, sorry - missed your comment. Will review today. |
|
Review status: all files reviewed at latest revision, all discussions resolved. Comments from Reviewable |
|
@mrobinson Ah, this was already mostly reviewed. Looks good, r=me once @staktrace is happy for this to merge. It would be good to have Gecko and Servo-side patches ready to go, if that's easy enough to do. |
|
@gw3583 Thanks! The Gecko patch should be attached to the try job and I'll get a Servo patch written today. There is one dependency though, which is waiting on review. Perhaps you could take a look? It is at servo/servo#20767. |
|
@gw3583 The Servo bit should be ready here: https://github.com/mrobinson/servo/commits/separate-reference-frame-api-update. This branch includes servo/servo#20767. |
|
@mrobinson OK, reviewed and r+'ed servo/servo#20767 |
|
@staktrace asked for a rebased Gecko try job. That is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c55e905ffea5271595b4d65563b17680b770dc76&selectedJob=181351208 Right now it's at 96% and the failures look to be intermittent. |
|
Yup, try push looks good |
|
|
Add an explicit API to add reference frames Previously reference frames were inserted into the ClipScrollTree when a stacking context had a transformation or a perspective. This changes makes the API for adding reference frames explicit in order to give clients better control over when and how they are created. This change also opens up future opportunities for shrinking the size of the display list (by separating out the ClipScrollTree from the list of display items) and for concurrency during the flattening 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/webrender/2756) <!-- Reviewable:end -->
|
@bors-servo r- |
|
|
Add an explicit API to add reference frames Previously reference frames were inserted into the ClipScrollTree when a stacking context had a transformation or a perspective. This changes makes the API for adding reference frames explicit in order to give clients better control over when and how they are created. This change also opens up future opportunities for shrinking the size of the display list (by separating out the ClipScrollTree from the list of display items) and for concurrency during the flattening 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/webrender/2756) <!-- Reviewable:end -->
|
Sorry. I meant to write "r=glennw." I'll let bors stall out and then land this properly. |
|
|
|
Odd. Typically r- should halt the merge. I'm not sure what happened here. |
mrobinson commentedMay 14, 2018
•
edited by larsbergstrom
Previously reference frames were inserted into the ClipScrollTree when a
stacking context had a transformation or a perspective. This changes
makes the API for adding reference frames explicit in order to give
clients better control over when and how they are created. This change
also opens up future opportunities for shrinking the size of the display
list (by separating out the ClipScrollTree from the list of display
items) and for concurrency during the flattening process.
This change is