[Gap Decorations]: Major paint logic for Optimized GapGeometry [chromium/src : main]

[Sam Davis Omekara (Gerrit)]

Attention needed from Javier Contreras and Kurt Catti-Schmidt

Sam Davis Omekara added 2 comments

Patchset-level comments

File-level comment, Patchset 7:

Sam Davis Omekara . resolved

This is done barring EdgeIntersection and Get Edge for flex cross gaps

Sam Davis Omekara

Done

File-level comment, Patchset 28 (Latest):

Sam Davis Omekara . resolved

Missing some test addition to TestExpectations but wanted to open this up for review. PTAL 😊

Open in Gerrit

Related details

Attention is currently required from:

• Javier Contreras
• Kurt Catti-Schmidt

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 28

Gerrit-Owner: Sam Davis Omekara <samome...@microsoft.com>

Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>

Gerrit-Reviewer: Kurt Catti-Schmidt <ksc...@microsoft.com>

Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>

Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>

Gerrit-CC: Akihiro Ota <akihi...@chromium.org>

Gerrit-CC: Ale Bzk <ales...@chromium.org>

Gerrit-CC: Alex Keng <shi...@microsoft.com>

Gerrit-CC: Alexis Menard <alexis...@intel.com>

Gerrit-CC: Andrew Rayskiy <green...@google.com>

Gerrit-CC: Andrew Williams <awi...@chromium.org>

Gerrit-CC: Arnaud Mandy <arnaud...@intel.com>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>

Gerrit-CC: CJ DiMeglio <lethala...@chromium.org>

Gerrit-CC: Christian Biesinger <cbies...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>

Gerrit-CC: Daniel Andersson <dande...@chromium.org>

Gerrit-CC: Daniel Cheng <dch...@chromium.org>

Gerrit-CC: David Bokan <bo...@chromium.org>

Gerrit-CC: David Worsham <dwor...@google.com>

Gerrit-CC: Dirk Schulze <dsch...@chromium.org>

Gerrit-CC: Dustin Mitchell <djmi...@chromium.org>

Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>

Gerrit-CC: Frank Liberato <libe...@chromium.org>

Gerrit-CC: Fredrik Söderquist <f...@opera.com>

Gerrit-CC: Hans Wennborg <ha...@chromium.org>

Gerrit-CC: Henrique Ferreiro <hfer...@igalia.com>

Gerrit-CC: Hirokazu Honda <hi...@chromium.org>

Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>

Gerrit-CC: Hongchan Choi <hong...@chromium.org>

Gerrit-CC: Ian Vollick <vol...@chromium.org>

Gerrit-CC: Ilya Biryukov <ibir...@google.com>

Gerrit-CC: James Maclean <wjma...@chromium.org>

Gerrit-CC: James Su <su...@chromium.org>

Gerrit-CC: Javier Fernandez <jfern...@igalia.com>

Gerrit-CC: Jerome Jiang <ji...@chromium.org>

Gerrit-CC: Jiewei Qian <q...@chromium.org>

Gerrit-CC: Kaan Icer <ic...@chromium.org>

Gerrit-CC: Kenneth R Christiansen <kenneth.r.c...@intel.com>

Gerrit-CC: Kenneth Rohde Christiansen <kenneth.ch...@gmail.com>

Gerrit-CC: Kentaro Hara <har...@chromium.org>

Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>

Gerrit-CC: Kevin McNee <mc...@chromium.org>

Gerrit-CC: Lei Zhang <the...@chromium.org>

Gerrit-CC: Mathias Bynens <mat...@chromium.org>

Gerrit-CC: Michael Moss <mm...@chromium.org>

Gerrit-CC: Michael Wilson <mjwi...@chromium.org>

Gerrit-CC: Mike Taylor <mike...@chromium.org>

Gerrit-CC: Mirko Bonadei <mbon...@chromium.org>

Gerrit-CC: Nate Chapin <jap...@chromium.org>

Gerrit-CC: Nektarios Paisios <nek...@chromium.org>

Gerrit-CC: Nico Weber <tha...@chromium.org>

Gerrit-CC: Nikhil Nayunigari <nikh...@google.com>

Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>

Gerrit-CC: Per Åhgren <pe...@chromium.org>

Gerrit-CC: Peter Beverloo <pe...@chromium.org>

Gerrit-CC: Peter Collingbourne <p...@chromium.org>

Gerrit-CC: Peter Williamson <pet...@chromium.org>

Gerrit-CC: Prashant Nevase <pne...@microsoft.com>

Gerrit-CC: Prashant Nevase <pras...@chromium.org>

Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>

Gerrit-CC: Reid Kleckner <r...@chromium.org>

Gerrit-CC: Robert Kroeger <rjkr...@chromium.org>

Gerrit-CC: Ryan Sultanem <rs...@google.com>

Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>

Gerrit-CC: Sam Zackrisson <sa...@chromium.org>

Gerrit-CC: Sarah Krakowiak <skra...@chromium.org>

Gerrit-CC: Sean Maher <sp...@chromium.org>

Gerrit-CC: Simon Hangl <sim...@google.com>

Gerrit-CC: Stephen Chenney <sche...@chromium.org>

Gerrit-CC: Steven Luong <stl...@chromium.org>

Gerrit-CC: Thiabaud Engelbrecht <thia...@google.com>

Gerrit-CC: Tommy Li <tomm...@chromium.org>

Gerrit-CC: Urvang Joshi <urv...@chromium.org>

Gerrit-CC: Victor Tan <vict...@chromium.org>

Gerrit-CC: Wanming Lin <wanmi...@intel.com>

Gerrit-CC: Wei4 Wang <wei4...@intel.com>

Gerrit-CC: Wojciech Dzierżanowski <wdzierz...@opera.com>

Gerrit-CC: Yoav Weiss (@Shopify) <yoav...@chromium.org>

Gerrit-CC: Zijie He <zij...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-CC: ningxin hu <ningx...@intel.com>

Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>

Gerrit-CC: Łukasz Bendig <lbe...@opera.com>

Gerrit-Attention: Javier Contreras <javi...@microsoft.com>

Gerrit-Attention: Kurt Catti-Schmidt <ksc...@microsoft.com>

Gerrit-Comment-Date: Mon, 25 Aug 2025 17:50:56 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Sam Davis Omekara <samome...@microsoft.com>

[Javier Contreras (Gerrit)]

Attention needed from Kurt Catti-Schmidt and Sam Davis Omekara

Javier Contreras added 7 comments

File third_party/blink/renderer/core/layout/flex/flex_layout_algorithm.cc

Line 802, Patchset 30 (Latest): LayoutUnit border_scrollbar_padding =
is_column_
? container_builder_->BorderScrollbarPadding().block_end
: container_builder_->BorderScrollbarPadding().inline_end;
content_main_end_ =
is_column_
? container_builder_->InitialBorderBoxSize().block_size -
border_scrollbar_padding
: container_builder_->InlineSize() - border_scrollbar_padding;

Javier Contreras . unresolved

I'm ok with keeping the flex changes in this CL, but let's call out in the CL description why we made these changes i.e.:

• Previously we were not setting the ContentStart/End offsets in cases where we only had one item in a flex line.
• The offset for the main gap does not need to be adjusted to inline/block depending on column, it will always be the offset of where the flex line ends + gap. (relevant test was adjusted as such.)

File third_party/blink/renderer/core/layout/gap/gap_geometry.h

Line 205, Patchset 30 (Latest): // Computes the end offset for a flex cross gap at `cross_gap_index`. The end
// offset is either:
// - The container's content end, or
// - The offset of the main gap where this cross gap ends (tracked by
// `main_gap_running_index_`)

Javier Contreras . unresolved

Thank you for detailed comments. I wish more of Blink had this.

Line 80, Patchset 30 (Latest):// Gap locations are used for painting gap decorations.

Javier Contreras . unresolved

We should link to readme here too.

File third_party/blink/renderer/core/layout/gap/gap_geometry.cc

Line 177, Patchset 30 (Latest): // 1. Cross gaps that appear before the main gap
// 2. Cross gaps that appear after the main gap

Javier Contreras . unresolved

would be good to link to the readme here, for future readers of this code, since this part can be confusing.

Line 257, Patchset 30 (Latest): // For a flex cross gap:
// - There are exactly two intersections:
// 1. The gap's start offset
// 2. Its computed end offset (either a main gap or the container's
// content-end edge)

Javier Contreras . unresolved

ditto here.

File third_party/blink/renderer/core/layout/gap/main_gap.h

Line 26, Patchset 30 (Latest):
bool HasCrossGapsBefore() const {
return range_of_cross_gaps_before_.IsValid();
}

bool HasCrossGapsAfter() const {
return range_of_cross_gaps_after_.IsValid();
}

wtf_size_t GetCrossGapBeforeStart() const {
return range_of_cross_gaps_before_.Start();
}

wtf_size_t GetCrossGapBeforeEnd() const {
return range_of_cross_gaps_before_.End();
}

wtf_size_t GetCrossGapAfterStart() const {
return range_of_cross_gaps_after_.Start();
}

wtf_size_t GetCrossGapAfterEnd() const {
return range_of_cross_gaps_after_.End();
}

void IncrementRangeOfCrossGapsBefore(wtf_size_t cross_gap_index) {
range_of_cross_gaps_before_.Increment(cross_gap_index);
}

void IncrementRangeOfCrossGapsAfter(wtf_size_t cross_gap_index) {
range_of_cross_gaps_after_.Increment(cross_gap_index);
}

void SetRangeOfCrossGapsBefore(const CrossGapRange& range) {
range_of_cross_gaps_before_ = range;
}

void SetRangeOfCrossGapsAfter(const CrossGapRange& range) {
range_of_cross_gaps_after_ = range;
}

Javier Contreras . unresolved

Call out in the description why we made these changes to the way with which we interface with the ranges.

File third_party/blink/web_tests/external/wpt/css/css-gaps/grid/grid-gap-decorations-003-ref.html

Line 1, Patchset 30 (Latest):<!DOCTYPE html>

Javier Contreras . unresolved

call out the reason for these changes in the CL description too.

Open in Gerrit

Related details

Attention is currently required from:

• Kurt Catti-Schmidt
• Sam Davis Omekara

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 30

Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>

Gerrit-Attention: Kurt Catti-Schmidt <ksc...@microsoft.com>

Gerrit-Comment-Date: Mon, 25 Aug 2025 21:58:08 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Javier Contreras (Gerrit)]

Attention needed from Kurt Catti-Schmidt and Sam Davis Omekara

Javier Contreras added 1 comment

File third_party/blink/renderer/core/paint/gap_painter.cc

Line 46, Patchset 30 (Latest):void GapPainter::Paint(GridTrackSizingDirection track_direction,

Javier Contreras . unresolved

nit: I feel like this function currently is doing too much (i feel like our previous version did too) and can be split to make it cleaner.

Foe example, we could have a separate function that handles returning the `center`, another for creating the `LogicalRect`, and maybe even the while loop itself could be split out into something like `ComputeGapSegments`

Gerrit-Comment-Date: Mon, 25 Aug 2025 22:14:33 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Kurt Catti-Schmidt (Gerrit)]

Attention needed from Sam Davis Omekara

Kurt Catti-Schmidt added 19 comments

Patchset-level comments

File-level comment, Patchset 29:

Kurt Catti-Schmidt . resolved

This is awesome, can't wait to see it land!

File third_party/blink/renderer/core/layout/gap/gap_geometry.h

Line 276, Patchset 29: mutable wtf_size_t main_gap_running_index_ = kNotFound;

Kurt Catti-Schmidt . unresolved

Can you make `ComputeEndOffsetForFlexCrossGap` non-const vs making this mutable?

Line 269, Patchset 29: GridTrackSizingDirection main_direction_ = kForRows;

Kurt Catti-Schmidt . unresolved

This is for all display types with gaps, right? Consider creating a new enum that uses inline/block for axis direction vs rows/columns. This will make it very obvious later when you assign inline vs block later (in `GapPainter::Paint`).

I realize Gap Decorations uses the concept of rows/columns already for non-grid display types, so this is also fine to keep as-is.

File third_party/blink/renderer/core/layout/gap/gap_geometry.cc

Line 168, Patchset 29: intersections.push_back(offset);

Kurt Catti-Schmidt . unresolved

`intersections.push_back(cross_gap.GetGapStartOffset().inline_offset);`

Line 182, Patchset 29: // TODO(samomekarajr): Can Do Merge two sorted lists here instead.

Kurt Catti-Schmidt . unresolved

This will also avoid the copy below - you can merge straight into `intersections`, right? (might be worth adding to the TODO).

Line 206, Patchset 29: for (const auto& cross_gap : all_cross_gaps) {

Kurt Catti-Schmidt . unresolved

```
// Copy merged and sorted values into `intersections`.
```

Line 245, Patchset 29: LayoutUnit offset = main_gap.GetGapStartOffset();
intersections.push_back(offset);

Kurt Catti-Schmidt . unresolved

You can combine these two lines:

```
        intersections.push_back(main_gap.GetGapStartOffset());
```

Line 296, Patchset 29: main_gap_running_index_ += 1;

Kurt Catti-Schmidt . unresolved

Nit: `++main_gap_running_index_;`

Line 299, Patchset 29: CHECK(main_gap_running_index_ < GetMainGaps().size());

Kurt Catti-Schmidt . unresolved

`CHECK_LT(main_gap_running_index_, GetMainGaps().size());`

(also do this above when you index into `GetMainGaps()`)

You can also store a reference to `GetMainGaps()` in a variable since you access it a few times here.

Line 315, Patchset 29: CHECK(!is_main_gap);

Kurt Catti-Schmidt . unresolved

This can probably be a `DCHECK` - you only want to `CHECK` if something unsafe or catastrophic would happen anyways

Line 323, Patchset 29: GetCrossGaps()[gap_index].GetEdgeIntersectionState();

Kurt Catti-Schmidt . unresolved

Always DCHECK or DCHECK before doing an array index like this `DCHECK_LT(gap_index, GetCrossGaps().size());`

File third_party/blink/renderer/core/paint/gap_painter.cc

Line 22, Patchset 29: BoxSide box_side;

if (style.IsHorizontalWritingMode()) {
if (style.IsLeftToRightDirection()) {
box_side = direction == kForColumns ? BoxSide::kLeft : BoxSide::kTop;
} else {
box_side = direction == kForColumns ? BoxSide::kRight : BoxSide::kBottom;
}
} else {
// Vertical Writing Mode.
const auto writing_direction = style.GetWritingDirection();
if (writing_direction.InlineEnd() == PhysicalDirection::kDown) {
// Top to Bottom.
box_side = direction == kForColumns ? BoxSide::kTop : BoxSide::kLeft;
} else {
// Bottom to Top.
box_side = direction == kForColumns ? BoxSide::kBottom : BoxSide::kRight;
}
}

return box_side;

Kurt Catti-Schmidt . unresolved

Can this be avoided by using `PhysicalBoxSides` and `ToLogical`?

Line 88, Patchset 29: if (is_main) {
center = gap_geometry.GetMainGaps()[gap_index].GetGapStartOffset();
} else {
const LogicalOffset cross_gap_offset =
gap_geometry.GetCrossGaps()[gap_index].GetGapStartOffset();
center = track_direction == kForColumns ? cross_gap_offset.inline_offset
: cross_gap_offset.block_offset;
}


Kurt Catti-Schmidt . unresolved

This might be cleaner to move the logic into `GapGeometry`, something like `GetStartOffset(wtf_size_t index, bool is_main_direction, GridTrackSizingDirection track_direction);`

Then this whole block reduces down to:

`LayoutUnit center = gap_geometry.GetStartOffset(index, is_main, track_direction);`

Line 97, Patchset 29: Vector<LayoutUnit> intersections;
gap_geometry.PopulateIntersectionListForGap(intersections, track_direction,
gap_index);

Kurt Catti-Schmidt . unresolved

Since `intersections` is just an out-parameter, you can just return it:

```
Vector<LayoutUnit> intersections = gap_geometry.PopulateIntersectionListForGap(intersections, track_direction,
                                                gap_index);
```

The compiler will automatically call `move` so it's not copying the vectors. See https://shaharmike.com/cpp/rvo/.

Line 114, Patchset 29: end = intersections.size() - 1;

Kurt Catti-Schmidt . unresolved

I would store this in a variable, something like:

`const wtf_size_t last_index = intersections.size() - 1;`

Line 126, Patchset 29: LayoutUnit start_width =
gap_geometry.IsEdgeIntersection(gap_index, start,
intersections.size(), is_main)
? LayoutUnit()
: cross_gutter_width;
LayoutUnit end_width = gap_geometry.IsEdgeIntersection(
gap_index, end, intersections.size(), is_main)
? LayoutUnit()
: cross_gutter_width;

Kurt Catti-Schmidt . unresolved

Make sure to `const` all of the locals that don't need to change

Line 147, Patchset 29: LayoutUnit(start_width / 2.0f) - start_outset;

Kurt Catti-Schmidt . unresolved

I think you can just do: `(start_width / 2) - start_outset` and don't need to wrap the intermediate value in a `LayoutUnit`

Line 153, Patchset 29: if (track_direction == kForColumns) {
// For columns, paint a vertical strip at the center of the gap.
inline_start = center - (rule_thickness / 2);
inline_size = rule_thickness;
// Compute the block positions using the computed offsets.
block_start = intersections[start] + decoration_start_offset;
block_size = intersections[end] - block_start - decoration_end_offset;
} else {
// For rows, paint a horizontal strip at the center of the gap.
block_start = center - (rule_thickness / 2);
block_size = rule_thickness;
// Compute the inline positions using the computed offsets.
inline_start = intersections[start] + decoration_start_offset;
inline_size = intersections[end] - inline_start - decoration_end_offset;
}

Kurt Catti-Schmidt . unresolved

You can reduce a lot of duplication here with the ternary, e.g.

```
(track_direction == kForColumn) ? inline_start : block_start = center - (rule_thickness / 2);
```

(I think that'll work)

Line 175, Patchset 29: start = end;

Kurt Catti-Schmidt . unresolved

Add a newline before this so it's more obvious how the iteration advances.

Open in Gerrit

Related details

Attention is currently required from:

• Sam Davis Omekara

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review
• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 29

Gerrit-Comment-Date: Mon, 25 Aug 2025 22:35:07 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Takuto Ikuta (Gerrit)]

Attention needed from Sam Davis Omekara

Takuto Ikuta removed chrome-b...@google.com from this change

Deleted Reviewers:

• chrome-b...@google.com

Open in Gerrit

Related details

Attention is currently required from:

• Sam Davis Omekara

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review
• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: deleteReviewer

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 30

[Sam Davis Omekara (Gerrit)]

Attention needed from Javier Contreras and Kurt Catti-Schmidt

Sam Davis Omekara added 26 comments

File third_party/blink/renderer/core/layout/flex/flex_layout_algorithm.cc

Line 802, Patchset 30: LayoutUnit border_scrollbar_padding =


is_column_
? container_builder_->BorderScrollbarPadding().block_end
: container_builder_->BorderScrollbarPadding().inline_end;
content_main_end_ =
is_column_
? container_builder_->InitialBorderBoxSize().block_size -
border_scrollbar_padding
: container_builder_->InlineSize() - border_scrollbar_padding;

Javier Contreras . resolved

I'm ok with keeping the flex changes in this CL, but let's call out in the CL description why we made these changes i.e.:

• Previously we were not setting the ContentStart/End offsets in cases where we only had one item in a flex line.
• The offset for the main gap does not need to be adjusted to inline/block depending on column, it will always be the offset of where the flex line ends + gap. (relevant test was adjusted as such.)

Sam Davis Omekara

Done

File third_party/blink/renderer/core/layout/gap/gap_geometry.h

Line 205, Patchset 30: // Computes the end offset for a flex cross gap at `cross_gap_index`. The end


// offset is either:
// - The container's content end, or
// - The offset of the main gap where this cross gap ends (tracked by
// `main_gap_running_index_`)

Javier Contreras . resolved

Thank you for detailed comments. I wish more of Blink had this.

Sam Davis Omekara

Done

Line 80, Patchset 30:// Gap locations are used for painting gap decorations.

Javier Contreras . resolved

We should link to readme here too.

Sam Davis Omekara

Done

Line 276, Patchset 29: mutable wtf_size_t main_gap_running_index_ = kNotFound;

Kurt Catti-Schmidt . unresolved

Can you make `ComputeEndOffsetForFlexCrossGap` non-const vs making this mutable?

Sam Davis Omekara

The main issue is that GapGeo is typically created during Layout and then used during Paint. The method ComputeEndOffsetForFlexCrossGap, which is called during Paint, is one of several methods that need to be const because gap geometry is const at Paint time. This means we can't use non-const methods on a const gap geometry, so it needed to be mutable.

Line 269, Patchset 29: GridTrackSizingDirection main_direction_ = kForRows;

Kurt Catti-Schmidt . unresolved

This is for all display types with gaps, right? Consider creating a new enum that uses inline/block for axis direction vs rows/columns. This will make it very obvious later when you assign inline vs block later (in `GapPainter::Paint`).

I realize Gap Decorations uses the concept of rows/columns already for non-grid display types, so this is also fine to keep as-is.

Sam Davis Omekara

Okay, if I get this right, you're of the opinion that we don't rely on the grid rows/cols concept and use inline vs block? Added this as a TODO to look into more. Let me know if this sufficient.

File third_party/blink/renderer/core/layout/gap/gap_geometry.cc

Line 168, Patchset 29: intersections.push_back(offset);

Kurt Catti-Schmidt . resolved

`intersections.push_back(cross_gap.GetGapStartOffset().inline_offset);`

Sam Davis Omekara

Done

Line 177, Patchset 30: // 1. Cross gaps that appear before the main gap


// 2. Cross gaps that appear after the main gap

Javier Contreras . resolved

would be good to link to the readme here, for future readers of this code, since this part can be confusing.

Sam Davis Omekara

Done

Line 182, Patchset 29: // TODO(samomekarajr): Can Do Merge two sorted lists here instead.

Kurt Catti-Schmidt . resolved

This will also avoid the copy below - you can merge straight into `intersections`, right? (might be worth adding to the TODO).

Sam Davis Omekara

Done

Line 206, Patchset 29: for (const auto& cross_gap : all_cross_gaps) {

Kurt Catti-Schmidt . resolved

```
// Copy merged and sorted values into `intersections`.
```

Sam Davis Omekara

Done

Line 245, Patchset 29: LayoutUnit offset = main_gap.GetGapStartOffset();
intersections.push_back(offset);

Kurt Catti-Schmidt . resolved

You can combine these two lines:

```
        intersections.push_back(main_gap.GetGapStartOffset());
```

Sam Davis Omekara

Done

Line 257, Patchset 30: // For a flex cross gap:


// - There are exactly two intersections:
// 1. The gap's start offset
// 2. Its computed end offset (either a main gap or the container's
// content-end edge)

Javier Contreras . resolved

ditto here.

Sam Davis Omekara

Done

Line 296, Patchset 29: main_gap_running_index_ += 1;

Kurt Catti-Schmidt . resolved

Nit: `++main_gap_running_index_;`

Sam Davis Omekara

Done

Line 299, Patchset 29: CHECK(main_gap_running_index_ < GetMainGaps().size());

Kurt Catti-Schmidt . resolved

`CHECK_LT(main_gap_running_index_, GetMainGaps().size());`

(also do this above when you index into `GetMainGaps()`)

You can also store a reference to `GetMainGaps()` in a variable since you access it a few times here.

Sam Davis Omekara

Done

Line 315, Patchset 29: CHECK(!is_main_gap);

Kurt Catti-Schmidt . resolved

This can probably be a `DCHECK` - you only want to `CHECK` if something unsafe or catastrophic would happen anyways

Sam Davis Omekara

Done

Line 323, Patchset 29: GetCrossGaps()[gap_index].GetEdgeIntersectionState();

Kurt Catti-Schmidt . unresolved

Always DCHECK or DCHECK before doing an array index like this `DCHECK_LT(gap_index, GetCrossGaps().size());`

Sam Davis Omekara

I actually used to do things like this but in one review, Kevin had mentioned it's redundant since [] operator in blink Vector already CHECKs this before accessing memory.

File third_party/blink/renderer/core/layout/gap/main_gap.h

Line 26, Patchset 30:


bool HasCrossGapsBefore() const {
return range_of_cross_gaps_before_.IsValid();
}

bool HasCrossGapsAfter() const {
return range_of_cross_gaps_after_.IsValid();
}

wtf_size_t GetCrossGapBeforeStart() const {
return range_of_cross_gaps_before_.Start();
}

wtf_size_t GetCrossGapBeforeEnd() const {
return range_of_cross_gaps_before_.End();
}

wtf_size_t GetCrossGapAfterStart() const {
return range_of_cross_gaps_after_.Start();
}

wtf_size_t GetCrossGapAfterEnd() const {
return range_of_cross_gaps_after_.End();
}

void IncrementRangeOfCrossGapsBefore(wtf_size_t cross_gap_index) {
range_of_cross_gaps_before_.Increment(cross_gap_index);
}

void IncrementRangeOfCrossGapsAfter(wtf_size_t cross_gap_index) {
range_of_cross_gaps_after_.Increment(cross_gap_index);
}

void SetRangeOfCrossGapsBefore(const CrossGapRange& range) {
range_of_cross_gaps_before_ = range;
}

void SetRangeOfCrossGapsAfter(const CrossGapRange& range) {
range_of_cross_gaps_after_ = range;
}

Javier Contreras . resolved

Call out in the description why we made these changes to the way with which we interface with the ranges.

Sam Davis Omekara

Done

File third_party/blink/renderer/core/paint/gap_painter.cc

Line 22, Patchset 29: BoxSide box_side;

if (style.IsHorizontalWritingMode()) {
if (style.IsLeftToRightDirection()) {
box_side = direction == kForColumns ? BoxSide::kLeft : BoxSide::kTop;
} else {
box_side = direction == kForColumns ? BoxSide::kRight : BoxSide::kBottom;
}
} else {
// Vertical Writing Mode.
const auto writing_direction = style.GetWritingDirection();
if (writing_direction.InlineEnd() == PhysicalDirection::kDown) {
// Top to Bottom.
box_side = direction == kForColumns ? BoxSide::kTop : BoxSide::kLeft;
} else {
// Bottom to Top.
box_side = direction == kForColumns ? BoxSide::kBottom : BoxSide::kRight;
}
}

return box_side;

Kurt Catti-Schmidt . unresolved

Can this be avoided by using `PhysicalBoxSides` and `ToLogical`?

Sam Davis Omekara

I'm not completely follow this 100%, but I checked PhysicalBoxSides and it covers all four sides. However, for the paint Op, we need just one side, which is BoxSide. Other similar methods that use this Paint Op also create a BoxSide.

Line 46, Patchset 30:void GapPainter::Paint(GridTrackSizingDirection track_direction,

Javier Contreras . resolved

nit: I feel like this function currently is doing too much (i feel like our previous version did too) and can be split to make it cleaner.

Foe example, we could have a separate function that handles returning the `center`, another for creating the `LogicalRect`, and maybe even the while loop itself could be split out into something like `ComputeGapSegments`

Sam Davis Omekara

Hmm, I see your point. but I'm weary of refactoring now that I haven't introduced the rule breaking code. I'll add this as a TODO and see how best to approach this refactoring to make function more modular.

Line 88, Patchset 29: if (is_main) {
center = gap_geometry.GetMainGaps()[gap_index].GetGapStartOffset();
} else {
const LogicalOffset cross_gap_offset =
gap_geometry.GetCrossGaps()[gap_index].GetGapStartOffset();
center = track_direction == kForColumns ? cross_gap_offset.inline_offset
: cross_gap_offset.block_offset;
}


Kurt Catti-Schmidt . resolved

This might be cleaner to move the logic into `GapGeometry`, something like `GetStartOffset(wtf_size_t index, bool is_main_direction, GridTrackSizingDirection track_direction);`

Then this whole block reduces down to:

`LayoutUnit center = gap_geometry.GetStartOffset(index, is_main, track_direction);`

Sam Davis Omekara

Done. You're so right. Thanks!!

Line 97, Patchset 29: Vector<LayoutUnit> intersections;
gap_geometry.PopulateIntersectionListForGap(intersections, track_direction,
gap_index);

Kurt Catti-Schmidt . resolved

Since `intersections` is just an out-parameter, you can just return it:

```
Vector<LayoutUnit> intersections = gap_geometry.PopulateIntersectionListForGap(intersections, track_direction,
                                                gap_index);
```

The compiler will automatically call `move` so it's not copying the vectors. See https://shaharmike.com/cpp/rvo/.

Sam Davis Omekara

Okay, if that's the case I'll rename functions to something like: `GenerateIntersection**`

Line 114, Patchset 29: end = intersections.size() - 1;

Kurt Catti-Schmidt . resolved

I would store this in a variable, something like:

`const wtf_size_t last_index = intersections.size() - 1;`

Sam Davis Omekara

Done

Line 126, Patchset 29: LayoutUnit start_width =
gap_geometry.IsEdgeIntersection(gap_index, start,
intersections.size(), is_main)
? LayoutUnit()
: cross_gutter_width;
LayoutUnit end_width = gap_geometry.IsEdgeIntersection(
gap_index, end, intersections.size(), is_main)
? LayoutUnit()
: cross_gutter_width;

Kurt Catti-Schmidt . resolved

Make sure to `const` all of the locals that don't need to change

Sam Davis Omekara

Done

Line 147, Patchset 29: LayoutUnit(start_width / 2.0f) - start_outset;

Kurt Catti-Schmidt . resolved

I think you can just do: `(start_width / 2) - start_outset` and don't need to wrap the intermediate value in a `LayoutUnit`

Sam Davis Omekara

Actually tried this so many times but it doesn't work, because:

"no viable conversion from 'float' to 'const LayoutUnit' "

Line 153, Patchset 29: if (track_direction == kForColumns) {
// For columns, paint a vertical strip at the center of the gap.
inline_start = center - (rule_thickness / 2);
inline_size = rule_thickness;
// Compute the block positions using the computed offsets.
block_start = intersections[start] + decoration_start_offset;
block_size = intersections[end] - block_start - decoration_end_offset;
} else {
// For rows, paint a horizontal strip at the center of the gap.
block_start = center - (rule_thickness / 2);
block_size = rule_thickness;
// Compute the inline positions using the computed offsets.
inline_start = intersections[start] + decoration_start_offset;
inline_size = intersections[end] - inline_start - decoration_end_offset;
}

Kurt Catti-Schmidt . unresolved

You can reduce a lot of duplication here with the ternary, e.g.

```
(track_direction == kForColumn) ? inline_start : block_start = center - (rule_thickness / 2);
```

(I think that'll work)

Sam Davis Omekara

I chose explicit branching here since it makes things clearer, rather than combining everything into one line with a ternary operator. If you agree, I'd prefer to keep it this way :)

Line 175, Patchset 29: start = end;

Kurt Catti-Schmidt . resolved

Add a newline before this so it's more obvious how the iteration advances.

Sam Davis Omekara

Done

File third_party/blink/web_tests/external/wpt/css/css-gaps/grid/grid-gap-decorations-003-ref.html

Line 1, Patchset 30:<!DOCTYPE html>

Javier Contreras . resolved

call out the reason for these changes in the CL description too.

Sam Davis Omekara

Done

Open in Gerrit

Related details

Attention is currently required from:

• Javier Contreras
• Kurt Catti-Schmidt

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review
• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 32

Gerrit-CC: Simon Hangl <sim...@google.com>

Gerrit-CC: Stephen Chenney <sche...@chromium.org>

Gerrit-CC: Steven Luong <stl...@chromium.org>

Gerrit-CC: Thiabaud Engelbrecht <thia...@google.com>

Gerrit-CC: Tommy Li <tomm...@chromium.org>

Gerrit-CC: Urvang Joshi <urv...@chromium.org>

Gerrit-CC: Victor Tan <vict...@chromium.org>

Gerrit-CC: Wanming Lin <wanmi...@intel.com>

Gerrit-CC: Wei4 Wang <wei4...@intel.com>

Gerrit-CC: Wojciech Dzierżanowski <wdzierz...@opera.com>

Gerrit-CC: Yoav Weiss (@Shopify) <yoav...@chromium.org>

Gerrit-CC: Zijie He <zij...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-CC: ningxin hu <ningx...@intel.com>

Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>

Gerrit-CC: Łukasz Bendig <lbe...@opera.com>

Gerrit-Attention: Javier Contreras <javi...@microsoft.com>

Gerrit-Attention: Kurt Catti-Schmidt <ksc...@microsoft.com>

Gerrit-Comment-Date: Tue, 26 Aug 2025 18:47:49 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Javier Contreras <javi...@microsoft.com>

Comment-In-Reply-To: Kurt Catti-Schmidt <ksc...@microsoft.com>

[Kurt Catti-Schmidt (Gerrit)]

Attention needed from Javier Contreras and Sam Davis Omekara

Kurt Catti-Schmidt added 8 comments

Patchset-level comments

File-level comment, Patchset 32 (Latest):

Kurt Catti-Schmidt . resolved

Getting very close. Also, do you know why it added so many people to CC?

File third_party/blink/renderer/core/layout/gap/gap_geometry.h

Line 276, Patchset 29: mutable wtf_size_t main_gap_running_index_ = kNotFound;

Kurt Catti-Schmidt . unresolved

Can you make `ComputeEndOffsetForFlexCrossGap` non-const vs making this mutable?

Sam Davis Omekara

The main issue is that GapGeo is typically created during Layout and then used during Paint. The method ComputeEndOffsetForFlexCrossGap, which is called during Paint, is one of several methods that need to be const because gap geometry is const at Paint time. This means we can't use non-const methods on a const gap geometry, so it needed to be mutable.

Kurt Catti-Schmidt

Typically you want to limit use of `mutable`, but this seems like a reasonable scenario to have a side effect. Can you add a comment here indicating why it needs to be mutable?

Line 269, Patchset 29: GridTrackSizingDirection main_direction_ = kForRows;

Kurt Catti-Schmidt . resolved

This is for all display types with gaps, right? Consider creating a new enum that uses inline/block for axis direction vs rows/columns. This will make it very obvious later when you assign inline vs block later (in `GapPainter::Paint`).

I realize Gap Decorations uses the concept of rows/columns already for non-grid display types, so this is also fine to keep as-is.

Sam Davis Omekara

Okay, if I get this right, you're of the opinion that we don't rely on the grid rows/cols concept and use inline vs block? Added this as a TODO to look into more. Let me know if this sufficient.

Kurt Catti-Schmidt

That works for me

File third_party/blink/renderer/core/layout/gap/gap_geometry.cc

Line 323, Patchset 29: GetCrossGaps()[gap_index].GetEdgeIntersectionState();

Kurt Catti-Schmidt . resolved

Always DCHECK or DCHECK before doing an array index like this `DCHECK_LT(gap_index, GetCrossGaps().size());`

Sam Davis Omekara

I actually used to do things like this but in one review, Kevin had mentioned it's redundant since [] operator in blink Vector already CHECKs this before accessing memory.

Kurt Catti-Schmidt

Nice, I wasn't aware of that

File third_party/blink/renderer/core/paint/gap_painter.cc

Line 22, Patchset 29: BoxSide box_side;

if (style.IsHorizontalWritingMode()) {
if (style.IsLeftToRightDirection()) {
box_side = direction == kForColumns ? BoxSide::kLeft : BoxSide::kTop;
} else {
box_side = direction == kForColumns ? BoxSide::kRight : BoxSide::kBottom;
}
} else {
// Vertical Writing Mode.
const auto writing_direction = style.GetWritingDirection();
if (writing_direction.InlineEnd() == PhysicalDirection::kDown) {
// Top to Bottom.
box_side = direction == kForColumns ? BoxSide::kTop : BoxSide::kLeft;
} else {
// Bottom to Top.
box_side = direction == kForColumns ? BoxSide::kBottom : BoxSide::kRight;
}
}

return box_side;

Kurt Catti-Schmidt . unresolved

Can this be avoided by using `PhysicalBoxSides` and `ToLogical`?

Sam Davis Omekara

I'm not completely follow this 100%, but I checked PhysicalBoxSides and it covers all four sides. However, for the paint Op, we need just one side, which is BoxSide. Other similar methods that use this Paint Op also create a BoxSide.

Kurt Catti-Schmidt

I think you can do something like this:

```
  PhysicalBoxSides sides;
  sides.left = (direction == kForColumns);
  sides.top = (direction == kForColumns);
  LogicalBoxSides logical_sizes = sides.ToLogical(
        WritingDirectionMode(style.GetWritingMode(), style.GetWritingDirection()))
  return (direction == kForColumns) ? logical_sizes.left : logical_sizes.top;
```

...where you just set top and left and let the converter handle the writing mode/direction. If this is too complicated what you have is fine. I might have mixed something up in this example too.

Line 96, Patchset 32 (Latest): LayoutUnit center =

Kurt Catti-Schmidt . unresolved

`const LayoutUnit`

Line 141, Patchset 32 (Latest): LayoutUnit(start_width / 2.0f - start_outset);

Kurt Catti-Schmidt . unresolved

Do you need the `LayoutUnit` constructor here? Algebra on `LayoutUnit` should just work for the most part. You might need to change the `2.0f` to just `2`.

Line 153, Patchset 29: if (track_direction == kForColumns) {
// For columns, paint a vertical strip at the center of the gap.
inline_start = center - (rule_thickness / 2);
inline_size = rule_thickness;
// Compute the block positions using the computed offsets.
block_start = intersections[start] + decoration_start_offset;
block_size = intersections[end] - block_start - decoration_end_offset;
} else {
// For rows, paint a horizontal strip at the center of the gap.
block_start = center - (rule_thickness / 2);
block_size = rule_thickness;
// Compute the inline positions using the computed offsets.
inline_start = intersections[start] + decoration_start_offset;
inline_size = intersections[end] - inline_start - decoration_end_offset;
}

Kurt Catti-Schmidt . unresolved

You can reduce a lot of duplication here with the ternary, e.g.

```
(track_direction == kForColumn) ? inline_start : block_start = center - (rule_thickness / 2);
```

(I think that'll work)

Sam Davis Omekara

I chose explicit branching here since it makes things clearer, rather than combining everything into one line with a ternary operator. If you agree, I'd prefer to keep it this way :)

Kurt Catti-Schmidt

This is kind of a lot of duplication. Another option is to use `std::swap`. e.g.

```

// For columns, paint a vertical strip at the center of the gap.
inline_start = center - (rule_thickness / 2);
inline_size = rule_thickness;
// Compute the block positions using the computed offsets.
block_start = intersections[start] + decoration_start_offset;
block_size = intersections[end] - block_start - decoration_end_offset;

// For rows, swap inline to block.
if(track_direction == kForRows) {
  std::swap(inline_start, block_start);
  std::swap(inline_end, block_end);
}

```

Open in Gerrit

Related details

Attention is currently required from:

• Javier Contreras
• Sam Davis Omekara

Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>

Gerrit-Comment-Date: Tue, 26 Aug 2025 21:09:35 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Sam Davis Omekara <samome...@microsoft.com>

Comment-In-Reply-To: Kurt Catti-Schmidt <ksc...@microsoft.com>

[Sam Davis Omekara (Gerrit)]

Attention needed from Javier Contreras and Kurt Catti-Schmidt

Sam Davis Omekara added 6 comments

Patchset-level comments

File-level comment, Patchset 32:

Kurt Catti-Schmidt . resolved

Getting very close. Also, do you know why it added so many people to CC?

Sam Davis Omekara

I think I didn't sync properly one time + I had this as a dependent change of some other branch that kept being out of sync with main. So when I uploaded it showed the diff between this and the current main and added a whole bunch of people to the CC.

File third_party/blink/renderer/core/layout/gap/gap_geometry.h

Line 276, Patchset 29: mutable wtf_size_t main_gap_running_index_ = kNotFound;

Kurt Catti-Schmidt . resolved

Can you make `ComputeEndOffsetForFlexCrossGap` non-const vs making this mutable?

Sam Davis Omekara

The main issue is that GapGeo is typically created during Layout and then used during Paint. The method ComputeEndOffsetForFlexCrossGap, which is called during Paint, is one of several methods that need to be const because gap geometry is const at Paint time. This means we can't use non-const methods on a const gap geometry, so it needed to be mutable.

Kurt Catti-Schmidt

Typically you want to limit use of `mutable`, but this seems like a reasonable scenario to have a side effect. Can you add a comment here indicating why it needs to be mutable?

Sam Davis Omekara

Done

File third_party/blink/renderer/core/paint/gap_painter.cc

Line 22, Patchset 29: BoxSide box_side;

if (style.IsHorizontalWritingMode()) {
if (style.IsLeftToRightDirection()) {
box_side = direction == kForColumns ? BoxSide::kLeft : BoxSide::kTop;
} else {
box_side = direction == kForColumns ? BoxSide::kRight : BoxSide::kBottom;
}
} else {
// Vertical Writing Mode.
const auto writing_direction = style.GetWritingDirection();
if (writing_direction.InlineEnd() == PhysicalDirection::kDown) {
// Top to Bottom.
box_side = direction == kForColumns ? BoxSide::kTop : BoxSide::kLeft;
} else {
// Bottom to Top.
box_side = direction == kForColumns ? BoxSide::kBottom : BoxSide::kRight;
}
}

return box_side;

Kurt Catti-Schmidt . resolved

Can this be avoided by using `PhysicalBoxSides` and `ToLogical`?

Sam Davis Omekara

I'm not completely follow this 100%, but I checked PhysicalBoxSides and it covers all four sides. However, for the paint Op, we need just one side, which is BoxSide. Other similar methods that use this Paint Op also create a BoxSide.

Kurt Catti-Schmidt

I think you can do something like this:

```
  PhysicalBoxSides sides;
  sides.left = (direction == kForColumns);
  sides.top = (direction == kForColumns);
  LogicalBoxSides logical_sizes = sides.ToLogical(
        WritingDirectionMode(style.GetWritingMode(), style.GetWritingDirection()))
  return (direction == kForColumns) ? logical_sizes.left : logical_sizes.top;
```

...where you just set top and left and let the converter handle the writing mode/direction. If this is too complicated what you have is fine. I might have mixed something up in this example too.

Sam Davis Omekara

Started this but added as a TODO because I got cold feet over seeing the PhysidalBoxSide initializes all four sides to true initially. Not sure what the implication would be here... Added as a TODO to explore. Thanks for the suggestion.

Line 96, Patchset 32: LayoutUnit center =

Kurt Catti-Schmidt . resolved

`const LayoutUnit`

Sam Davis Omekara

Done

Line 141, Patchset 32: LayoutUnit(start_width / 2.0f - start_outset);

Kurt Catti-Schmidt . resolved

Do you need the `LayoutUnit` constructor here? Algebra on `LayoutUnit` should just work for the most part. You might need to change the `2.0f` to just `2`.

Sam Davis Omekara

Done

Sam Davis Omekara

Opted for shared names as discussed offline. Let me know if this is sufficient.

Open in Gerrit

Related details

Attention is currently required from:

• Javier Contreras
• Kurt Catti-Schmidt

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review
• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 34

Gerrit-Attention: Kurt Catti-Schmidt <ksc...@microsoft.com>

Gerrit-Comment-Date: Tue, 26 Aug 2025 23:03:30 +0000

[Kurt Catti-Schmidt (Gerrit)]

Attention needed from Javier Contreras and Sam Davis Omekara

Kurt Catti-Schmidt voted and added 1 comment

Votes added by Kurt Catti-Schmidt

Code-Review

+1

1 comment

File third_party/blink/renderer/core/paint/gap_painter.cc

Line 153, Patchset 29: if (track_direction == kForColumns) {
// For columns, paint a vertical strip at the center of the gap.
inline_start = center - (rule_thickness / 2);
inline_size = rule_thickness;
// Compute the block positions using the computed offsets.
block_start = intersections[start] + decoration_start_offset;
block_size = intersections[end] - block_start - decoration_end_offset;
} else {
// For rows, paint a horizontal strip at the center of the gap.
block_start = center - (rule_thickness / 2);
block_size = rule_thickness;
// Compute the inline positions using the computed offsets.
inline_start = intersections[start] + decoration_start_offset;
inline_size = intersections[end] - inline_start - decoration_end_offset;
}

Kurt Catti-Schmidt . resolved

You can reduce a lot of duplication here with the ternary, e.g.

```
(track_direction == kForColumn) ? inline_start : block_start = center - (rule_thickness / 2);
```

(I think that'll work)

Sam Davis Omekara

I chose explicit branching here since it makes things clearer, rather than combining everything into one line with a ternary operator. If you agree, I'd prefer to keep it this way :)

Kurt Catti-Schmidt

This is kind of a lot of duplication. Another option is to use `std::swap`. e.g.

```
// For columns, paint a vertical strip at the center of the gap.
inline_start = center - (rule_thickness / 2);
inline_size = rule_thickness;
// Compute the block positions using the computed offsets.
block_start = intersections[start] + decoration_start_offset;
block_size = intersections[end] - block_start - decoration_end_offset;

// For rows, swap inline to block.
if(track_direction == kForRows) {
  std::swap(inline_start, block_start);
  std::swap(inline_end, block_end);
}

```

Sam Davis Omekara

Opted for shared names as discussed offline. Let me know if this is sufficient.

Kurt Catti-Schmidt

Marked as resolved.

Open in Gerrit

Related details

Attention is currently required from:

• Javier Contreras
• Sam Davis Omekara

Submit Requirements:

• Code-Coverage
• Code-Owners

• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 35

Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>

Gerrit-Comment-Date: Tue, 26 Aug 2025 23:19:01 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Sam Davis Omekara (Gerrit)]

Attention needed from Ian Kilpatrick and Javier Contreras

Sam Davis Omekara added 1 comment

Patchset-level comments

File-level comment, Patchset 35 (Latest):

Sam Davis Omekara . resolved

ikilp...@chromium.org ptal :)

Open in Gerrit

Related details

Attention is currently required from:

• Ian Kilpatrick
• Javier Contreras

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 35

Gerrit-Owner: Sam Davis Omekara <samome...@microsoft.com>

Gerrit-Reviewer: Ian Kilpatrick <ikilp...@chromium.org>

Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>

Gerrit-Reviewer: Kurt Catti-Schmidt <ksc...@microsoft.com>

Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>

Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>

Gerrit-CC: Akihiro Ota <akihi...@chromium.org>

Gerrit-CC: Ale Bzk <ales...@chromium.org>

Gerrit-CC: Prashant Nevase <pne...@microsoft.com>

Gerrit-CC: Prashant Nevase <pras...@chromium.org>

Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>

Gerrit-CC: Reid Kleckner <r...@chromium.org>

Gerrit-CC: Robert Kroeger <rjkr...@chromium.org>

Gerrit-CC: Ryan Sultanem <rs...@google.com>

Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>

Gerrit-CC: Sam Zackrisson <sa...@chromium.org>

Gerrit-CC: Sarah Krakowiak <skra...@chromium.org>

Gerrit-CC: Simon Hangl <sim...@google.com>

Gerrit-CC: Steven Luong <stl...@chromium.org>

Gerrit-CC: Thiabaud Engelbrecht <thia...@google.com>

Gerrit-CC: Tommy Li <tomm...@chromium.org>

Gerrit-CC: Urvang Joshi <urv...@chromium.org>

Gerrit-CC: Victor Tan <vict...@chromium.org>

Gerrit-CC: Wanming Lin <wanmi...@intel.com>

Gerrit-CC: Wei4 Wang <wei4...@intel.com>

Gerrit-CC: Wojciech Dzierżanowski <wdzierz...@opera.com>

Gerrit-CC: Yoav Weiss (@Shopify) <yoav...@chromium.org>

Gerrit-CC: Zijie He <zij...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-CC: ningxin hu <ningx...@intel.com>

Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>

Gerrit-CC: Łukasz Bendig <lbe...@opera.com>

Gerrit-Attention: Javier Contreras <javi...@microsoft.com>

Gerrit-Attention: Ian Kilpatrick <ikilp...@chromium.org>

Gerrit-Comment-Date: Wed, 27 Aug 2025 00:42:42 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Andrew Liu (Gerrit)]

Attention needed from Ian Kilpatrick and Javier Contreras

Andrew Liu removed liu+wa...@chromium.org from this change

Deleted Reviewers:

• liu+wa...@chromium.org

Open in Gerrit

Related details

Attention is currently required from:

• Ian Kilpatrick
• Javier Contreras

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: deleteReviewer

[Greg Thompson (Gerrit)]

Attention needed from Ian Kilpatrick and Javier Contreras

Greg Thompson removed grt+...@chromium.org from this change

Deleted Reviewers:

• grt+...@chromium.org

[Adem Derinel (Gerrit)]

Attention needed from Ian Kilpatrick and Javier Contreras

Adem Derinel removed derinel+wat...@google.com from this change

Deleted Reviewers:

• derinel+wat...@google.com

[gwsq (Gerrit)]

Attention needed from Ian Kilpatrick, Javier Contreras and Sam Davis Omekara

Message from gwsq

gwsq error while processing this CL: Error looking up group chromeos-commercial-r...@google.com: Streaming RPC error: status 2 error: ??? to (unknown) : DEADLINE_EXCEEDED goa-auth: RPC::DEADLINE_EXCEEDED: LocalIAM check returned an error: goa-auth: RPC::DEADLINE_EXCEEDED: LocalIAM check returned an error:

Open in Gerrit

Related details

Attention is currently required from:

• Ian Kilpatrick
• Javier Contreras
• Sam Davis Omekara

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-CC: gwsq

Gerrit-CC: ningxin hu <ningx...@intel.com>

Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>

Gerrit-CC: Łukasz Bendig <lbe...@opera.com>

Gerrit-Attention: Javier Contreras <javi...@microsoft.com>

Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>

Gerrit-Attention: Ian Kilpatrick <ikilp...@chromium.org>

Gerrit-Comment-Date: Wed, 27 Aug 2025 19:12:31 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: No

[Kurt Catti-Schmidt (Gerrit)]

Attention needed from Ian Kilpatrick, Javier Contreras and Sam Davis Omekara

Kurt Catti-Schmidt voted and added 2 comments

Votes added by Kurt Catti-Schmidt

Code-Review

+1

2 comments

File third_party/blink/renderer/core/layout/gap/gap_geometry.cc

Line 331, Patchset 37 (Latest): intersection_index == intersection_count - 1;

Kurt Catti-Schmidt . unresolved

You should `DHECK_GT(intersection_count, 0);` to validate that it doesn't underflow.

You can also store `intersection_count - 1` in a variable, since it's repeated a few times.

File third_party/blink/renderer/core/paint/gap_painter.cc

Line 74, Patchset 37 (Latest): LayoutUnit cross_gutter_width = track_direction == kForRows
? gap_geometry.GetInlineGapSize()
: gap_geometry.GetBlockGapSize();

Kurt Catti-Schmidt . unresolved

Nit: some of these variables in this method can be `const`. This, `gap_count`, and maybe more.

Open in Gerrit

Related details

Attention is currently required from:

• Ian Kilpatrick
• Javier Contreras
• Sam Davis Omekara

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 37

Gerrit-Comment-Date: Thu, 28 Aug 2025 17:01:11 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Sam Davis Omekara (Gerrit)]

Sam Davis Omekara added 2 comments

File third_party/blink/renderer/core/layout/gap/gap_geometry.cc

Line 331, Patchset 37: intersection_index == intersection_count - 1;

Kurt Catti-Schmidt . resolved

You should `DHECK_GT(intersection_count, 0);` to validate that it doesn't underflow.

You can also store `intersection_count - 1` in a variable, since it's repeated a few times.

Sam Davis Omekara

Done

File third_party/blink/renderer/core/paint/gap_painter.cc

Line 74, Patchset 37: LayoutUnit cross_gutter_width = track_direction == kForRows
? gap_geometry.GetInlineGapSize()
: gap_geometry.GetBlockGapSize();

Kurt Catti-Schmidt . resolved

Nit: some of these variables in this method can be `const`. This, `gap_count`, and maybe more.

Sam Davis Omekara

Done

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Coverage
• Code-Owners

• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 41

Gerrit-Comment-Date: Thu, 28 Aug 2025 17:47:19 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Kurt Catti-Schmidt <ksc...@microsoft.com>

[Kurt Catti-Schmidt (Gerrit)]

Attention needed from Sam Davis Omekara

Kurt Catti-Schmidt voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

• Sam Davis Omekara

Submit Requirements:

• Code-Coverage
• Code-Owners

• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 42

Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>

Gerrit-Comment-Date: Thu, 28 Aug 2025 17:56:27 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Andrew Paseltiner (Gerrit)]

Attention needed from Sam Davis Omekara

Andrew Paseltiner removed apaselti...@chromium.org from this change

Deleted Reviewers:

• apaselti...@chromium.org

Open in Gerrit

Related details

Attention is currently required from:

• Sam Davis Omekara

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: deleteReviewer

[Javier Contreras (Gerrit)]

Attention needed from Ian Kilpatrick

Javier Contreras added 1 comment

File third_party/blink/renderer/core/layout/gap/gap_geometry.cc

Line 244, Patchset 42 (Latest): Vector<LayoutUnit> intersections;

Javier Contreras . unresolved

just realized this, but I think we should `ReserveInitialCapacity(main_gaps_.size() + 2)` since at most we'll have that number of cross intersections? I guess for flex we know its just two, so we could instead call .reserve() inside each of the cases.

Open in Gerrit

Related details

Attention is currently required from:

• Ian Kilpatrick

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Attention: Ian Kilpatrick <ikilp...@chromium.org>

Gerrit-Comment-Date: Fri, 29 Aug 2025 19:17:17 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Javier Contreras (Gerrit)]

Attention needed from Ian Kilpatrick and Sam Davis Omekara

Javier Contreras voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

• Ian Kilpatrick
• Sam Davis Omekara

Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>

Gerrit-Attention: Ian Kilpatrick <ikilp...@chromium.org>

Gerrit-Comment-Date: Fri, 29 Aug 2025 19:42:38 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Ian Kilpatrick (Gerrit)]

Attention needed from Sam Davis Omekara

Ian Kilpatrick added 1 comment

File third_party/blink/renderer/core/paint/gap_painter.cc

Line 23, Patchset 42 (Latest): // ToLogical to avoid manual conversion logic.

Ian Kilpatrick . unresolved

Might be worth to do this now, e.g.

Can use:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/text/writing_mode_utils.h;l=40-54?q=physicaltoLogical&ss=chromium

or LogicalToPhysical

Open in Gerrit

Related details

Attention is currently required from:

• Sam Davis Omekara

Gerrit-Comment-Date: Fri, 29 Aug 2025 23:56:50 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Sam Davis Omekara (Gerrit)]

Sam Davis Omekara added 2 comments

File third_party/blink/renderer/core/layout/gap/gap_geometry.cc

Line 244, Patchset 42: Vector<LayoutUnit> intersections;

Javier Contreras . resolved

just realized this, but I think we should `ReserveInitialCapacity(main_gaps_.size() + 2)` since at most we'll have that number of cross intersections? I guess for flex we know its just two, so we could instead call .reserve() inside each of the cases.

Sam Davis Omekara

Done

File third_party/blink/renderer/core/paint/gap_painter.cc

Line 23, Patchset 42: // ToLogical to avoid manual conversion logic.

Ian Kilpatrick . resolved

Might be worth to do this now, e.g.

Can use:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/text/writing_mode_utils.h;l=40-54?q=physicaltoLogical&ss=chromium

or LogicalToPhysical

Sam Davis Omekara

Done

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 43

Gerrit-Comment-Date: Tue, 02 Sep 2025 20:49:12 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Javier Contreras <javi...@microsoft.com>

Comment-In-Reply-To: Ian Kilpatrick <ikilp...@chromium.org>

[Alison Maher (Gerrit)]

Attention needed from Sam Davis Omekara

Alison Maher added 26 comments

Commit Message

Line 33, Patchset 43 (Latest):change.

Alison Maher . unresolved

nit: might be worth noting the test failures. Were they all related to this?

File third_party/blink/renderer/core/layout/flex/flex_layout_algorithm.cc

Line 801, Patchset 43 (Latest): if (num_lines_ > 1 && !content_main_end_.has_value()) {

Alison Maher . unresolved

Will this not be guaranteed if `is_first_item` is true? (i.e. could this be a CHECK instead?)

Line 845, Patchset 43 (Latest): main_gaps_[flex_line_index].SetGapStartOffset(cross_intersection_offset);

Alison Maher . unresolved

Kind of confused why we populate main gaps with the cross intersection. Should this variable be renamed? If not, might be worth a comment to explain

File third_party/blink/renderer/core/layout/gap/gap_geometry.h

Line 288, Patchset 43 (Latest): // this index as part of its calculation. Making this mutable allows us to
// maintain necessary state without breaking const-correctness for the overall
// GapGeometry object.


mutable wtf_size_t main_gap_running_index_ = kNotFound;

Alison Maher . unresolved

Would it make sense for paint to hold this instead and pass it in as an input/output param wherever needed?

Line 287, Patchset 43 (Latest): // Paint, but ComputeEndOffsetForFlexCrossGap (called at paint time) updates

Alison Maher . unresolved

ComputeEndOffsetForFlexCrossGap()

Line 213, Patchset 43 (Latest): // - The container's content end, or


// - The offset of the main gap where this cross gap ends (tracked by
// `main_gap_running_index_`)

Alison Maher . unresolved

Might be worth a note on when it is each of these

Line 199, Patchset 43 (Latest): Vector<LayoutUnit> GenerateIntersectionListForMain(
GridTrackSizingDirection direction,
wtf_size_t gap_index) const;

// Generates intersections for a cross gap. For grid containers, this
// includes the container content edges and every main gap offset. For flex
// containers, it includes the cross-gap start offset and its computed end
// offset.
Vector<LayoutUnit> GenerateIntersectionListForCross(

Alison Maher . unresolved

Can these two be made private?

Line 193, Patchset 43 (Latest): // Generates intersection offsets for a main gap at `gap_index`. This list

Alison Maher . unresolved

Same here and below "Returns a list of..."

Line 184, Patchset 43 (Latest): // This methods generates `intersections` with the ordered intersection

Alison Maher . unresolved

I'm guessing this is a var within the method. If so, I'd remove this and say something like "returns a Vector of offsets" instead

Line 180, Patchset 43 (Latest): LayoutUnit GetGapCenterOffset(GridTrackSizingDirection direction,

Alison Maher . unresolved

nit: might be worth a brief comment for this one, too

Line 152, Patchset 43 (Latest): bool IsDirectionMain(GridTrackSizingDirection direction) const {

Alison Maher . unresolved

nit: `IsMainDirection`

File third_party/blink/renderer/core/layout/gap/gap_geometry.cc

Line 140, Patchset 43 (Latest):LayoutUnit GapGeometry::GetGapCenterOffset(GridTrackSizingDirection direction,

Alison Maher . unresolved

If it is the start, it won't be center, right? Should this be GetGapIntersectionOffset()? Not a strong preference though either way

Line 156, Patchset 43 (Latest): } else {

Alison Maher . unresolved

nit: else isn't needed in this case

i.e.

```
  if (IsDirectionMain(direction)) {
    return GenerateIntersectionListForMain(direction, gap_index);
  }
  return GenerateIntersectionListForCross(direction, gap_index);
```

Line 161, Patchset 43 (Latest):Vector<LayoutUnit> GapGeometry::GenerateIntersectionListForMain(

Alison Maher . unresolved

Instead of building up a vector, would it make sense to have an iterator instead?

I guess maybe not because of the complexity of flexbox?

Line 161, Patchset 43 (Latest):Vector<LayoutUnit> GapGeometry::GenerateIntersectionListForMain(

Alison Maher . unresolved

GenerateMainIntersectionList

same for Cross

Line 174, Patchset 43 (Latest): CHECK(GetMainDirection() == kForRows);

Alison Maher . unresolved

CHECK_EQ

Line 191, Patchset 43 (Latest): CrossGaps all_cross_gaps;

Alison Maher . unresolved

nit: could just be cross_gaps

Line 316, Patchset 43 (Latest): // advance main_gap_running_index_ to the next main gap.

Alison Maher . unresolved

missing ``

Line 343, Patchset 43 (Latest): // * kBoth: Both first and last intersections are edges.
// * kStart: Only the first intersection is an edge.
// * kEnd: Only the last intersection is an edge.

Alison Maher . unresolved

Can we not just use these for all cases rather than diverging?

File third_party/blink/renderer/core/layout/gap/main_gap.h

Line 36, Patchset 43 (Latest): return range_of_cross_gaps_before_.Start();

Alison Maher . unresolved

Should this CHECK(HasCrossGapsBefore());

Same with other relevant methods

File third_party/blink/renderer/core/paint/gap_painter.h

Line 18, Patchset 43 (Latest):class GapPainter {

Alison Maher . unresolved

Might be worth an high level overall comment for this class (even if just with a link to the spec or something)

Line 18, Patchset 43 (Latest):class GapPainter {

Alison Maher . unresolved

nit: Would it make sense for this to be GapDecorationPainter instead to avoid confusion with just painting a normal gap (which I guess isn't really painted at all, but just an offset)

File third_party/blink/renderer/core/paint/gap_painter.cc

Line 20, Patchset 43 (Latest):BoxSide BoxSideFromGridDirection(const ComputedStyle& style,

Alison Maher . unresolved

This may make sense in one of the grid utils files instead

Line 88, Patchset 43 (Latest): wtf_size_t end = start;

Alison Maher . unresolved

Why do we set this to start instead of directly to `last_intersection_index`?

Also I don't see end or last_intersection_index ever changing. Do we need both of these vars, or am I just overlooking something?

Line 93, Patchset 43 (Latest): if (start >= end) {

Alison Maher . unresolved

Is this ever going to happen because of the `while (start < last_intersection_index) {`?

Line 139, Patchset 43 (Latest): const bool is_column_gap = (track_direction == kForColumns);

Alison Maher . unresolved

I'd move this up higher so we don't need to recompute it throughout the loop (and there are some places we can use this higher up, as well)

Open in Gerrit

Related details

Attention is currently required from:

• Sam Davis Omekara

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 43

Gerrit-Owner: Sam Davis Omekara <samome...@microsoft.com>

Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>

Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>

Gerrit-Comment-Date: Wed, 03 Sep 2025 20:38:38 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Sam Davis Omekara (Gerrit)]

Attention needed from Alison Maher

Sam Davis Omekara added 26 comments

Commit Message

Line 33, Patchset 43:change.

Alison Maher . resolved

nit: might be worth noting the test failures. Were they all related to this?

Sam Davis Omekara

Yes, they are. Added to the CL desc

File third_party/blink/renderer/core/layout/flex/flex_layout_algorithm.cc

Line 801, Patchset 43: if (num_lines_ > 1 && !content_main_end_.has_value()) {

Alison Maher . unresolved

Will this not be guaranteed if `is_first_item` is true? (i.e. could this be a CHECK instead?)

Sam Davis Omekara

Was going to be on board but I think this code path hits for every first item in every line so the extra condition ensures we only set it once.

cc: @javi...@microsoft.com is this assumption right?

Line 845, Patchset 43: main_gaps_[flex_line_index].SetGapStartOffset(cross_intersection_offset);

Alison Maher . resolved

Kind of confused why we populate main gaps with the cross intersection. Should this variable be renamed? If not, might be worth a comment to explain

Sam Davis Omekara

Great catch. Wrong naming, renaming to gap_offset.

File third_party/blink/renderer/core/layout/gap/gap_geometry.h

Line 288, Patchset 43: // this index as part of its calculation. Making this mutable allows us to


// maintain necessary state without breaking const-correctness for the overall
// GapGeometry object.
mutable wtf_size_t main_gap_running_index_ = kNotFound;

Alison Maher . unresolved

Would it make sense for paint to hold this instead and pass it in as an input/output param wherever needed?

Sam Davis Omekara

I think that might work and make sense, but might need some restructuring. Are you okay landing as is while I explore that in a follow up change or you think it's worth pursuing now.

Line 287, Patchset 43: // Paint, but ComputeEndOffsetForFlexCrossGap (called at paint time) updates

Alison Maher . resolved

ComputeEndOffsetForFlexCrossGap()

Sam Davis Omekara

Done

Line 213, Patchset 43: // - The container's content end, or


// - The offset of the main gap where this cross gap ends (tracked by
// `main_gap_running_index_`)

Alison Maher . resolved

Might be worth a note on when it is each of these

Sam Davis Omekara

Done

Line 199, Patchset 43: Vector<LayoutUnit> GenerateIntersectionListForMain(


GridTrackSizingDirection direction,
wtf_size_t gap_index) const;

// Generates intersections for a cross gap. For grid containers, this
// includes the container content edges and every main gap offset. For flex
// containers, it includes the cross-gap start offset and its computed end
// offset.
Vector<LayoutUnit> GenerateIntersectionListForCross(

Alison Maher . resolved

Can these two be made private?

Sam Davis Omekara

All three can. Yes!

Line 193, Patchset 43: // Generates intersection offsets for a main gap at `gap_index`. This list

Alison Maher . resolved

Same here and below "Returns a list of..."

Sam Davis Omekara

Done

Line 184, Patchset 43: // This methods generates `intersections` with the ordered intersection

Alison Maher . resolved

I'm guessing this is a var within the method. If so, I'd remove this and say something like "returns a Vector of offsets" instead

Sam Davis Omekara

Done

Line 180, Patchset 43: LayoutUnit GetGapCenterOffset(GridTrackSizingDirection direction,

Alison Maher . unresolved

nit: might be worth a brief comment for this one, too

Sam Davis Omekara

Ditto, I created the

Line 152, Patchset 43: bool IsDirectionMain(GridTrackSizingDirection direction) const {

Alison Maher . resolved

nit: `IsMainDirection`

Sam Davis Omekara

Done

File third_party/blink/renderer/core/layout/gap/gap_geometry.cc

Line 140, Patchset 43:LayoutUnit GapGeometry::GetGapCenterOffset(GridTrackSizingDirection direction,

Alison Maher . unresolved

If it is the start, it won't be center, right? Should this be GetGapIntersectionOffset()? Not a strong preference though either way

Sam Davis Omekara

Yeahh that method and naming is so confusing. I'm creating a separate CL[1] that removes start from these offset's naming, I think this can be renamed to GetGapOffset with that landing.

[1]: https://chromium-review.googlesource.com/c/chromium/src/+/6915478

Line 156, Patchset 43: } else {

Alison Maher . resolved

nit: else isn't needed in this case

i.e.

```
  if (IsDirectionMain(direction)) {
    return GenerateIntersectionListForMain(direction, gap_index);
  }
  return GenerateIntersectionListForCross(direction, gap_index);
```

Sam Davis Omekara

Done

Line 161, Patchset 43:Vector<LayoutUnit> GapGeometry::GenerateIntersectionListForMain(

Alison Maher . resolved

GenerateMainIntersectionList

same for Cross

Sam Davis Omekara

Done

Line 161, Patchset 43:Vector<LayoutUnit> GapGeometry::GenerateIntersectionListForMain(

Alison Maher . unresolved

Instead of building up a vector, would it make sense to have an iterator instead?

I guess maybe not because of the complexity of flexbox?

Sam Davis Omekara

I had explored this earlier a bit, but I think the iterator route imo was becoming a tad too complicated for something that could easily be represented as a list. More than happy to add as a TODO and explore that further later.

Line 174, Patchset 43: CHECK(GetMainDirection() == kForRows);

Alison Maher . resolved

CHECK_EQ

Sam Davis Omekara

Done

Line 191, Patchset 43: CrossGaps all_cross_gaps;

Alison Maher . resolved

nit: could just be cross_gaps

Sam Davis Omekara

Done

Line 316, Patchset 43: // advance main_gap_running_index_ to the next main gap.

Alison Maher . resolved

missing ``

Sam Davis Omekara

Done

Line 343, Patchset 43: // * kBoth: Both first and last intersections are edges.


// * kStart: Only the first intersection is an edge.
// * kEnd: Only the last intersection is an edge.

Alison Maher . unresolved

Can we not just use these for all cases rather than diverging?

Sam Davis Omekara

No we can't use this for all cases because I for all main gaps the first regardless of edge state the first and last intersections are always edge intersections.

If your suggestion is to mark all main gaps kBoth(because technically they are) then I think we can use this for all cases. But rn, the EdgeGap state is only for cross gaps.

File third_party/blink/renderer/core/layout/gap/main_gap.h

Line 36, Patchset 43: return range_of_cross_gaps_before_.Start();

Alison Maher . resolved

Should this CHECK(HasCrossGapsBefore());

Same with other relevant methods

Sam Davis Omekara

Done

File third_party/blink/renderer/core/paint/gap_painter.h

Line 18, Patchset 43:class GapPainter {

Alison Maher . resolved

Might be worth an high level overall comment for this class (even if just with a link to the spec or something)

Sam Davis Omekara

Done

Line 18, Patchset 43:class GapPainter {

Alison Maher . resolved

nit: Would it make sense for this to be GapDecorationPainter instead to avoid confusion with just painting a normal gap (which I guess isn't really painted at all, but just an offset)

Sam Davis Omekara

Done

File third_party/blink/renderer/core/paint/gap_painter.cc

Line 20, Patchset 43:BoxSide BoxSideFromGridDirection(const ComputedStyle& style,

Alison Maher . resolved

This may make sense in one of the grid utils files instead

Sam Davis Omekara

Hmm, grid or maybe CSS Gap Decorations Utils?

Line 88, Patchset 43: wtf_size_t end = start;

Alison Maher . unresolved

Why do we set this to start instead of directly to `last_intersection_index`?

Also I don't see end or last_intersection_index ever changing. Do we need both of these vars, or am I just overlooking something?

Sam Davis Omekara

I think the additional context here is that the code line below won't live for long (i.e. after the next CL). It was just a placeholder to allow us paint from the first intersection to the last one.

In the child CL we call the Adjust method that properly assigns start and end.

Line 93, Patchset 43: if (start >= end) {

Alison Maher . unresolved

Is this ever going to happen because of the `while (start < last_intersection_index) {`?

Sam Davis Omekara

Similar to the above comment, I think the Adjust method in the next CL could see us land in a scenario where start>= end. Didn't work on this bit of logic recently but I vaguely remember this being an edge case in the way we adjust the start/end pair.

Line 139, Patchset 43: const bool is_column_gap = (track_direction == kForColumns);

Alison Maher . resolved

I'd move this up higher so we don't need to recompute it throughout the loop (and there are some places we can use this higher up, as well)

Sam Davis Omekara

Done

Open in Gerrit

Related details

Attention is currently required from:

• Alison Maher

Submit Requirements:

• Code-Coverage
• Code-Owners

• Code-Review

• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 45

Gerrit-Attention: Alison Maher <alm...@microsoft.com>

Gerrit-Comment-Date: Thu, 04 Sep 2025 16:44:00 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Alison Maher <alm...@microsoft.com>

[Sam Davis Omekara (Gerrit)]

Attention needed from Alison Maher

Sam Davis Omekara added 1 comment

File third_party/blink/renderer/core/layout/gap/gap_geometry.h

Line 180, Patchset 43: LayoutUnit GetGapCenterOffset(GridTrackSizingDirection direction,

Alison Maher . resolved

nit: might be worth a brief comment for this one, too

Sam Davis Omekara

Ditto, I created the

Sam Davis Omekara

Forgot to finish this comment lol. Added comment.

Open in Gerrit

Related details

Attention is currently required from:

• Alison Maher

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review
• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 46

Gerrit-Comment-Date: Thu, 04 Sep 2025 16:50:56 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Sam Davis Omekara <samome...@microsoft.com>

Comment-In-Reply-To: Alison Maher <alm...@microsoft.com>

[Alison Maher (Gerrit)]

Attention needed from Javier Contreras and Sam Davis Omekara

Alison Maher voted and added 9 comments

Votes added by Alison Maher

Code-Review

+1

9 comments

File third_party/blink/renderer/core/layout/flex/flex_layout_algorithm.cc

Line 801, Patchset 43: if (num_lines_ > 1 && !content_main_end_.has_value()) {

Alison Maher . unresolved

Will this not be guaranteed if `is_first_item` is true? (i.e. could this be a CHECK instead?)

Sam Davis Omekara

Was going to be on board but I think this code path hits for every first item in every line so the extra condition ensures we only set it once.

cc: @javi...@microsoft.com is this assumption right?

Alison Maher

Ah ok gotcha, if that's the case, then makes sense

File third_party/blink/renderer/core/layout/gap/gap_geometry.h

Line 288, Patchset 43: // this index as part of its calculation. Making this mutable allows us to
// maintain necessary state without breaking const-correctness for the overall
// GapGeometry object.
mutable wtf_size_t main_gap_running_index_ = kNotFound;

Alison Maher . resolved

Would it make sense for paint to hold this instead and pass it in as an input/output param wherever needed?

Sam Davis Omekara

I think that might work and make sense, but might need some restructuring. Are you okay landing as is while I explore that in a follow up change or you think it's worth pursuing now.

Alison Maher

Yeah a TODO works for me

File third_party/blink/renderer/core/layout/gap/gap_geometry.cc

Line 140, Patchset 43:LayoutUnit GapGeometry::GetGapCenterOffset(GridTrackSizingDirection direction,

Alison Maher . resolved

If it is the start, it won't be center, right? Should this be GetGapIntersectionOffset()? Not a strong preference though either way

Sam Davis Omekara

Yeahh that method and naming is so confusing. I'm creating a separate CL[1] that removes start from these offset's naming, I think this can be renamed to GetGapOffset with that landing.

[1]: https://chromium-review.googlesource.com/c/chromium/src/+/6915478

Alison Maher

Acknowledged

Line 161, Patchset 43:Vector<LayoutUnit> GapGeometry::GenerateIntersectionListForMain(

Alison Maher . resolved

Instead of building up a vector, would it make sense to have an iterator instead?

I guess maybe not because of the complexity of flexbox?

Sam Davis Omekara

I had explored this earlier a bit, but I think the iterator route imo was becoming a tad too complicated for something that could easily be represented as a list. More than happy to add as a TODO and explore that further later.

Alison Maher

Yeah I think given the code for flexbox, this seems fine for now

Line 343, Patchset 43: // * kBoth: Both first and last intersections are edges.
// * kStart: Only the first intersection is an edge.
// * kEnd: Only the last intersection is an edge.

Alison Maher . unresolved

Can we not just use these for all cases rather than diverging?

Sam Davis Omekara

No we can't use this for all cases because I for all main gaps the first regardless of edge state the first and last intersections are always edge intersections.

If your suggestion is to mark all main gaps kBoth(because technically they are) then I think we can use this for all cases. But rn, the EdgeGap state is only for cross gaps.

Alison Maher

Yeah my thinking was to mark the EdgeGap state for all cases, that way we only have one way of determining that state (I think that may make this code a bit simpler here).

Can be done in a follow up though

File third_party/blink/renderer/core/paint/gap_decorations_painter.cc

Line 27, Patchset 46 (Latest): GapDataList<StyleColor> rule_colors = track_direction == kForColumns

Alison Maher . unresolved

These spots also could make use of is_column_gap if we move that up a bit higher

Line 66, Patchset 46 (Latest): rule_color, style, /*is_column_rule=*/is_column_gap);

Alison Maher . unresolved

nit: this is no longer needed

File third_party/blink/renderer/core/paint/gap_painter.cc

Line 88, Patchset 43: wtf_size_t end = start;

Alison Maher . resolved

Why do we set this to start instead of directly to `last_intersection_index`?

Also I don't see end or last_intersection_index ever changing. Do we need both of these vars, or am I just overlooking something?

Sam Davis Omekara

I think the additional context here is that the code line below won't live for long (i.e. after the next CL). It was just a placeholder to allow us paint from the first intersection to the last one.

In the child CL we call the Adjust method that properly assigns start and end.

Alison Maher

Acknowledged

Line 93, Patchset 43: if (start >= end) {

Alison Maher . resolved

Is this ever going to happen because of the `while (start < last_intersection_index) {`?

Sam Davis Omekara

Similar to the above comment, I think the Adjust method in the next CL could see us land in a scenario where start>= end. Didn't work on this bit of logic recently but I vaguely remember this being an edge case in the way we adjust the start/end pair.

Alison Maher

Acknowledged

Open in Gerrit

Related details

Attention is currently required from:

• Javier Contreras
• Sam Davis Omekara

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Gerrit-Attention: Javier Contreras <javi...@microsoft.com>

Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>

Gerrit-Comment-Date: Thu, 04 Sep 2025 17:16:10 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Sam Davis Omekara (Gerrit)]

Sam Davis Omekara added 4 comments

File third_party/blink/renderer/core/layout/flex/flex_layout_algorithm.cc

Line 801, Patchset 43: if (num_lines_ > 1 && !content_main_end_.has_value()) {

Alison Maher . resolved

Will this not be guaranteed if `is_first_item` is true? (i.e. could this be a CHECK instead?)

Sam Davis Omekara

Was going to be on board but I think this code path hits for every first item in every line so the extra condition ensures we only set it once.

cc: @javi...@microsoft.com is this assumption right?

Alison Maher

Ah ok gotcha, if that's the case, then makes sense

Sam Davis Omekara

Done

File third_party/blink/renderer/core/layout/gap/gap_geometry.cc

Line 343, Patchset 43: // * kBoth: Both first and last intersections are edges.
// * kStart: Only the first intersection is an edge.
// * kEnd: Only the last intersection is an edge.

Alison Maher . resolved

Can we not just use these for all cases rather than diverging?

Sam Davis Omekara

No we can't use this for all cases because I for all main gaps the first regardless of edge state the first and last intersections are always edge intersections.

If your suggestion is to mark all main gaps kBoth(because technically they are) then I think we can use this for all cases. But rn, the EdgeGap state is only for cross gaps.

Alison Maher

Yeah my thinking was to mark the EdgeGap state for all cases, that way we only have one way of determining that state (I think that may make this code a bit simpler here).

Can be done in a follow up though

Sam Davis Omekara

Added a TODO, will work on the follow up in coming days.

File third_party/blink/renderer/core/paint/gap_decorations_painter.cc

Line 27, Patchset 46: GapDataList<StyleColor> rule_colors = track_direction == kForColumns

Alison Maher . resolved

These spots also could make use of is_column_gap if we move that up a bit higher

Sam Davis Omekara

Done

Line 66, Patchset 46: rule_color, style, /*is_column_rule=*/is_column_gap);

Alison Maher . resolved

nit: this is no longer needed

Sam Davis Omekara

Done

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 48

Gerrit-Comment-Date: Thu, 04 Sep 2025 18:20:28 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Blink W3C Test Autoroller (Gerrit)]

Message from Blink W3C Test Autoroller

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/54697.

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

Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>

Gerrit-Comment-Date: Thu, 04 Sep 2025 18:27:33 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: No

[Sam Davis Omekara (Gerrit)]

Sam Davis Omekara voted Commit-Queue+2

Commit-Queue

+2

Gerrit-Comment-Date: Thu, 04 Sep 2025 20:04:23 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Chromium LUCI CQ (Gerrit)]

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

46 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/renderer/core/paint/gap_decorations_painter.cc
Insertions: 11, Deletions: 15.

The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/core/layout/flex/flex_layout_algorithm.cc
Insertions: 1, Deletions: 1.

The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/core/layout/gap/gap_geometry.cc
Insertions: 18, Deletions: 13.

The diff is too large to show. Please review the diff.
```

Change information

Commit message:

[Gap Decorations]: Major paint logic for Optimized GapGeometry

This CL implements the core paint logic for the optimized approach to
gap geometry. The main changes are in the "population" methods in
`gap_geometry.h` and the addition of the new `GapPainter` class in
`gap_painter.h`.

Gap decorations are painted relative to intersections within a gap, as a
result this change generates these intersection points at paint time
rather than storing precomputed intersections from layout. The general
principle for generating intersection points is:

content-start → intersections between main × cross gaps → content-end

However, the way we compute the middle intersections depends on the
container type. For flex cross gaps, there are only two intersections
(start and end), so the pattern doesn’t fully apply. This distinction
requires calculating the end offset for flex cross gaps dynamically.

The code is exercised through existing web tests in the
virtual/css-gap-decorations-optimized/external/wpt/css/css-gaps/ folder
as all changes are behind the CSSGapDecorationOptimized feature flag.

Note: This CL does not yet handle rule-break behavior as a result tests
that are exercising that logic are marked as failing in this CL.
Currently, decorations are painted from the start to the end of a gap
without interruption. Rule-break handling will be addressed in an
upcoming change.

Other changes made in this CL are:
* For flex:
- Previously, ContentStart and ContentEnd offsets were not set when a flex line contained only a single item. This has been corrected.
- The offset for the main gap no longer needs to be adjusted based on flex direction. It now consistently represents the position where the flex line ends plus the gap. (Relevant test updated accordingly.)
* For `MainGap` and `CrossGapRange`:
- Introduced getters and setters for `CrossGapRange`, improving ease of use during paint time.
* For grid tests:
- Updated test references to use standard div elements instead of relying on column rules from multi-column layout.

Bug: 436140061, 357648037

Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6848348

Reviewed-by: Alison Maher <alm...@microsoft.com>

Commit-Queue: Sam Davis Omekara <samome...@microsoft.com>

Cr-Commit-Position: refs/heads/main@{#1511107}

Files:

• M third_party/blink/renderer/core/css/css_gap_decoration_property_utils.cc
• M third_party/blink/renderer/core/css/css_gap_decoration_property_utils.h
• M third_party/blink/renderer/core/layout/flex/flex_layout_algorithm.cc
• M third_party/blink/renderer/core/layout/flex/flex_layout_algorithm_test.cc
• M third_party/blink/renderer/core/layout/gap/cross_gap.h
• M third_party/blink/renderer/core/layout/gap/gap_geometry.cc
• M third_party/blink/renderer/core/layout/gap/gap_geometry.h
• M third_party/blink/renderer/core/layout/gap/main_gap.h
• M third_party/blink/renderer/core/layout/grid/grid_layout_algorithm.cc
• M third_party/blink/renderer/core/paint/box_fragment_painter.cc
• M third_party/blink/renderer/core/paint/build.gni
• A third_party/blink/renderer/core/paint/gap_decorations_painter.cc
• A third_party/blink/renderer/core/paint/gap_decorations_painter.h
• M third_party/blink/web_tests/TestExpectations
• M third_party/blink/web_tests/external/wpt/css/css-gaps/grid/grid-gap-decorations-003-ref.html
• M third_party/blink/web_tests/external/wpt/css/css-gaps/grid/grid-gap-decorations-004-ref.html
• M third_party/blink/web_tests/external/wpt/css/css-gaps/grid/grid-gap-decorations-005-ref.html

Change size: L

Delta: 17 files changed, 750 insertions(+), 119 deletions(-)

Branch: refs/heads/main

Submit Requirements:

• Code-Review: +1 by Alison Maher

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: merged

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I0a0eadbad7f4911a98ae22060327ef1acc526ab3

Gerrit-Change-Number: 6848348

Gerrit-PatchSet: 49

Gerrit-Owner: Sam Davis Omekara <samome...@microsoft.com>

Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>

Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Ian Kilpatrick <ikilp...@chromium.org>

Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>

Gerrit-Reviewer: Kurt Catti-Schmidt <ksc...@microsoft.com>

Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>

Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>

Gerrit-CC: Akihiro Ota <akihi...@chromium.org>

Gerrit-CC: Ale Bzk <ales...@chromium.org>

Gerrit-CC: Alexis Menard <alexis...@intel.com>

Gerrit-CC: Andrew Rayskiy <green...@google.com>

Gerrit-CC: Andrew Williams <awi...@chromium.org>

Gerrit-CC: Arnaud Mandy <arnaud...@intel.com>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>

Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>

Gerrit-CC: CJ DiMeglio <lethala...@chromium.org>

Gerrit-CC: Christian Biesinger <cbies...@chromium.org>

[Blink W3C Test Autoroller (Gerrit)]

Message from Blink W3C Test Autoroller

The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/54697

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Comment-Date: Thu, 04 Sep 2025 20:44:12 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: No