[webkit-reviews] review granted: [Bug 134419] [CSS Grid Layout] Implement justify-self css property : [Attachment 234103] Fixed root cause of the canvas-color test failure.

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Javier Fernandez
<jfernandez at igalia.com>'s request for review:
Bug 134419: [CSS Grid Layout] Implement justify-self css property
https://bugs.webkit.org/show_bug.cgi?id=134419

Attachment 234103: Fixed root cause of the canvas-color test failure.
https://bugs.webkit.org/attachment.cgi?id=234103&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234103&action=review


> Source/WebCore/css/CSSParser.cpp:1371
> -    color = document->page()->theme().systemColor(id).rgb();
> -    return true;
> +    Color parsedColor = document->page()->theme().systemColor(id);
> +    color = parsedColor.rgb();
> +    return parsedColor.isValid();

This seems fine, but I'm quite concerned that there was a bug that only
manifests with this patch applied and that we can't reproduce normally.

> Source/WebCore/css/CSSParser.cpp:3123
> +	       if (value->id == CSSValueTrue || value->id == CSSValueSafe)
> +		   overflowAlignmentKeyword =
cssValuePool().createIdentifierValue(value->id);
> +	       else
> +		   return false;

Please write this the other way around, as an early return.

if (id != True && id != Safe)
  return false;

overflowAlignmentKeyword = ...

> Source/WebCore/css/CSSParser.cpp:3132
> +	       if (isItemPositionKeyword(value->id))
> +		   position = cssValuePool().createIdentifierValue(value->id);
> +	       else
> +		   return false;

Same here.

> Source/WebCore/css/CSSParser.cpp:3144
> +	   addProperty(propId, createPrimitiveValuePair(position,
overflowAlignmentKeyword), important);
> +    else
> +	   addProperty(propId, position.release(), important);

I assume createPrimitiveValuePair releases position? Do you even need to do
this?

> Source/WebCore/css/CSSValueKeywords.in:563
> +
> +

Nit: double space