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 support for some basic queries to layout_2020 #25690
Merged
Conversation
highfive
commented
Feb 5, 2020
|
Heads up! This PR modifies the following files:
|
components/layout_2020/flow/root.rs
Outdated
| Au::from_f32_px(self.bounding_box_of_border_boxes.size.height.px()), | ||
| ); | ||
| Rect::new(origin, size) | ||
| fn iterate_through_fragments(&self, iterator: &mut dyn FragmentIterator) { |
This comment has been minimized.
This comment has been minimized.
SimonSapin
Feb 5, 2020
Member
Suggested change
| fn iterate_through_fragments(&self, iterator: &mut dyn FragmentIterator) { | |
| fn find_fragment<T>(&self, iterator: impl FnMut(&Fragment, &PhysicalRect<Length>) -> Option<T>) -> Option<T> { |
- Why
dyn Traitoverimpl Trait? I tend to default monomorphized static dispatch because it gives more inlining opportunities to the optimizer, but if there’s a reason to prefer dynamic dispatch I’m not opposed - Both impls of the
FragmentIteratortrait returnfalsewhen they’ve found an appropriate fragment, and store some result in a field as a side effect. If we make that result be part of the return value instead (by replacingboolwithOption) them those fields initialized to a zero/dummy value would not be needed. - With the change above there would be no need to access fields after iteration as ended, so by replacing the custom trait with
FnMutwe could use closure syntax and not need to define explicit structs, which would be more concise.
What do you think?
This comment has been minimized.
This comment has been minimized.
mrobinson
Feb 7, 2020
Author
Member
Why dyn Trait over impl Trait? I tend to default monomorphized static dispatch because it gives more inlining opportunities to the optimizer, but if there’s a reason to prefer dynamic dispatch I’m not opposed
There's no explicit reason. I've modified this to use impl Trait static dispatch.
Both impls of the FragmentIterator trait return false when they’ve found an appropriate fragment, and store some result in a field as a side effect. If we make that result be part of the return value instead (by replacing bool with Option) them those fields initialized to a zero/dummy value would not be needed.
One issue is that some of these iterators will need to accumulate values. In fact, I have just modified the getBoundingClientRect iterator to accumulate a single rectangle for every fragment that has the appropriate tag. I think this is what should be done here. Perhaps it's still possible to do this with two generic type parameters?
This comment has been minimized.
This comment has been minimized.
SimonSapin
Feb 7, 2020
Member
With FnMut (as opposed to Fn) closures can mutate their environment / captures. So accumulating things across calls should not be a problem. Would you like me to try moving to closures?
This comment has been minimized.
This comment has been minimized.
mrobinson
Feb 10, 2020
Author
Member
Oops. I think I misinterpreted your FnMut suggestion. I've pushed a new version of the branch that does away with the traits and uses mutable closures.
components/layout_2020/flow/root.rs
Outdated
| Au::from_f32_px(rect.origin.x.px()), | ||
| Au::from_f32_px(rect.origin.y.px()), | ||
| ), | ||
| Size2D::new( | ||
| Au::from_f32_px(rect.size.width.px()), | ||
| Au::from_f32_px(rect.size.height.px()), |
Comment on lines
252
to
282
This comment has been minimized.
This comment has been minimized.
SimonSapin
Feb 5, 2020
Member
It seems silly to do a lossy conversion from floating point to integer here when all consumers end up doing the opposite one. For example:
servo/components/script/dom/element.rs
Lines 2131 to 2138 in 5f55cd5
We can probably avoid it by changing Rect<Au> types to Rect<Length> in the query response, and move the conversion to Layout 2013 when creating that response. But let’s keep this for a future PR.
This comment has been minimized.
This comment has been minimized.
mrobinson
force-pushed the
mrobinson:queries
branch
2 times, most recently
from
e61a78b
to
9e9be43
|
@SimonSapin, thanks for the review! I've pushed a new version of this change. |
|
Looks good. I’ve taken the liberty to add some commits to do the next step of returning @bors-servo r+ |
|
|
bors-servo
added a commit
that referenced
this pull request
Feb 10, 2020
Add support for some basic queries to layout_2020 These queries will make it easier to track progressions in the layout_2020 code. --- <!-- 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: --> - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- 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. -->
|
|
|
|
Hmm my commits seem to have changed the behavior more than I expected. I’ll look into it. |
|
|
bors-servo
added a commit
that referenced
this pull request
Feb 11, 2020
Add support for some basic queries to layout_2020 These queries will make it easier to track progressions in the layout_2020 code. --- <!-- 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: --> - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- 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. -->
|
|
|
@bors-servo retry |
bors-servo
added a commit
that referenced
this pull request
Feb 11, 2020
Add support for some basic queries to layout_2020 These queries will make it easier to track progressions in the layout_2020 code. --- <!-- 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: --> - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- 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. -->
|
|
|
@bors-servo retry |
bors-servo
added a commit
that referenced
this pull request
Feb 11, 2020
Add support for some basic queries to layout_2020 These queries will make it easier to track progressions in the layout_2020 code. --- <!-- 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: --> - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- 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. -->
|
|
|
@bors-servo retry |
bors-servo
added a commit
that referenced
this pull request
Feb 11, 2020
Add support for some basic queries to layout_2020 These queries will make it easier to track progressions in the layout_2020 code. --- <!-- 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: --> - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- 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. -->
|
|
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 commentedFeb 5, 2020
These queries will make it easier to track progressions in the layout_2020 code.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors