[webkit-reviews] review granted: [Bug 104818] [CSS Grid Layout] Include paddings and borders into the grid element's logical height / width : [Attachment 179062] Proposed change.

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 104818: [CSS Grid Layout] Include paddings and borders into the grid
element's logical height / width
https://bugs.webkit.org/show_bug.cgi?id=104818

Attachment 179062: Proposed change.
https://bugs.webkit.org/attachment.cgi?id=179062&action=review

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


> Source/WebCore/rendering/RenderGrid.cpp:203
> +    LayoutUnit borderAndPaddingInBlockDirection =
borderAndPaddingLogicalHeight();
> +    setLogicalHeight(logicalHeight() + borderAndPaddingInBlockDirection);

Nit: I would remove the temporary variable.

> LayoutTests/fast/css-grid-layout/grid-element-padding-margin.html:9
> +.grid {
> +    display: -webkit-grid;

We should consider making a .css file in resources that define classes for the
various grid properties.  This makes it easier to handle property name changes
and makes it possible to make the tests cross platform by adding -ms prefixed
properties.  See css3/flexbox/resources/flexbox.css for an example of this. 
Would be good as a separate patch.