[webkit-reviews] review granted: [Bug 113546] [CSS Grid Layout] Align our grid-line handling with the updated specification : [Attachment 196033] Proposed change 2: Updated to better match Tab's answers.

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 113546: [CSS Grid Layout] Align our grid-line handling with the updated
specification
https://bugs.webkit.org/show_bug.cgi?id=113546

Attachment 196033: Proposed change 2: Updated to better match Tab's answers.
https://bugs.webkit.org/attachment.cgi?id=196033&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=196033&action=review


> Source/WebCore/ChangeLog:10
> +	   to always resolve against the start/before edge (the previous code
would resolve end/after
> +	   grid lines resolve against the end/after edge).

I think you have one too many resolves here.

> Source/WebCore/rendering/RenderGrid.cpp:776
> +	   // FIXME: This returns the wrong result for side == StartSide or
BeforeSide (off-by-one) but
> +	   // that avoids the issue of growing the grid due to e.g. -1 / auto.

I don't really understand this comment. Can you explain it a bit more? Here or
in the ChangeLog is fine.

> Source/WebCore/rendering/RenderGrid.cpp:778
> +	   // Per
http://lists.w3.org/Archives/Public/www-style/2013Mar/0589.html, we should

Typically, we put links to email threads or specs in the ChangeLog. It'll be
there in the blame history should someone try to understand why these lines
were added. I don't think this line really requires an explanatory comment in
addition to the explanation in the ChangeLog.

In the limit, nearly every line of code in this file could point to a bit of
the specification, which would obviously be unwieldy.