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

Remove ServoScrollRootId #983

Merged
merged 1 commit into from Mar 20, 2017
Merged

Conversation

@mrobinson
Copy link
Member

mrobinson commented Mar 14, 2017

Remove the concept of ServoScrollRootId and just wrap up external ids
into ScrollLayerId. This will allow API clients to specify ids without
having to do complicated bookkeeping when building display lists. In
addition, this makes the code much simpler. This is a major API break
because clips cannot share the same id. This is only possible because
now Servo does not need to split scroll layers / clips.


This change is Reviewable

@mrobinson
Copy link
Member Author

mrobinson commented Mar 14, 2017

@glennw r?

@mrobinson mrobinson force-pushed the mrobinson:remove-scroll-root-id branch from 45ee625 to 59e6e92 Mar 14, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Mar 14, 2017

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

Remove the concept of ServoScrollRootId and just wrap up external ids
into ScrollLayerId. This will allow API clients to specify ids without
having to do complicated bookkeeping when building display lists. In
addition, this makes the code much simpler. This is a major API break
because clips cannot share the same id. This is only possible because
now Servo does not need to split scroll layers / clips.
@mrobinson mrobinson force-pushed the mrobinson:remove-scroll-root-id branch from 59e6e92 to a71585e Mar 14, 2017
@glennw
Copy link
Member

glennw commented Mar 15, 2017

@mrobinson Is there a corresponding Servo patch for this that I can do some testing with / use for the next WR update?

@mrobinson
Copy link
Member Author

mrobinson commented Mar 16, 2017

@glennw I've cleaned up my Servo work here: https://github.com/mrobinson/servo/tree/scrolling. I'm getting a couple timeouts locally, but I think they are independent of my change. If you had a moment to run the tests, I would be very grateful. In WebRender I had to revert a commit to run it with the most recent Servo (59c6fe7).

@glennw
Copy link
Member

glennw commented Mar 16, 2017

Reviewed 14 of 14 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@glennw
Copy link
Member

glennw commented Mar 16, 2017

@mrobinson OK, this looks good to me. I'd like to hold off on merging it until we have the corresponding Servo PR ready to go for the WR update though. I'm working through the remaining changes in Servo and dependencies to deal with the GL function pointer change, so that shouldn't be too far away.

@glennw
Copy link
Member

glennw commented Mar 19, 2017

@mrobinson I realized my comment above might be ambiguous - I'm fine with merging this so long as we have a corresponding commit for Servo that I can cherry-pick when I do a WR update. This doesn't need to wait on the GL function pointer changes.

@glennw
Copy link
Member

glennw commented Mar 19, 2017

r=me once there is a servo patch available that I can use in the WR update.

@mrobinson
Copy link
Member Author

mrobinson commented Mar 20, 2017

Okay. Thanks! The patch is in my scrolling branch here: https://github.com/mrobinson/servo/tree/scrolling. Just let me know if you need my help in any way. I'm happy to do merges or even work on the function pointer change, if you like.

@mrobinson
Copy link
Member Author

mrobinson commented Mar 20, 2017

@bors-servo r=glennw

@bors-servo
Copy link
Contributor

bors-servo commented Mar 20, 2017

📌 Commit a71585e has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Mar 20, 2017

Testing commit a71585e with merge b9a0406...

bors-servo added a commit that referenced this pull request Mar 20, 2017
Remove ServoScrollRootId

Remove the concept of ServoScrollRootId and just wrap up external ids
into ScrollLayerId. This will allow API clients to specify ids without
having to do complicated bookkeeping when building display lists. In
addition, this makes the code much simpler. This is a major API break
because clips cannot share the same id. This is only possible because
now Servo does not need to split scroll layers / clips.

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

bors-servo commented Mar 20, 2017

☀️ Test successful - status-travis
Approved by: glennw
Pushing b9a0406 to master...

@bors-servo bors-servo merged commit a71585e into servo:master Mar 20, 2017
3 checks passed
3 checks passed
code-review/reviewable 14 files reviewed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@mrobinson mrobinson deleted the mrobinson:remove-scroll-root-id branch Mar 20, 2017
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

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