[Masonry] Support column|row-reverse for dense-packing [chromium/src : main]
Celeste Pan (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Alison Maher
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 11 comments![]( "Open in Gerrit")
Some initial thoughts, haven't looked at tests yet
This change fixes a previously existing bug in RunningPositionsIteratornit: missing ``
https://issues.chromium.org/issues/458993338, the tests in this changeMight be worth linking to this bug below, as well, or splitting this piece out into a pre-requisite CL
Some changes were made to RunningPositionsIterator as we want to make itnit: Missing ``
// TODO(celestepan): we may want to remove the CHECK in the pre-increment
// operator if we choose to have this iterator loop infinitely.nit: is this now removable at this point?
// the direction in which we iterate through the vector.I'd expand on this comment to include something on `max_index`
max_index_(max_index),Wondering if we should do a CHECK below here that max_index_ is < running_positions.size() (same with auto_placement_index now that I think about it)
end_index_ = (current_index_ < max_index_) ? current_index_ + 1 : 0;I think this is guarenteed to be the case in this else statement (since the if checks `current_index_ >= max_index_`) so we could condense this if else to either
`end_index_ = (current_index_ < max_index_) ? current_index_ + 1 : 0;`
// Post-increment operatornit: missing punctuation. Also might be worth expanding on why we use a post increment operator in the comment if there is an easy to explain reason that is general
/*max_index=*/max_running_positions.size() - 1, max_running_positions);Could this one also be `- span_size` similar to below, or does it need to be - 1 in this case? If so, we could avoid passing in a max_index I think
is_reverse_direction_ ? running_positions_.size() : 0, span_size,Should this be - 1?
Related details
- Celeste Pan
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Celeste Pan (Gerrit)
Celeste Pan added 10 comments![]( "Open in Gerrit")
This change fixes a previously existing bug in RunningPositionsIteratorCeleste Pannit: missing ``
Done
https://issues.chromium.org/issues/458993338, the tests in this changeMight be worth linking to this bug below, as well, or splitting this piece out into a pre-requisite CL
Done
Some changes were made to RunningPositionsIterator as we want to make itCeleste Pannit: Missing ``
Done
// TODO(celestepan): we may want to remove the CHECK in the pre-increment
// operator if we choose to have this iterator loop infinitely.nit: is this now removable at this point?
Yup!
// the direction in which we iterate through the vector.I'd expand on this comment to include something on `max_index`
`max_index` parameter was removed, added a comment about usage of `max_index_` above the private variable.
Wondering if we should do a CHECK below here that max_index_ is < running_positions.size() (same with auto_placement_index now that I think about it)
Added check for `max_index_` but not `auto_placement_index` since it's possible that it will be greater than running_positions.size(), and in that case we adjust it.
end_index_ = (current_index_ < max_index_) ? current_index_ + 1 : 0;I think this is guarenteed to be the case in this else statement (since the if checks `current_index_ >= max_index_`) so we could condense this if else to either
`end_index_ = (current_index_ < max_index_) ? current_index_ + 1 : 0;`
Done
// Post-increment operatornit: missing punctuation. Also might be worth expanding on why we use a post increment operator in the comment if there is an easy to explain reason that is general
The reasoning is that due to infinite looping, if we use `ReturnedToStart()` as a conditional for exiting the loop, we cannot perform incrementation inside of the loop (in our case, the `do` of the loop). By using a post-increment operator, we can cleanly check the value of ReturnedToStart() and increment afterwards.
I'm kind of on the fence of actually putting this in, since it doesn't feel that general (it's affected by how we're specifically calling RunningPositionsIterator); what are your thoughts?
/*max_index=*/max_running_positions.size() - 1, max_running_positions);Could this one also be `- span_size` similar to below, or does it need to be - 1 in this case? If so, we could avoid passing in a max_index I think
Technically, the size of `max_running_positions` will always be `running_positions.size() - span_size`. However, I write it like this here so we maintain consistency with the vector that we're actually accessing.
We can't get rid of `max_index_` and just perform a `running_positions.size() - span_size` calculation, because then that calculation using when we pass in `max_running_positions` for the `running_positions` parameter would be inaccurate.
I ended up removing the GetMaxPositionsForAllTracks optimization and performing the calculation in the `RunningPositionsIterator` ctor, as it would really only save significant memory for extremely large span sizes. If we hit performance issues specifically for large span sizes, this is something we can easily change.
is_reverse_direction_ ? running_positions_.size() : 0, span_size,Should this be - 1?
Not in this case, because we should be passing in what is essentially the auto-placement cursor's position, and in this case, it should be at the very last line (if there are 3 tracks, the auto-placement cursor would be at the last line, which in our current 0-index state, would be 3). `auto_placement_index` was what had been suggested in the previous change, but I think that terminology is actually confusing in our case; perhaps simply having `auto_placement_cursor` as the parameter name would be more accurate.
Related details
- Alison Maher
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 7 comments![]( "Open in Gerrit")
EXPECT_EQ(GetMaxPositionsForAllTracks(running_positions, /*span_size=*/2),
Vector<LayoutUnit>({LayoutUnit(3), LayoutUnit(3.5), LayoutUnit(3.5),
LayoutUnit(3.5)}));
EXPECT_EQ(GetMaxPositionsForAllTracks(running_positions, /*span_size=*/4),
Vector<LayoutUnit>({LayoutUnit(3.5), LayoutUnit(3.5),
LayoutUnit(3.5), LayoutUnit(3.5)}));Out of curiosity, why did these end up changing?
CHECK_LT(max_index_, running_positions_.size());nit: Since we now set this to `(running_positions.size() - span_size)`, rather than passing it in, this is now implied, so ok to remove
// Post-increment operatorCeleste Pannit: missing punctuation. Also might be worth expanding on why we use a post increment operator in the comment if there is an easy to explain reason that is general
The reasoning is that due to infinite looping, if we use `ReturnedToStart()` as a conditional for exiting the loop, we cannot perform incrementation inside of the loop (in our case, the `do` of the loop). By using a post-increment operator, we can cleanly check the value of ReturnedToStart() and increment afterwards.
I'm kind of on the fence of actually putting this in, since it doesn't feel that general (it's affected by how we're specifically calling RunningPositionsIterator); what are your thoughts?
Yeah I'm ok with leaving that out in this case
.masonry {
display: masonry;As a heads up, I have a change to update everything use grid-lanes https://chromium-review.googlesource.com/c/chromium/src/+/7170998?tab=checks
I have been dealing with a bunch of merge conflicts from Apple, so in order to avoid more, I'm hoping we can land that one before other CLs with tests. In which case, we'd need to update this and other tests to `display: grid-lanes` and the class name to `.grid-lanes` (but I'd wait updating until that change lands)
<div name="item_is_densely_packed_into_start_most_gap_of_the_same_layout_size_as_auto_placement_without_dense_packing" style="background: yellow; height: 20px;">Is this name, and the ones below needed? Could maybe be a comment, instead, if needed
Same comment for rows
<p>Ensure that dense-packing with multi-span items are placed into tracks with the same used size and the same number of tracks.</p>This might make sense as 5 separate tests instead of one
Same comment for rows
<div style="background: lightsteelblue; height: 60px; grid-column: 1;">Is this needed? Shouldn't it end up here anyways per dense packing?
Related details
- Celeste Pan
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Celeste Pan (Gerrit)
Celeste Pan added 6 comments![]( "Open in Gerrit")
EXPECT_EQ(GetMaxPositionsForAllTracks(running_positions, /*span_size=*/2),
Vector<LayoutUnit>({LayoutUnit(3), LayoutUnit(3.5), LayoutUnit(3.5),
LayoutUnit(3.5)}));
EXPECT_EQ(GetMaxPositionsForAllTracks(running_positions, /*span_size=*/4),
Vector<LayoutUnit>({LayoutUnit(3.5), LayoutUnit(3.5),
LayoutUnit(3.5), LayoutUnit(3.5)}));Out of curiosity, why did these end up changing?
Since I removed the optimization for GetMaxPositionsForAllTracks, now the vector returned by the method will equal the number of tracks, as opposed to number of tracks minus the span size.
nit: Since we now set this to `(running_positions.size() - span_size)`, rather than passing it in, this is now implied, so ok to remove
Done
As a heads up, I have a change to update everything use grid-lanes https://chromium-review.googlesource.com/c/chromium/src/+/7170998?tab=checks
I have been dealing with a bunch of merge conflicts from Apple, so in order to avoid more, I'm hoping we can land that one before other CLs with tests. In which case, we'd need to update this and other tests to `display: grid-lanes` and the class name to `.grid-lanes` (but I'd wait updating until that change lands)
Sounds good, thank you for the heads up!
<div name="item_is_densely_packed_into_start_most_gap_of_the_same_layout_size_as_auto_placement_without_dense_packing" style="background: yellow; height: 20px;">Is this name, and the ones below needed? Could maybe be a comment, instead, if needed
Same comment for rows
I'll add a comment instead then! I do like having these descriptions so later on I can keep track of what's being tested.
<p>Ensure that dense-packing with multi-span items are placed into tracks with the same used size and the same number of tracks.</p>This might make sense as 5 separate tests instead of one
Same comment for rows
Done
<div style="background: lightsteelblue; height: 60px; grid-column: 1;">Is this needed? Shouldn't it end up here anyways per dense packing?
- Alison Maher
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alison Maher
| Code-Review | +1 |
2 comments
Related details
- Celeste Pan
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 4 comments![]( "Open in Gerrit")
// Post-increment operator.
RunningPositionsIterator operator++(int) {Since you're doing more work after the iteration, does a pre-increment make more sense here?
RunningPositionsIterator prev(*this);I'm assuming `prev` means previous, but shouldn't this be `next`?
bool ReturnedToStart() { return current_index_ == end_index_; }Iterators have a concept of "end", that seems like a better fit here, see the example here: https://en.cppreference.com/w/cpp/iterator/iterator.html
It might be simpler here to just name this `end` instead of returning a new iterator like the example does though.
Then your usage can be:
```
while (++iterator != iterator.end());
```
// placing multi-span items. Example case: if Track 3 has an opening from 40px
// -> 50px and Track 4 is fully open starting from 30px, we should be attempting
// to place an item which spans Track 3 -> Track 4.This example would be helpful with some ASCII art. Copilot/ChatGPT/Gemini should be able to generate it pretty easily, just double check it.
Related details
- Celeste Pan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Celeste Pan (Gerrit)
Celeste Pan added 6 comments![]( "Open in Gerrit")
// Post-increment operator.
RunningPositionsIterator operator++(int) {Since you're doing more work after the iteration, does a pre-increment make more sense here?
I think pre-increment is if incrementing the value before we return `this`, while post-increment is if we're returning the incremented value then doing work, so I believe "post-increment" is the right term in this case!
RunningPositionsIterator prev(*this);I'm assuming `prev` means previous, but shouldn't this be `next`?
Hm, I mean `prev` represents the previous value of `*this` that we return, so I think that using `next` wouldn't necessarily make sense, since it's not the `next` value that this will become. I could do something like `prev_value`; what are your thoughts?
bool ReturnedToStart() { return current_index_ == end_index_; }Iterators have a concept of "end", that seems like a better fit here, see the example here: https://en.cppreference.com/w/cpp/iterator/iterator.html
It might be simpler here to just name this `end` instead of returning a new iterator like the example does though.
Then your usage can be:
```
while (++iterator != iterator.end());
```
Sure, done!
// placing multi-span items. Example case: if Track 3 has an opening from 40px
// -> 50px and Track 4 is fully open starting from 30px, we should be attempting
// to place an item which spans Track 3 -> Track 4.This example would be helpful with some ASCII art. Copilot/ChatGPT/Gemini should be able to generate it pretty easily, just double check it.
Done
Related details
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Celeste Pan (Gerrit)
Celeste Pan added 1 comment![]( "Open in Gerrit")
Rebase - tests moved into grid-lanes, but previous comments were addressed before rebase.
Related details
- Alison Maher
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 4 comments![]( "Open in Gerrit")
// TODO(crbug.com/361780162): This test has been flaky on Windows ASan testers.Guessing this was left from a bad merge maybe?
RunningPositionsIterator prev(*this);Celeste PanI'm assuming `prev` means previous, but shouldn't this be `next`?
Hm, I mean `prev` represents the previous value of `*this` that we return, so I think that using `next` wouldn't necessarily make sense, since it's not the `next` value that this will become. I could do something like `prev_value`; what are your thoughts?
Yeah maybe `prev_position` would help
// able to place the item laid out across Track and Track 2, even though Track 1nit: missing number
masonry-direction: column-reverse;This is now using grid-lanes-direction. Feel free to lmk if I need to take another look after reabse
Related details
- Celeste Pan
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Celeste Pan (Gerrit)
Celeste Pan added 5 comments![]( "Open in Gerrit")
Rebase to use `grid-lanes-direction` instead of `masonry-direction`. Also addressed comments.
// TODO(crbug.com/361780162): This test has been flaky on Windows ASan testers.Guessing this was left from a bad merge maybe?
I'm not sure why it's saying that I removed it, when the version I have is what's actually showing up in chromium code search; I was planning on rebasing again later, which I can do now!
Next time I'll leave a comment with a note. Here's a link to the current code (https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/frame/browser_frame_view_browsertest_win.cc;l=498?q=chrome%2Fbrowser%2Fui%2Fviews%2Fframe%2Fbrowser_frame_view_browsertest_win.cc&ss=chromium) which is what I have right now.
RunningPositionsIterator prev(*this);Celeste PanI'm assuming `prev` means previous, but shouldn't this be `next`?
Alison MaherHm, I mean `prev` represents the previous value of `*this` that we return, so I think that using `next` wouldn't necessarily make sense, since it's not the `next` value that this will become. I could do something like `prev_value`; what are your thoughts?
Yeah maybe `prev_position` would help
Done
// able to place the item laid out across Track and Track 2, even though Track 1Celeste Pannit: missing number
Done
This is now using grid-lanes-direction. Feel free to lmk if I need to take another look after reabse
- Alison Maher
- Ian Kilpatrick
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher voted Code-Review+1![]( "Open in Gerrit")
- Celeste Pan
- Ian Kilpatrick
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/56314.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Celeste Pan (Gerrit)
Celeste Pan voted Commit-Queue+2![]( "Open in Gerrit")
- Ian Kilpatrick
- Kurt Catti-Schmidt
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Masonry] Support column|row-reverse for dense-packing
This change adds support for reverse item placement when dense-packing
is enabled.
This change fixes a previously existing bug in
`RunningPositionsIterator` constructor where `end_index_` was not
correctly calculated. This was caught by multiple fuzzer tests as well
(listed in the Bug section), and tests were added in this change to
cover for this case.
The optimization in `GetMaxPositionsForAllTracks` where a vector
`span_size` smaller than `running_positions_` is return has been removed
to allow for cleaner more readable code in `RunningPositionsIterator`.
The optimization should only be significant when placing an item with a
very large span size.
Some changes were made to `RunningPositionsIterator` as we want to make
it usable for both the regular reverse placement case and the
dense-packing reverse placement case:
1) The `RunningPositionsIterator` constructor has been updated to take
in a `max_index_` (highest possible index we can access safely) value,
as opposed to calculating the value within the class, given that the
calculations of what the `max_index_` would be vary across different
usages of the iterator.
2) The pre-increment operator for `RunningPositionsIterator` was
removed, and we now use a post-increment operator, which is needed if we
wish to use `wtf_size_t` (unsigned values) for our indices, maintain an
infinite looping capability, and check if the iterator has returned to
its starting index.
- M third_party/blink/renderer/core/layout/masonry/masonry_layout_algorithm.cc
- M third_party/blink/renderer/core/layout/masonry/masonry_layout_algorithm_test.cc
- M third_party/blink/renderer/core/layout/masonry/masonry_running_positions.cc
- M third_party/blink/renderer/core/layout/masonry/masonry_running_positions.h
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/column-reverse-dense-packing-001-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/column-reverse-dense-packing-001.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/column-reverse-dense-packing-multi-span-001-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/column-reverse-dense-packing-multi-span-001.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/column-reverse-dense-packing-multi-span-002-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/column-reverse-dense-packing-multi-span-002.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/column-reverse-dense-packing-multi-span-003-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/column-reverse-dense-packing-multi-span-003.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/column-reverse-dense-packing-multi-span-004-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/column-reverse-dense-packing-multi-span-004.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/column-reverse-dense-packing-multi-span-005-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/column-reverse-dense-packing-multi-span-005.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/row-reverse-dense-packing-001-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/row-reverse-dense-packing-001.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/row-reverse-dense-packing-multi-span-001-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/row-reverse-dense-packing-multi-span-001.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/row-reverse-dense-packing-multi-span-002-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/row-reverse-dense-packing-multi-span-002.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/row-reverse-dense-packing-multi-span-003-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/row-reverse-dense-packing-multi-span-003.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/row-reverse-dense-packing-multi-span-004-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/row-reverse-dense-packing-multi-span-004.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/row-reverse-dense-packing-multi-span-005-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/tentative/item-placement/row-reverse-dense-packing-multi-span-005.html
Code-Review: +1 by Alison Maher
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/56314
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |