[webkit-reviews] review granted: [Bug 103335] [CSS Grid Layout] Support all specified breadth size : [Attachment 177286] breadth-sizes.diff

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Xan Lopez
<xan.lopez at gmail.com>'s request for review:
Bug 103335: [CSS Grid Layout] Support all specified breadth size
https://bugs.webkit.org/show_bug.cgi?id=103335

Attachment 177286: breadth-sizes.diff
https://bugs.webkit.org/attachment.cgi?id=177286&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=177286&action=review


> LayoutTests/ChangeLog:3
> +	   [CSS Grid Layout] Support all specified breadth size

We should probably rename the title to cover only <percentage> and viewport
relative sizes.

> LayoutTests/ChangeLog:14
> +	   * fast/css-grid-layout/resources/grid-columns-rows-get-set.js: test
the new breatdth length types.

Let's add a test for the multiple entry cases to make sure we support it
properly too!

> LayoutTests/fast/css-grid-layout/breadth-size-resolution-grid.html:69
> +

You should probably keep some orthogonal writing mode case. Here the grid
element and the grid items always have the same writing mode.

Here is an example of orthogonal writing mode:
<div class="grid">
     <div id="a" class="verticalRL" data-expected-width="100"
data-expected-height="60"></div>
     <div id="b" data-expected-width="100" data-expected-height="60"></div>
     <div id="c" class="verticalRL" data-expected-width="100"
data-expected-height="100"></div>
     <div id="d" data-expected-width="80" data-expected-height="100"></div>
 </div>

(#a and #c have an orthogonal writing mode with respect to their parent)

> LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js:3
>  debug("Test getting |display| set through CSS");

This is wrong btw, we should change it while we are touching this code:

debug("Test getting -webkit-grid-columns and -webkit-grid-rows through CSS.");

> LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js:28
> +var gridWithFitContentElement =
document.getElementById("gridWithFitContentElement");

Adding a debug(...) comment about why those are not valid would be nice as it
would make the output more readable. For example:

debug("Test getting wrong values for -webkit-grid-columns and -webkit-grid-rows
through CSS (they should resolve to the default: 'none'.");

> Source/WebCore/ChangeLog:10
> +	   types. calc() support is still missing, we'll do it in a follow-up
> +	   patch.

Would be nice to give the *why* here: calc() support is postponed to a
follow-up bug as it seems to require more work to be properly serialized in
CSSComputedStyleDeclaration.

> Source/WebCore/ChangeLog:13
> +	   (WebCore::valueForGridTrackBreadth): support the new types.

Sentences starts with a capital letter (this applies to all entries).

> Source/WebCore/ChangeLog:14
> +	   (WebCore::valueForGridTrackList): modify the call to the upper

s/upper/above/ (upper doesn't sound right here but I am not a native speaker so
I could be wrong).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1002
> +static PassRefPtr<CSSValue> valueForGridTrackBreadth(const GridTrackSize&
trackSize, const RenderStyle* style, RenderView *renderView)

This could be a const RenderView* if valueForLength wasn't dumb (bug worthy
though)

> Source/WebCore/rendering/RenderGrid.cpp:145
> +	   if (trackLength.isFixed() || trackLength.isPercent() ||
trackLength.isViewportPercentage())

Let's add a FIXME about supporting calc.