[webkit-reviews] review denied: [Bug 104700] [CSS Grid Layout] Implement grid items sizing for fixed minmax grid tracks : [Attachment 178869] Proposed implementation: Feedback seeked on whether testing logical width and logical height independently is sufficient.

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 104700: [CSS Grid Layout] Implement grid items sizing for fixed minmax grid
tracks
https://bugs.webkit.org/show_bug.cgi?id=104700

Attachment 178869: Proposed implementation: Feedback seeked on whether testing
logical width and logical height independently is sufficient.
https://bugs.webkit.org/attachment.cgi?id=178869&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=178869&action=review


Mac failure is real:

/Volumes/Data/EWS/WebKit/Source/WebCore/rendering/RenderGrid.cpp:192:63: error:
unused parameter 'direction' [-Werror,-Wunused-parameter]
void RenderGrid::distributeSpaceToTracks(TrackSizingDirection direction,
Vector<GridTrack>& tracks, LayoutUnit availableLogicalSpace)

> Source/WebCore/ChangeLog:9
> +	   to resolve any sizing function that doesn't size based on the
content (mincontent, maxcontent).

Nit: min-content, max-content.

> Source/WebCore/rendering/RenderGrid.cpp:125
> +	   const Length& minTrackLength = trackStyles[i].minTrackBreadth();
> +	   const Length& maxTrackLength = trackStyles[i].maxTrackBreadth();

I think most of the time we don't bother with using a const reference since
Length is pretty small (actually, looks like it's trying to be small, but
failing-- I'll fix that in a separate patch).

> Source/WebCore/rendering/RenderGrid.cpp:155
> +	   const Length& minTrackBreadth = trackStyles[i].minTrackBreadth();
> +	   const Length& maxTrackBreadth = trackStyles[i].maxTrackBreadth();

Nit: Maybe drop the const &.

> Source/WebCore/rendering/RenderGrid.cpp:160
> +	   // Ignore the max track breadth if it is smaller than the min track
breadth.

Remove this comment. It describe's the code and doesn't answer 'why'.

> Source/WebCore/rendering/RenderGrid.cpp:162
> +	   if (track.m_maxBreadth < track.m_usedBreadth)
> +	       track.m_maxBreadth = track.m_usedBreadth;

Nit: This could be track.m_maxBreadth = std::max(track.m_maxBreadth,
track.m_usedBreadth).

> Source/WebCore/rendering/RenderGrid.cpp:179
> +    // FIXME: We stil need to support calc() here (bug 103761).

Nit: stil -> still

> Source/WebCore/rendering/RenderGrid.cpp:187
> +bool RenderGrid::sortByGridTrackGrowthPotential(GridTrack* track1,
GridTrack* track2)

Does this function need to be part of RenderGrid?  I would declare it static to
the cpp file.

> Source/WebCore/rendering/RenderGrid.cpp:194
> +    const unsigned tracksSize = tracks.size();

size_t and I would drop the const.  Most other uses of const are for global
values.

> Source/WebCore/rendering/RenderGrid.cpp:196
> +    for (unsigned i = 0; i < tracksSize; ++i)

size_t

> Source/WebCore/rendering/RenderGrid.cpp:201
> +    for (unsigned i = 0; i < tracksSize; ++i) {

size_t