[webkit-reviews] review granted: [Bug 204578] [css-grid] Failures on css/css-grid/grid-model/grid-box-sizing-001.html : [Attachment 418866] Patch

[bugzilla-daemon at webkit.org]

Javier Fernandez <jfernandez at igalia.com> has granted zsun at igalia.com's request
for review:
Bug 204578: [css-grid] Failures on
css/css-grid/grid-model/grid-box-sizing-001.html
https://bugs.webkit.org/show_bug.cgi?id=204578

Attachment 418866: Patch

https://bugs.webkit.org/attachment.cgi?id=418866&action=review




--- Comment #7 from Javier Fernandez <jfernandez at igalia.com> ---
Comment on attachment 418866
  --> https://bugs.webkit.org/attachment.cgi?id=418866
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418866&action=review

r=me

> Source/WebCore/ChangeLog:14
> +

Nit: sometimes it helps to add a line here to describe the change in this file.

> Source/WebCore/rendering/RenderGrid.cpp:196
> +	   bool hasDefiniteLogicalHeight =
RenderBlock::hasDefiniteLogicalHeight() || hasOverridingLogicalHeight() ||
computeContentLogicalHeight(MainOrPreferredSize, style().logicalHeight(),
WTF::nullopt);

Maybe we could add here  FIXME comment to suggest the idea of caching the
hasDefiniteLogicalHeight if this new call causes a relevant performance
regression.

> Source/WebCore/rendering/RenderGrid.cpp:253
> +

Remove this empty line, please.

> LayoutTests/imported/w3c/ChangeLog:8
> +	   *
web-platform-tests/css/css-grid/grid-model/grid-box-sizing-001-expected.txt:

You could mention here that some test cases pass now thanks to this change.