[webkit-reviews] review granted: [Bug 236287] Lookup line names for sub grids using line names from ancestor grids : [Attachment 451961] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Matt Woodrow
<mattwoodrow at apple.com>'s request for review:
Bug 236287: Lookup line names for sub grids using line names from ancestor
grids
https://bugs.webkit.org/show_bug.cgi?id=236287

Attachment 451961: Patch

https://bugs.webkit.org/attachment.cgi?id=451961&action=review




--- Comment #10 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 451961
  --> https://bugs.webkit.org/attachment.cgi?id=451961
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451961&action=review

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:74
> +    if (side == ColumnStartSide)
> +	   return RowStartSide;
> +    if (side == ColumnEndSide)
> +	   return RowEndSide;
> +    if (side == RowStartSide)
> +	   return ColumnStartSide;
> +    return ColumnEndSide;

switch?

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:80
> +    NamedGridAreaMap::const_iterator gridAreaIt = areas.find(name);

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:83
> +	   const GridArea& gridArea = gridAreaIt->value;
> +	   GridSpan gridSpan = isRowAxis ? gridArea.columns : gridArea.rows;

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:88
> +    return -1;

Rather than returning -1 on failure, use a std::optional<int> return value

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:95
> +    GridTrackSizingDirection direction = directionFromSide(side);

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:97
> +    const RenderGrid* grid = &initialGrid;
> +    const RenderStyle* gridContainerStyle = &grid->style();

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:105
> +    const NamedGridLinesMap& gridLineNames = isRowAxis ?
gridContainerStyle->namedGridColumnLines() :
gridContainerStyle->namedGridRowLines();
> +    const NamedGridLinesMap& autoRepeatGridLineNames = isRowAxis ?
gridContainerStyle->autoRepeatNamedGridColumnLines() :
gridContainerStyle->autoRepeatNamedGridRowLines();
> +    const NamedGridLinesMap& implicitGridLineNames = isRowAxis ?
gridContainerStyle->implicitNamedGridColumnLines() :
gridContainerStyle->implicitNamedGridRowLines();
> +

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:133
> +	   // subgrid might have inherited less tracks than needed to cover the
specified area. We want to clamp

fewer tracks

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:174
> +void NamedLineCollectionBase::ensureLocalStorage()

"local storage" has a different meaning on the web. Maybe ensureNamedIndices()
or something.

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:176
> +    if (m_implicitNamedLinesIndexes != &m_inheritedNamedLinesIndexes) {

nit: indexes -> indices

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:202
> +	   unsigned autoRepeatIndexInFirstRepetition = (line -
m_insertionPoint) % m_autoRepeatTrackListLength;

Can m_autoRepeatTrackListLength be 0?

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:230
> +    GridSpan search = GridSpan::translatedDefiniteGridSpan(0, m_lastLine);
> +    GridPositionSide currentSide = side;
> +    GridTrackSizingDirection direction = directionFromSide(currentSide);

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:237
> +	   const RenderGrid* parent = downcast<RenderGrid>(grid->parent());

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:247
> +	   GridSpan span = parent->gridSpanForChild(*grid, direction);

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:253
> +	       i -= search.startLine();
> +	       if (GridLayoutFunctions::isFlippedDirection(*parent, direction)
!= initialFlipped)
> +		   i = m_lastLine - i;

Hopefully these can't underflow?

> Source/WebCore/rendering/style/GridPositionsResolver.h:68
> +    bool m_subgrid;

This should have "is" or "has" in the name. Auto initialize it here: bool
m_isSubgrid { false }. Same for the other members.

> Source/WebCore/rendering/style/GridPositionsResolver.h:81
> +    unsigned lastLine() const;

Maybe it's clear that a "line" here is really a "line index"?
I'm not sure what a "position" is in this context.