[Masonry] Implement dense-packing for multi-spanning items [chromium/src : main]

0 views
Skip to first unread message

Celeste Pan (Gerrit)

unread,
Sep 15, 2025, 9:07:00 PMSep 15
to Alison Maher, Kurt Catti-Schmidt, Ian Kilpatrick, Yanling Wang, Hoch Hochkeppel, Javier Fernandez, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org
Attention needed from Alison Maher and Kurt Catti-Schmidt

Celeste Pan added 42 comments

File third_party/blink/renderer/core/layout/masonry/masonry_running_positions.h
Line 196, Patchset 15: wtf_size_t track_to_check_for_openings,
Alison Maher . resolved

next_track_index?

Celeste Pan

I think in this case the current variable name makes the most sense, since `next_track_index` would imply that this isn't the current track index we're working with. Will keep this for now, if I get other comments/you do think it's very confusing I'll change it!

Line 195, Patchset 15: wtf_size_t num_tracks_remaining_to_check,
Alison Maher . resolved

Maybe just `track_count_remaining`

Celeste Pan

Done

Line 191, Patchset 15: bool FindPathOfTrackOpeningsToAccomodateItem(
Alison Maher . resolved

I wonder if this should be AccumulateTrackOpeningsToAccomodateItem() since this only takes one track at a time

Celeste Pan

Done

Line 188, Patchset 15: // track openings which align to accomodate an item with the height
Alison Maher . resolved

block size, although in rows it would be the inline size, right, not the height/block size.

so maybe "with a contribution size in the stacking axis of `item_stacking_axis_contribution`"

Celeste Pan

Done

Line 184, Patchset 15: // Calculate the total size of the tracks across the given span.
LayoutUnit CalculateUsedTrackSize(const GridSpan& span);
Alison Maher . resolved

I don't know that we can rely on the total. I think we need to guarantee that the span is placed in tracks of the exact same size in the same order as where it was originally laid out

So if we had tracks
50px 25px 25px 50px

And a 2 track spanner was placed in track 2 (i.e. 25px 50px), we would only look for openings starting at track 2 since that is the only way you'd get the same track sizes in the same order

Celeste Pan

Discussed this offline, but based on the CSSWG discussions it seems like only the total track size size and # of spans matter: https://github.com/w3c/csswg-drafts/issues/9326, but we are now tracking a task to confirm that the current behavior is what we are looking for.

Line 151, Patchset 15: wtf_size_t opening_index{0};
Alison Maher . resolved

Is this an index or an offset?

Celeste Pan

This is an index, so it would be accessing the specific opening within the vector of track openings for each track.

Line 150, Patchset 15: wtf_size_t track_index{0};
Alison Maher . resolved

Do we need to track index for each or just the index of the first track in the sequence (since they have to be consecutive)?

Celeste Pan

Good idea, we can definitely just track the first index!

Line 143, Patchset 15: // opening in `track_collection_openings_`. `start_position` refers to the
Alison Maher . resolved

nit: extra space

Celeste Pan

Done

Line 142, Patchset 15: // `EligibleTrackOpeningPositions` holds the indices within for a track
Alison Maher . resolved

nit: within what?

Celeste Pan

fixed comment.

Line 142, Patchset 15: // `EligibleTrackOpeningPositions` holds the indices within for a track
Alison Maher . resolved

Is this defined anywhere?

Celeste Pan

It's supposed to be `EligibleTrackOpeningIndices `, will fix the comment!

File third_party/blink/renderer/core/layout/masonry/masonry_running_positions.cc
Line 92, Patchset 15: LayoutUnit used_track_size;
Kurt Catti-Schmidt . resolved

Can this get called multiple times with the same span? If so, it might be worth adding a cache (as a TODO).

Celeste Pan

It's possible that we'll get the overlap just one time, but I'm not sure if it's worth adding a cache in that case.

For example, we calculate the used track size of `masonry_item_span_ref`, and then later when we're iterating through the tracks trying to find a path of track openings, we will end up hitting the same span again and we'll re-calculate for the same span in that case.

I'll resolve this for now, but if you do think a cache is a good idea, please re-open!

Line 96, Patchset 15: start_line++) {
Kurt Catti-Schmidt . resolved

`++start_line` per https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement

(and in the loop in `FindPathOfTrackOpeningsToAccomodateItem)

Celeste Pan

Done

Line 97, Patchset 15: CHECK_LT(start_line, track_collection_sizes_.size());
Kurt Catti-Schmidt . resolved

This seems unnecessary, given the `CHECK_LE` above on `end_line`.

Celeste Pan

Done

Line 107, Patchset 15: wtf_size_t num_tracks_remaining_to_check,
Kurt Catti-Schmidt . resolved

`num_tracks_remaining`

Celeste Pan

Actually, I'll go with this suggestion instead of alison's!

Line 111, Patchset 15: LayoutUnit overlap_range_size;
Kurt Catti-Schmidt . resolved

This doesn't need to be outside of the loop. Putting it in the loop will allow it to be `const`.

Celeste Pan

Done

Line 112, Patchset 15: Vector<TrackOpening> current_track_openings =
Kurt Catti-Schmidt . resolved

`const` (can this be a `const&`?)

(a few more locals in this method can be `const` as well, like `overlap_start_position`, `overlap_end_position`, `found_eligible_track_opening_span `)

Celeste Pan

Done

Line 116, Patchset 15:
// Calculate the overlap between the previous track's eligible opening and
// the current opening.
Kurt Catti-Schmidt . resolved

You should add a comment explaining why the start position uses `max` and the end uses `min`.

Celeste Pan

Done

Line 125, Patchset 15: overlap_range_size = overlap_end_position - overlap_start_position;
Kurt Catti-Schmidt . resolved

Is this allowed to be negative? Or possible to be negative? Might be worth adding a `DCHECK`.

Celeste Pan

Yes, it's possible that it would be negatively, but in that case it would definitely be less than item_stacking_axis_contribution, and we would return false because there's not path that can fit the item.

Line 162, Patchset 15: const auto& masonry_item_span_ref =
Alison Maher . resolved

nit: I'd use the actual value here since it will be easier to look up in code search later

Celeste Pan

Done

Line 162, Patchset 15: const auto& masonry_item_span_ref =
Alison Maher . resolved

nit: ref not needed in the name

Celeste Pan

Done

Line 162, Patchset 15: const auto& masonry_item_span_ref =
Kurt Catti-Schmidt . resolved

a `GridSpan` is super light-weight, so you probably don't need a reference here.

Celeste Pan

Done

Line 185, Patchset 15: item_span.Translate(1)) {
Kurt Catti-Schmidt . resolved

Might be worth adding a `operator++` to `GridSpan` to make this cleaner.

Celeste Pan

Done

Line 186, Patchset 15: // If the item spans N tracks, only check for equivalent total track
Alison Maher . resolved

As mentioned in another comment, I don't think we can rely on the total here

Celeste Pan

Discussed offline; we think based on the CSSWG discussions that the current behavior, ie using the same number of tracks with the same total track size, should be fine, but will confirm with ian later.

Line 188, Patchset 15: auto current_track = item_span.StartLine();
if (CalculateUsedTrackSize(item_span) == used_track_size) {
Kurt Catti-Schmidt . resolved

A general comment on this algorithm - it would really help to add comments with verbatims from the masonry placement spec - https://www.w3.org/TR/css-grid-3/#running-position

e.g.

```
// "Repeat the previous step for each successive line number until the item would no longer fit inside the grid."
```

(it can also help to use the same variable names as the spec does)

Celeste Pan

Okay, I did my best to map to similar language to the spec! But since there isn't a 1:1 mapping with the placement algorithm (I think the dense-packing search algorithm is pretty different from the placement algorithm) I couldn't quite pull everything from the spec.

Line 189, Patchset 15: if (CalculateUsedTrackSize(item_span) == used_track_size) {
Alison Maher . resolved

nit: I'd swap the polarity of this and do:

```
if (CalculateUsedTrackSize(item_span) != used_track_size) {
continue;
}
```

That way we don't need all the extra indentation

Celeste Pan

Done

Line 195, Patchset 15: track_collection_openings_[current_track];
Alison Maher . resolved

I am curious, with how this is currently implemented, will we be checking every opening combination across tracks?

i.e. if track 1 has two openeings that overlap with one larger opening in track 2, but track 3 only overlaps the second opening in track one and the single opening in track two. Will we find this case? Trying to figure out where that iteration is happening

Celeste Pan

The loop here is choosing which track we want to start iterating through.

For each track, we then call `AccumulateTrackOpeningsToAccomodateItem`, which:
1. For each track opening:
a. iterates recursively to find a possible path
b. if no possible path is found, move to start the path using the next track
opening in the current track.

So the actual path-finding only starts in `AccumulateTrackOpeningsToAccomodateItem`.

The check highlighted here was just to save time by skipping iteration on the track items at all if we already know the current track is empty/the track openings aren't going to be higher than a path of track openings we already have anyways.

Line 197, Patchset 15: (highest_eligible_track_opening_result.IsValid() &&
Alison Maher . resolved

We don't seem to set this before the first time - is it valid by default?

Celeste Pan

It's invalid by default (the default value of start_position is LayoutUnit::Max())!

Line 198, Patchset 15: current_track_openings[0].start_position >=
highest_eligible_track_opening_result.start_position)) {
Alison Maher . resolved

To confirm, this is saying that if the current start track has openings that are below the highest eligible start, we will never overlap?

I'm kind of confused as to where highest_eligible_track_opening_result first gets set

Celeste Pan

This is saying that if our current track's first opening is going to be higher than our existing highest track opening, then there's no reason for us to start looking for path of track openings starting from this track, since it's going to be too low to add anyways. So we skip even trying to run `AccumulateTrackOpeningsToAccomodateItem` and move onto the next track.

The first instance highest_eligible_track_opening_result gets set is after we've run `AccumulateTrackOpeningsToAccomodateItem` for first time and successfully found a path of eligible track openings.

Line 213, Patchset 15: eligible_track_opening_result);
Alison Maher . resolved

Will this always be "invalid" when we return false? If so, maybe that shoul be the return type instead of bool

Celeste Pan

Okay, I'll have the method return whether or not it's valid then!

Line 214, Patchset 15: if (found_eligible_track_opening_span &&
(!highest_eligible_track_opening_result.IsValid() ||
eligible_track_opening_result.start_position <
highest_eligible_track_opening_result.start_position)) {
Alison Maher . resolved

This check is complex and can probably use a comment

Celeste Pan

Done

Line 222, Patchset 15: }
Alison Maher . resolved

I know we had discussed that do-while was advised against previously, but given how complex this is getting, I actually think it will be easier to reason about in a do-while loop

Celeste Pan

Done!

Line 240, Patchset 15: if (!highest_eligible_opening_span.IsTranslatedDefinite()) {
Kurt Catti-Schmidt . resolved

This could use a comment - when is this not true?

Celeste Pan

this ended up being removed, so resolving this comment.

Line 257, Patchset 15: const wtf_size_t current_track_index =
eligible_track_opening_positions.track_index;
const wtf_size_t current_opening_index =
eligible_track_opening_positions.opening_index;
const TrackOpening eligible_track_opening =
track_collection_openings_[current_track_index][current_opening_index];
if (item_stacking_axis_contribution == eligible_track_opening.Size()) {
track_collection_openings_[current_track_index].EraseAt(
current_opening_index);
Alison Maher . resolved

Isn't it possible for a track opening to be split in two when it comes to the spanning case?

Celeste Pan

Yes that's totally possible, added a test case for this + have fixed this issue!

File third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-003-ref.html
Line 6, Patchset 15: display: grid;
background: gray;
position: relative;
grid-template-columns: repeat(3, 50px);
width: 170px;
grid-auto-flow: dense;
}
Kurt Catti-Schmidt . resolved

Would this test case be easier if the ref is identical but doesn't specify dense packing?

Celeste Pan

Good idea, will change it to that!

File third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-003.html
Line 5, Patchset 15:-ref.html">
Kurt Catti-Schmidt . resolved

This added newline looks unintentional

Celeste Pan

Done

Line 18, Patchset 15: <p>Ensure that dense-packing does not affect the layout when all items are single-spanning.</p>
Kurt Catti-Schmidt . resolved

I would phrase this as "...has no impact when all items..."

Celeste Pan

Done

File third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-004.html
Line 17, Patchset 15: <p>Ensure dense-packing only lays items into gaps where the track size is the same as where the item would have been laid out WITHOUT dense packing.</p>
Kurt Catti-Schmidt . resolved

lays -> places

Celeste Pan

Done

File third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-001.html
Line 15, Patchset 15: <p>Ensure that dense-packing places multi-spanning item into highest eligible track opening. The yellow item is the item that should be placed in track opening.</p>
Kurt Catti-Schmidt . resolved

This can go away

Celeste Pan

Done

Line 15, Patchset 15: <p>Ensure that dense-packing places multi-spanning item into highest eligible track opening. The yellow item is the item that should be placed in track opening.</p>
Kurt Catti-Schmidt . resolved

"...in the track..."?

Celeste Pan

I mean track opening, since it's the item that because of dense-packing will go into a previous track opening.

File third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-002.html
Line 15, Patchset 15: <p>Ensure that dense-packing with multi-span items are placed into tracks with the same used size and the same number of tracks, and priority is given to tracks closest after the auto-placement cursor.</p>
Kurt Catti-Schmidt . resolved

This is a little awkward, how about "closest (but after)"?

Celeste Pan

Done

File third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-004.html
Line 15, Patchset 15: <p>Ensure that dense-packing with multi-span items can find the right path of aligning gaps when there are multiple gaps in a track.</p>
Kurt Catti-Schmidt . resolved

This could be rephrased as "follows the masonry dense packing algorithm"

Celeste Pan

Done

File third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-003-ref.html
Line 3, Patchset 15:<link rel="help" href="https://drafts.csswg.org/css-grid-3">
Kurt Catti-Schmidt . resolved

I don't think ref files typically have help links

Celeste Pan

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Alison Maher
  • Kurt Catti-Schmidt
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I102f6d22562557744d207b7c8f52f4be699f82d4
Gerrit-Change-Number: 6909814
Gerrit-PatchSet: 16
Gerrit-Owner: Celeste Pan <celes...@microsoft.com>
Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
Gerrit-Reviewer: Celeste Pan <celes...@microsoft.com>
Gerrit-Reviewer: Kurt Catti-Schmidt <ksc...@microsoft.com>
Gerrit-CC: Hoch Hochkeppel <mho...@microsoft.com>
Gerrit-CC: Ian Kilpatrick <ikilp...@chromium.org>
Gerrit-CC: Javier Fernandez <jfern...@igalia.com>
Gerrit-CC: Yanling Wang <yanli...@microsoft.com>
Gerrit-Attention: Alison Maher <alm...@microsoft.com>
Gerrit-Attention: Kurt Catti-Schmidt <ksc...@microsoft.com>
Gerrit-Comment-Date: Tue, 16 Sep 2025 01:06:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Alison Maher <alm...@microsoft.com>
Comment-In-Reply-To: Kurt Catti-Schmidt <ksc...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Alison Maher (Gerrit)

unread,
Sep 16, 2025, 11:58:34 AMSep 16
to Celeste Pan, Kurt Catti-Schmidt, Ian Kilpatrick, Yanling Wang, Hoch Hochkeppel, Javier Fernandez, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org
Attention needed from Celeste Pan and Kurt Catti-Schmidt

Alison Maher added 14 comments

File third_party/blink/renderer/core/layout/masonry/masonry_running_positions.h
Line 189, Patchset 16 (Latest): bool AccumulateTrackOpeningsToAccomodateItem(
Alison Maher . unresolved

Instead of a bool, would it make sense to return EligibleTrackOpeningPath? If not, then it may be fine to just leave is as void and then utilize EligibleTrackOpeningPath as whether it was valid.

If we do need the bool, too, then would be worth noting what is being returned in the method comment

Line 145, Patchset 16 (Latest): // vector of openings. `start_position` refers to the highest possible
Alison Maher . unresolved

nit: extra space

File third_party/blink/renderer/core/layout/masonry/masonry_running_positions.cc
Line 90, Patchset 16 (Latest):LayoutUnit MasonryRunningPositions::CalculateUsedTrackSize(
Alison Maher . unresolved

This method likely makes more sense in GridSpan in case it ends up being useful in other scenarios at some point

Line 142, Patchset 16 (Latest): eligible_track_opening_result.track_opening_indices.emplace_back(i);
Alison Maher . unresolved

Will this be emplaced back in reverse order given this is recursive? I see this called out later - may be worth documenting that somewhere

Line 166, Patchset 16 (Latest): GridSpan highest_eligible_opening_span = GridSpan::IndefiniteGridSpan();
Alison Maher . unresolved

This one could likely use a comment

Line 182, Patchset 16 (Latest): if (item_span.EndLine() > running_positions_.size()) {
Alison Maher . unresolved

I'd add a comment here to note we are looping back to the start because the starting cursor did not start at index 0

Line 193, Patchset 16 (Latest): // far, we won't want any result that comes from this track.
Alison Maher . unresolved

"end up finding any better results that start with this track"

Line 210, Patchset 16 (Latest): // Starting at `current_track`, find a
Alison Maher . unresolved

nit: I'd add an extra line before this comment

This comment also looks like it can be re-wrapped

Line 211, Patchset 16 (Latest): // series of adjacent track openings that the item could could into if
Alison Maher . unresolved

nit: double "could"

Also missing a verb (maybe "be placed into")

Line 211, Patchset 16 (Latest): // series of adjacent track openings that the item could could into if
// placed at this line. If there is not previous result for the highest
Alison Maher . unresolved

"starting at this line"

Line 227, Patchset 16 (Latest): } while (iterations <= max_iterations);
Alison Maher . unresolved

Instead of tracking iterations, is it possible to update as suggested in https://chromium-review.googlesource.com/c/chromium/src/+/6819879/8..28/third_party/blink/renderer/core/layout/masonry/masonry_running_positions.cc#b177?

If not, I think where we set max_iterations could use a comment

Line 236, Patchset 16 (Latest): highest_eligible_track_opening_result.track_opening_indices.Reverse();
Alison Maher . unresolved

Can we just iterate through them in the opposite direction instead?

Another option is to make the openings vector always be the size of the span needed and that way we can place these in the vector at the correct index as we recurse instead of emplacing back

Line 248, Patchset 16 (Latest): // If the item causes a split for the opening, create a new track opening
Alison Maher . unresolved

nit: "an opening to split"

Line 267, Patchset 16 (Latest): if (highest_eligible_opening_span.IsTranslatedDefinite()) {
Alison Maher . unresolved

Can this ever not be the case? If so, what happens?

Open in Gerrit

Related details

Attention is currently required from:
  • Celeste Pan
  • Kurt Catti-Schmidt
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I102f6d22562557744d207b7c8f52f4be699f82d4
    Gerrit-Change-Number: 6909814
    Gerrit-PatchSet: 16
    Gerrit-Owner: Celeste Pan <celes...@microsoft.com>
    Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
    Gerrit-Reviewer: Celeste Pan <celes...@microsoft.com>
    Gerrit-Reviewer: Kurt Catti-Schmidt <ksc...@microsoft.com>
    Gerrit-CC: Hoch Hochkeppel <mho...@microsoft.com>
    Gerrit-CC: Ian Kilpatrick <ikilp...@chromium.org>
    Gerrit-CC: Javier Fernandez <jfern...@igalia.com>
    Gerrit-CC: Yanling Wang <yanli...@microsoft.com>
    Gerrit-Attention: Kurt Catti-Schmidt <ksc...@microsoft.com>
    Gerrit-Attention: Celeste Pan <celes...@microsoft.com>
    Gerrit-Comment-Date: Tue, 16 Sep 2025 15:58:26 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Celeste Pan (Gerrit)

    unread,
    Sep 16, 2025, 7:48:52 PM (14 days ago) Sep 16
    to Alison Maher, Kurt Catti-Schmidt, Ian Kilpatrick, Yanling Wang, Hoch Hochkeppel, Javier Fernandez, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org
    Attention needed from Alison Maher and Kurt Catti-Schmidt

    Celeste Pan added 14 comments

    File third_party/blink/renderer/core/layout/masonry/masonry_running_positions.h
    Line 189, Patchset 16: bool AccumulateTrackOpeningsToAccomodateItem(
    Alison Maher . resolved

    Instead of a bool, would it make sense to return EligibleTrackOpeningPath? If not, then it may be fine to just leave is as void and then utilize EligibleTrackOpeningPath as whether it was valid.

    If we do need the bool, too, then would be worth noting what is being returned in the method comment

    Celeste Pan

    I do have an existing comment line 88, "This method returns whether or not a path of eligible track openings were found."

    The code is definitely cleaner with a boolean + the `eligible_track_opening_result` out param. If we were to return EligibleTrackOpeningPath, we'd have to have 2 separate conditionals on line 134, since we'd check for the number of tracks first, and only run `AccumulateTrackOpeningsToAccomodateItem` if we need to. The two conditions would have us performing the same action, so it would also lead to some code repeating.

    Line 145, Patchset 16: // vector of openings. `start_position` refers to the highest possible
    Alison Maher . resolved

    nit: extra space

    Celeste Pan

    Hm, I think there's only one space here, so resolving!

    File third_party/blink/renderer/core/layout/masonry/masonry_running_positions.cc
    Line 90, Patchset 16:LayoutUnit MasonryRunningPositions::CalculateUsedTrackSize(
    Alison Maher . resolved

    This method likely makes more sense in GridSpan in case it ends up being useful in other scenarios at some point

    Celeste Pan

    At the moment we could only do this in GridSpan if we pass in `track_collection_sizes_`, since GridSpan doesn't have access to the sizes of the tracks it spans.

    I think it would make sense to have something like this where it can take in a GridLayoutTrackCollection and figure out the used size of the spans; but until we actually implement the ability to get individual track sizes in GridLayoutCollection, it wouldn't make sense to have something like this in GridSpan yet.

    I'll add a TODO for this!

    Line 142, Patchset 16: eligible_track_opening_result.track_opening_indices.emplace_back(i);
    Alison Maher . resolved

    Will this be emplaced back in reverse order given this is recursive? I see this called out later - may be worth documenting that somewhere

    Celeste Pan

    I'll document this in the method description!

    Line 166, Patchset 16: GridSpan highest_eligible_opening_span = GridSpan::IndefiniteGridSpan();
    Alison Maher . resolved

    This one could likely use a comment

    Celeste Pan

    Done

    Line 182, Patchset 16: if (item_span.EndLine() > running_positions_.size()) {
    Alison Maher . resolved

    I'd add a comment here to note we are looping back to the start because the starting cursor did not start at index 0

    Celeste Pan

    I feel like this is covered in the comment in lines 171-174, so it might be repetitive to say again. Will resolve for now, but let me know if you feel strongly that a comment should be here as well!

    Line 193, Patchset 16: // far, we won't want any result that comes from this track.
    Alison Maher . resolved

    "end up finding any better results that start with this track"

    Celeste Pan

    Done

    Line 210, Patchset 16: // Starting at `current_track`, find a
    Alison Maher . resolved

    nit: I'd add an extra line before this comment

    This comment also looks like it can be re-wrapped

    Celeste Pan

    Done

    Line 211, Patchset 16: // series of adjacent track openings that the item could could into if
    Alison Maher . resolved

    nit: double "could"

    Also missing a verb (maybe "be placed into")

    Celeste Pan

    Done

    Line 211, Patchset 16: // series of adjacent track openings that the item could could into if

    // placed at this line. If there is not previous result for the highest
    Alison Maher . resolved

    "starting at this line"

    Celeste Pan

    Done

    Line 227, Patchset 16: } while (iterations <= max_iterations);
    Alison Maher . resolved

    Instead of tracking iterations, is it possible to update as suggested in https://chromium-review.googlesource.com/c/chromium/src/+/6819879/8..28/third_party/blink/renderer/core/layout/masonry/masonry_running_positions.cc#b177?

    If not, I think where we set max_iterations could use a comment

    Celeste Pan

    This was originally the intention, but it ended up becoming a lot more complex, and we end up having to keep track of other variables for breaking the loop; this ended up being the simplest solution.

    I'll add a comment for where we're setting `max_iterations`!

    Line 236, Patchset 16: highest_eligible_track_opening_result.track_opening_indices.Reverse();
    Alison Maher . resolved

    Can we just iterate through them in the opposite direction instead?

    Another option is to make the openings vector always be the size of the span needed and that way we can place these in the vector at the correct index as we recurse instead of emplacing back

    Celeste Pan

    Yup, I can just change this to iterate in the opposite direction!

    Line 248, Patchset 16: // If the item causes a split for the opening, create a new track opening
    Alison Maher . resolved

    nit: "an opening to split"

    Celeste Pan

    Done

    Line 267, Patchset 16: if (highest_eligible_opening_span.IsTranslatedDefinite()) {
    Alison Maher . resolved

    Can this ever not be the case? If so, what happens?

    Celeste Pan

    Ended up deleting this in favor of just creating a span using the highest_eligible_track_opening_result, so comment no longer relevant!

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Alison Maher
    • Kurt Catti-Schmidt
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I102f6d22562557744d207b7c8f52f4be699f82d4
    Gerrit-Change-Number: 6909814
    Gerrit-PatchSet: 17
    Gerrit-Owner: Celeste Pan <celes...@microsoft.com>
    Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
    Gerrit-Reviewer: Celeste Pan <celes...@microsoft.com>
    Gerrit-Reviewer: Kurt Catti-Schmidt <ksc...@microsoft.com>
    Gerrit-CC: Hoch Hochkeppel <mho...@microsoft.com>
    Gerrit-CC: Ian Kilpatrick <ikilp...@chromium.org>
    Gerrit-CC: Javier Fernandez <jfern...@igalia.com>
    Gerrit-CC: Yanling Wang <yanli...@microsoft.com>
    Gerrit-Attention: Alison Maher <alm...@microsoft.com>
    Gerrit-Attention: Kurt Catti-Schmidt <ksc...@microsoft.com>
    Gerrit-Comment-Date: Tue, 16 Sep 2025 23:48:42 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Alison Maher <alm...@microsoft.com>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Alison Maher (Gerrit)

    unread,
    Sep 17, 2025, 11:56:41 AM (13 days ago) Sep 17
    to Celeste Pan, Kurt Catti-Schmidt, Ian Kilpatrick, Yanling Wang, Hoch Hochkeppel, Javier Fernandez, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org
    Attention needed from Celeste Pan and Kurt Catti-Schmidt

    Alison Maher voted and added 1 comment

    Votes added by Alison Maher

    Code-Review+1

    1 comment

    Patchset-level comments
    File-level comment, Patchset 17 (Latest):
    Alison Maher . resolved

    LGTM, but it looks like there is a test failure with the recent upload, so if I need to take a look another look after the fix is uploaded, feel free to lmk

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Celeste Pan
    • Kurt Catti-Schmidt
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Owners
    • requirement satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I102f6d22562557744d207b7c8f52f4be699f82d4
    Gerrit-Change-Number: 6909814
    Gerrit-PatchSet: 17
    Gerrit-Owner: Celeste Pan <celes...@microsoft.com>
    Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
    Gerrit-Reviewer: Celeste Pan <celes...@microsoft.com>
    Gerrit-Reviewer: Kurt Catti-Schmidt <ksc...@microsoft.com>
    Gerrit-CC: Hoch Hochkeppel <mho...@microsoft.com>
    Gerrit-CC: Ian Kilpatrick <ikilp...@chromium.org>
    Gerrit-CC: Javier Fernandez <jfern...@igalia.com>
    Gerrit-CC: Yanling Wang <yanli...@microsoft.com>
    Gerrit-Attention: Celeste Pan <celes...@microsoft.com>
    Gerrit-Attention: Kurt Catti-Schmidt <ksc...@microsoft.com>
    Gerrit-Comment-Date: Wed, 17 Sep 2025 15:56:32 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    open
    diffy

    Kurt Catti-Schmidt (Gerrit)

    unread,
    Sep 17, 2025, 12:39:05 PM (13 days ago) Sep 17
    to Celeste Pan, Alison Maher, Ian Kilpatrick, Yanling Wang, Hoch Hochkeppel, Javier Fernandez, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org
    Attention needed from Celeste Pan

    Kurt Catti-Schmidt added 6 comments

    Patchset-level comments
    Kurt Catti-Schmidt . resolved

    Looks great, just some minor issues.

    File third_party/blink/renderer/core/layout/masonry/masonry_running_positions.h
    Line 188, Patchset 17 (Latest): LayoutUnit CalculateUsedTrackSize(const GridSpan& span);
    Kurt Catti-Schmidt . unresolved

    Can this be `const` - e.g. `LayoutUnit CalculateUsedTrackSize(const GridSpan& span) const;`?

    File third_party/blink/renderer/core/layout/masonry/masonry_running_positions.cc
    Line 110, Patchset 17 (Latest): const Vector<TrackOpening> current_track_openings =
    Kurt Catti-Schmidt . unresolved

    Can this be a reference? e.g. `const Vector<TrackOpening>&`? That'll avoid a vector copy.

    Line 140, Patchset 17 (Latest): num_tracks_remaining - 1, track_to_check_for_openings + 1,
    Kurt Catti-Schmidt . unresolved

    If `num_tracks_remaining == 0`, this will underflow - this needs to be handled somewhere.

    Line 269, Patchset 17 (Latest): --current_track_index;
    Kurt Catti-Schmidt . unresolved

    `DCHECK_GT(current_track_index, 0U);` to catch underflows

    File third_party/blink/renderer/core/style/grid_area.h
    Line 86, Patchset 17 (Latest): GridSpan& operator++() {
    Kurt Catti-Schmidt . unresolved

    `DCHECK(IsTranslatedDefinite());`

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Celeste Pan
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I102f6d22562557744d207b7c8f52f4be699f82d4
      Gerrit-Change-Number: 6909814
      Gerrit-PatchSet: 17
      Gerrit-Owner: Celeste Pan <celes...@microsoft.com>
      Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
      Gerrit-Reviewer: Celeste Pan <celes...@microsoft.com>
      Gerrit-Reviewer: Kurt Catti-Schmidt <ksc...@microsoft.com>
      Gerrit-CC: Hoch Hochkeppel <mho...@microsoft.com>
      Gerrit-CC: Ian Kilpatrick <ikilp...@chromium.org>
      Gerrit-CC: Javier Fernandez <jfern...@igalia.com>
      Gerrit-CC: Yanling Wang <yanli...@microsoft.com>
      Gerrit-Attention: Celeste Pan <celes...@microsoft.com>
      Gerrit-Comment-Date: Wed, 17 Sep 2025 16:38:54 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Celeste Pan (Gerrit)

      unread,
      Sep 17, 2025, 4:38:22 PM (13 days ago) Sep 17
      to Alison Maher, Kurt Catti-Schmidt, Ian Kilpatrick, Yanling Wang, Hoch Hochkeppel, Javier Fernandez, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org
      Attention needed from Kurt Catti-Schmidt

      Celeste Pan added 5 comments

      File third_party/blink/renderer/core/layout/masonry/masonry_running_positions.h
      Line 188, Patchset 17: LayoutUnit CalculateUsedTrackSize(const GridSpan& span);
      Kurt Catti-Schmidt . resolved

      Can this be `const` - e.g. `LayoutUnit CalculateUsedTrackSize(const GridSpan& span) const;`?

      Celeste Pan

      Done

      File third_party/blink/renderer/core/layout/masonry/masonry_running_positions.cc
      Line 110, Patchset 17: const Vector<TrackOpening> current_track_openings =
      Kurt Catti-Schmidt . resolved

      Can this be a reference? e.g. `const Vector<TrackOpening>&`? That'll avoid a vector copy.

      Celeste Pan

      Done

      Line 140, Patchset 17: num_tracks_remaining - 1, track_to_check_for_openings + 1,
      Kurt Catti-Schmidt . resolved

      If `num_tracks_remaining == 0`, this will underflow - this needs to be handled somewhere.

      Celeste Pan

      This case would be covered by the first part of the conditional, since if `num_track_remaining == 0`, we would skip the run of `AccumulateTrackOpeningsToAccomodateItem`.

      Line 269, Patchset 17: --current_track_index;
      Kurt Catti-Schmidt . resolved

      `DCHECK_GT(current_track_index, 0U);` to catch underflows

      Celeste Pan

      Can't do this since at some point `current_track_index` will be 0; I added `DCHECK_GE(current_track_index, track_opening_indices.size());` before the beginning of the loop moved the subtraction to the beginning of the while loop, and this should cover the case of underflow.

      File third_party/blink/renderer/core/style/grid_area.h
      Line 86, Patchset 17: GridSpan& operator++() {
      Kurt Catti-Schmidt . resolved

      `DCHECK(IsTranslatedDefinite());`

      Celeste Pan

      Done

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Kurt Catti-Schmidt
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement satisfiedCode-Review
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I102f6d22562557744d207b7c8f52f4be699f82d4
      Gerrit-Change-Number: 6909814
      Gerrit-PatchSet: 19
      Gerrit-Owner: Celeste Pan <celes...@microsoft.com>
      Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
      Gerrit-Reviewer: Celeste Pan <celes...@microsoft.com>
      Gerrit-Reviewer: Kurt Catti-Schmidt <ksc...@microsoft.com>
      Gerrit-CC: Hoch Hochkeppel <mho...@microsoft.com>
      Gerrit-CC: Ian Kilpatrick <ikilp...@chromium.org>
      Gerrit-CC: Javier Fernandez <jfern...@igalia.com>
      Gerrit-CC: Yanling Wang <yanli...@microsoft.com>
      Gerrit-Attention: Kurt Catti-Schmidt <ksc...@microsoft.com>
      Gerrit-Comment-Date: Wed, 17 Sep 2025 20:38:12 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: Kurt Catti-Schmidt <ksc...@microsoft.com>
      satisfied_requirement
      open
      diffy

      Celeste Pan (Gerrit)

      unread,
      Sep 17, 2025, 4:50:05 PM (13 days ago) Sep 17
      to Alison Maher, Kurt Catti-Schmidt, Ian Kilpatrick, Yanling Wang, Hoch Hochkeppel, Javier Fernandez, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org
      Attention needed from Kurt Catti-Schmidt

      Celeste Pan voted Commit-Queue+2

      Commit-Queue+2
      Gerrit-Comment-Date: Wed, 17 Sep 2025 20:49:56 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      open
      diffy

      Kurt Catti-Schmidt (Gerrit)

      unread,
      Sep 17, 2025, 4:57:38 PM (13 days ago) Sep 17
      to Celeste Pan, Alison Maher, Ian Kilpatrick, Yanling Wang, Hoch Hochkeppel, Javier Fernandez, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org
      Attention needed from Celeste Pan

      Kurt Catti-Schmidt voted Code-Review+1

      Code-Review+1
      Open in Gerrit

      Related details

      Attention is currently required from:
      • Celeste Pan
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement satisfiedCode-Review
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I102f6d22562557744d207b7c8f52f4be699f82d4
      Gerrit-Change-Number: 6909814
      Gerrit-PatchSet: 19
      Gerrit-Owner: Celeste Pan <celes...@microsoft.com>
      Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
      Gerrit-Reviewer: Celeste Pan <celes...@microsoft.com>
      Gerrit-Reviewer: Kurt Catti-Schmidt <ksc...@microsoft.com>
      Gerrit-CC: Hoch Hochkeppel <mho...@microsoft.com>
      Gerrit-CC: Ian Kilpatrick <ikilp...@chromium.org>
      Gerrit-CC: Javier Fernandez <jfern...@igalia.com>
      Gerrit-CC: Yanling Wang <yanli...@microsoft.com>
      Gerrit-Attention: Celeste Pan <celes...@microsoft.com>
      Gerrit-Comment-Date: Wed, 17 Sep 2025 20:57:29 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      open
      diffy

      Celeste Pan (Gerrit)

      unread,
      Sep 17, 2025, 9:35:15 PM (13 days ago) Sep 17
      to Kurt Catti-Schmidt, Alison Maher, Ian Kilpatrick, Yanling Wang, Hoch Hochkeppel, Javier Fernandez, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org

      Celeste Pan voted Commit-Queue+2

      Commit-Queue+2
      Open in Gerrit

      Related details

      Attention set is empty
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement satisfiedCode-Review
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I102f6d22562557744d207b7c8f52f4be699f82d4
      Gerrit-Change-Number: 6909814
      Gerrit-PatchSet: 20
      Gerrit-Owner: Celeste Pan <celes...@microsoft.com>
      Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
      Gerrit-Reviewer: Celeste Pan <celes...@microsoft.com>
      Gerrit-Reviewer: Kurt Catti-Schmidt <ksc...@microsoft.com>
      Gerrit-CC: Hoch Hochkeppel <mho...@microsoft.com>
      Gerrit-CC: Ian Kilpatrick <ikilp...@chromium.org>
      Gerrit-CC: Javier Fernandez <jfern...@igalia.com>
      Gerrit-CC: Yanling Wang <yanli...@microsoft.com>
      Gerrit-Comment-Date: Thu, 18 Sep 2025 01:35:05 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      open
      diffy

      Celeste Pan (Gerrit)

      unread,
      Sep 17, 2025, 9:37:39 PM (13 days ago) Sep 17
      to Kurt Catti-Schmidt, Alison Maher, Ian Kilpatrick, Yanling Wang, Hoch Hochkeppel, Javier Fernandez, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org

      Celeste Pan voted Commit-Queue+2

      Commit-Queue+2
      Open in Gerrit

      Related details

      Attention set is empty
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement satisfiedCode-Review
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I102f6d22562557744d207b7c8f52f4be699f82d4
      Gerrit-Change-Number: 6909814
      Gerrit-PatchSet: 21
      Gerrit-Owner: Celeste Pan <celes...@microsoft.com>
      Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
      Gerrit-Reviewer: Celeste Pan <celes...@microsoft.com>
      Gerrit-Reviewer: Kurt Catti-Schmidt <ksc...@microsoft.com>
      Gerrit-CC: Hoch Hochkeppel <mho...@microsoft.com>
      Gerrit-CC: Ian Kilpatrick <ikilp...@chromium.org>
      Gerrit-CC: Javier Fernandez <jfern...@igalia.com>
      Gerrit-CC: Yanling Wang <yanli...@microsoft.com>
      Gerrit-Comment-Date: Thu, 18 Sep 2025 01:37:29 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      open
      diffy

      Chromium LUCI CQ (Gerrit)

      unread,
      Sep 17, 2025, 10:47:44 PM (13 days ago) Sep 17
      to Celeste Pan, Kurt Catti-Schmidt, Alison Maher, Ian Kilpatrick, Yanling Wang, Hoch Hochkeppel, Javier Fernandez, AyeAye, chromium...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org

      Chromium LUCI CQ submitted the change with unreviewed changes

      Unreviewed changes

      19 is the latest approved patch-set.
      The change was submitted with unreviewed changes in the following files:

      ```
      The name of the file: third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-multi-span-002.html
      Insertions: 0, Deletions: 2.

      @@ -3,7 +3,6 @@

      <link rel="help" href="https://drafts.csswg.org/css-grid-3">
       <link rel="match" href="row-dense-packing-multi-span-002-ref.html">
      <link rel="author" title="Celeste Pan" href="mailto:celes...@microsoft.com">
      -<link rel="stylesheet" href="/fonts/ahem.css">
      <style>
      .masonry {
      display: masonry;
      @@ -11,7 +10,6 @@
      masonry-direction: row;
      grid-template-rows: 10px 10px 20px 15px 5px;
      grid-auto-flow: dense;
      - font: 5px/1 "Ahem";
      }
      </style>
      <body>
      ```
      ```
      The name of the file: third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-multi-span-002-ref.html
      Insertions: 10, Deletions: 16.

      @@ -1,12 +1,10 @@
      <!DOCTYPE html>
      <html>
      -<link rel="stylesheet" href="/fonts/ahem.css">
      <style>
      .grid {
      display: grid;
      grid-template-rows: 10px 10px 20px 15px 5px;
      grid-auto-flow: dense;
      - font: 5px/1 "Ahem";
      }
      .flex {
      display: flex;
      @@ -17,22 +15,18 @@

      <p>Ensure that dense-packing with multi-span items are placed into tracks with the same used size and the same number of tracks, and priority is given to tracks closest after the auto-placement cursor.</p>
         <div class="grid">
      <div class="flex">
      - <div style="display: flex; flex-direction: column; transform: translateY(20px);">
      - <div style="background: lightskyblue; width: 20px; height: 40px;" >
      - 1
      - </div>
      - <div style="background: yellow; width: 20px; height: 20px;" >
      - 4
      - </div>
      + <div style="background: lightskyblue; width: 20px; height: 20px; transform: translateY(20px);" >
      + 1
      </div>
      - <div class="flex">
      - <div style="background: lightblue; width: 20px; height: 60px;" >
      - 2
      - </div>
      - <div style="background: darkblue; width: 20px; height: 40px;">
      - 3
      - </div>
      + <div style="background: lightblue; width: 20px; height: 60px;" >
      + 2
      </div>
      + <div style="background: darkblue; width: 20px; height: 40px;">
      + 3
      + </div>
      + </div>
      + <div style="background: yellow; width: 20px; height: 20px; transform: translateY(30px);" >
      + 4
      </div>
      </div>
      </body>
      ```

      Change information

      Commit message:
      [Masonry] Implement dense-packing for multi-spanning items

      This change allows masonry to use dense-packing on multi-spanning items.
      Items that span N tracks must be laid out across N tracks, regardless of
      whether or not it is going into a track opening or not.

      The rescursive method `FindPathOfTrackOpeningsToAccomodateItem` performs
      backtracking search for a list of consecutive track openings that can
      contain the item with the given height.

      The only changes in the dense-packing single-spanning tests are renames,
      as the contents of [row|column-dense-packing-003] were moved into
      [row|column]-dense-packing-multi-span-003.html.
      Bug: 343257585
      Change-Id: I102f6d22562557744d207b7c8f52f4be699f82d4
      Reviewed-by: Kurt Catti-Schmidt <ksc...@microsoft.com>
      Commit-Queue: Celeste Pan <celes...@microsoft.com>
      Reviewed-by: Alison Maher <alm...@microsoft.com>
      Cr-Commit-Position: refs/heads/main@{#1517023}
      Files:
      • M third_party/blink/renderer/core/layout/masonry/masonry_running_positions.cc
      • M third_party/blink/renderer/core/layout/masonry/masonry_running_positions.h
      • M third_party/blink/renderer/core/style/grid_area.h
      • M third_party/blink/web_tests/TestExpectations
      • M third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-003-ref.html
      • M third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-003.html
      • M third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-004-ref.html
      • M third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-004.html
      • M third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-005-ref.html
      • M third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-005.html
      • D third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-006-ref.html
      • D third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-006.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-001-ref.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-001.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-002-ref.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-002.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-003-ref.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-003.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-004-ref.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-004.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-005-ref.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-005.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-006-ref.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-006.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-007-ref.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-007.html
      • M third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-003-ref.html
      • M third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-003.html
      • D third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-004-ref.html
      • D third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-004.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-multi-span-001-ref.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-multi-span-001.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-multi-span-002-ref.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-multi-span-002.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-multi-span-003-ref.html
      • A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-multi-span-003.html
      Change size: XL
      Delta: 36 files changed, 949 insertions(+), 393 deletions(-)
      Branch: refs/heads/main
      Submit Requirements:
      • requirement satisfiedCode-Review: +1 by Kurt Catti-Schmidt, +1 by Alison Maher
      Open in Gerrit
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: merged
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I102f6d22562557744d207b7c8f52f4be699f82d4
      Gerrit-Change-Number: 6909814
      Gerrit-PatchSet: 22
      Gerrit-Owner: Celeste Pan <celes...@microsoft.com>
      Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
      Gerrit-Reviewer: Celeste Pan <celes...@microsoft.com>
      Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
      Gerrit-Reviewer: Kurt Catti-Schmidt <ksc...@microsoft.com>
      open
      diffy
      satisfied_requirement
      Reply all
      Reply to author
      Forward
      0 new messages