[webkit-reviews] review granted: [Bug 172707] [css-align][css-flex][css-grid] 'auto' values of align-self and justify-self must not be resolved : [Attachment 311807] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Javier Fernandez
<jfernandez at igalia.com>'s request for review:
Bug 172707: [css-align][css-flex][css-grid] 'auto' values of align-self and
justify-self must not be resolved
https://bugs.webkit.org/show_bug.cgi?id=172707

Attachment 311807: Patch

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




--- Comment #43 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 311807
  --> https://bugs.webkit.org/attachment.cgi?id=311807
Patch

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

This cleans up things nicely, r=me

> Source/WebCore/rendering/RenderFlexibleBox.cpp:249
> +	   for (auto* child = firstChildBox(); child; child =
child->nextSiblingBox()) {

You should be able to use

    for (auto& child : childrenOfType<RenderBox>(*this)) {

> Source/WebCore/rendering/RenderFlexibleBox.cpp:250
> +	       ItemPosition previousAlignment
=child->style().resolvedAlignSelf(oldStyle,
selfAlignmentNormalBehavior()).position();

missing space after =

> Source/WebCore/rendering/RenderGrid.cpp:96
> +StyleSelfAlignmentData RenderGrid::selfAlignmentForChild(GridAxis axis,
const RenderBox& child, const RenderStyle* style) const

Why is the style a pointer? I don't see this being called with null style.

> Source/WebCore/rendering/RenderGrid.cpp:128
> +	   for (RenderBox* child = firstChildBox(); child; child =
child->nextSiblingBox()) {

Here too

> Source/WebCore/rendering/RenderGrid.cpp:1205
> +StyleSelfAlignmentData RenderGrid::alignSelfForChild(const RenderBox& child,
const RenderStyle* style) const

It is bit unclear whose style the 'style' is and when it is ok to call this
will null style.

> Source/WebCore/rendering/RenderGrid.cpp:1212
> +StyleSelfAlignmentData RenderGrid::justifySelfForChild(const RenderBox&
child, const RenderStyle* style) const

Same here.