[webkit-reviews] review granted: [Bug 103343] [CSS Grid Layout] track sizing functions should have their own type : [Attachment 176530] Change 2: Renamed the new structure to TrackSize.

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 103343: [CSS Grid Layout] track sizing functions should have their own type
https://bugs.webkit.org/show_bug.cgi?id=103343

Attachment 176530: Change 2: Renamed the new structure to TrackSize.
https://bugs.webkit.org/attachment.cgi?id=176530&action=review

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


I think GridTrackSize would be a bit better than TrackSize, but I guess it's
not a big deal.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:996
> +static PassRefPtr<CSSValue> valueForGridTrackList(const Vector<TrackSize>&
trackFunctions, const RenderStyle* style)

Nit: trackFunctions -> trackSizes

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1000
> -    if (trackLengths.size() == 1 && trackLengths[0].isUndefined())
> +    if (!trackFunctions.size())
>	   return cssValuePool().createIdentifierValue(CSSValueNone);

Can we add a test case for 0 tracks?

> Source/WebCore/css/StyleResolver.cpp:2662
> +static bool createGridTrackBreadth(CSSPrimitiveValue* primitiveValue,
StyleResolver* selector, TrackSize& function)

Nit: function -> trackSize

> Source/WebCore/css/StyleResolver.cpp:2675
> +static bool createGridTrackList(CSSValue* value, Vector<TrackSize>&
functions, StyleResolver* selector)

Nit: functions -> sizes or trackSizes

> Source/WebCore/css/StyleResolver.cpp:2690
> +	       TrackSize function;
> +	       if
(!createGridTrackBreadth(static_cast<CSSPrimitiveValue*>(currValue), selector,
function))

Nit: function -> trackSize

> Source/WebCore/rendering/style/StyleGridData.h:54
> +    Vector<TrackSize> m_gridColumns;
> +    Vector<TrackSize> m_gridRows;

Maybe we should rename this to m_gridColumnTracks or something?