[Webkit-unassigned] [Bug 134419] [CSS Grid Layout] Implement justify-self css property

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=134419


Dean Jackson <dino at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #234103|review?                     |review+
               Flag|                            |




--- Comment #20 from Dean Jackson <dino at apple.com>  2014-07-03 14:57:28 PST ---
(From update of attachment 234103)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.