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 upWait as late as possible to assign ClipIds #18951
Merged
+365
−274
Conversation
highfive
commented
Oct 19, 2017
|
Heads up! This PR modifies the following files:
|
highfive
commented
Oct 19, 2017
|
|
|
@glennw This is the first step toward eliminating rounded rectangle from LocalClip. After this we just need to:
|
|
|
mrobinson
force-pushed the
mrobinson:later-id-assignment-for-clip-scroll-nodes
branch
from
8a156cb
to
a0c12b6
|
Reviewed 6 of 6 files at r1. components/gfx/display_list/mod.rs, line 229 at r1 (raw file):
nit: could use the new syntax to avoid duplicating the names here components/layout/display_list_builder.rs, line 215 at r1 (raw file):
Should this be zero? components/layout/display_list_builder.rs, line 275 at r1 (raw file):
remove this? Comments from Reviewable |
|
Nice! One question - and a couple of minor nits. r=me once those are resolved. |
jdm
added
S-awaiting-answer
S-needs-code-changes
and removed
S-awaiting-review
S-needs-rebase
labels
|
|
|
|
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. components/gfx/display_list/mod.rs, line 229 at r1 (raw file): Previously, glennw (Glenn Watson) wrote…
Oh. Good idea. I'll do that. components/layout/display_list_builder.rs, line 215 at r1 (raw file): Previously, glennw (Glenn Watson) wrote…
This node is only used to take up a spot in the list of ClipScrollNodes the real root scroll node is added by WebRender (though I think we should change that eventually, because it's an extra node that Gecko doesn't need). components/layout/display_list_builder.rs, line 275 at r1 (raw file): Previously, glennw (Glenn Watson) wrote…
Thanks! Not sure how that slipped through. I'll also add a documentation comment for the member. Comments from Reviewable |
mrobinson
force-pushed the
mrobinson:later-id-assignment-for-clip-scroll-nodes
branch
from
a0c12b6
to
b3c8e4e
|
|
mrobinson
force-pushed the
mrobinson:later-id-assignment-for-clip-scroll-nodes
branch
from
b3c8e4e
to
cc7b01d
|
@bors-servo r=glennw |
|
|
bors-servo
added a commit
that referenced
this pull request
Oct 22, 2017
…nodes, r=glennw Wait as late as possible to assign ClipIds <!-- 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/18951) <!-- Reviewable:end -->
|
|
|
mrobinson
force-pushed the
mrobinson:later-id-assignment-for-clip-scroll-nodes
branch
from
cc7b01d
to
636722a
|
This seems to have exposed some cases where we were inheriting properties that we should not have been for table pseudo-elements (elements created by display: table* properties). I have updated the PR to:
Maybe @emilio can review the changes to resources/servo.css to ensure that they make sense? |
This will allow Servo to create ClipScrollNodes later during display list construction, which will be necessary once rounded rectangles are removed from the LocalClip structure. Instead of keeping track of the ClipId of each ClipScrollNode, we keep track of its index in an array of ClipScrollNodes. This will allow us to access them without a hash lookup.
mrobinson
force-pushed the
mrobinson:later-id-assignment-for-clip-scroll-nodes
branch
from
636722a
to
5937f62
|
@bors-servo r=glennw,emilio |
|
|
bors-servo
added a commit
that referenced
this pull request
Oct 24, 2017
…nodes, r=glennw,emilio Wait as late as possible to assign ClipIds <!-- 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/18951) <!-- Reviewable:end -->
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
mrobinson commentedOct 19, 2017
•
edited by SimonSapin
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is