[Webkit-unassigned] [Bug 133359] [CSS Grid Layout] Upgrade align-self and align-items parsing to CSS 3

[bugzilla-daemon at webkit.org]

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





--- Comment #22 from Benjamin Poulain <benjamin at webkit.org>  2014-07-30 21:15:29 PST ---
(From update of attachment 235775)
View in context: https://bugs.webkit.org/attachment.cgi?id=235775&action=review

I only had a quick look :(
Some comments:

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1650
> +static ItemPosition resolveAlignmentAuto(ItemPosition position, Node* element)
> +{
> +    if (position != ItemPositionAuto || !element || !element->computedStyle())
> +        return position;
> +
> +    return element->computedStyle()->isDisplayFlexibleOrGridBox() ? ItemPositionStretch : ItemPositionStart;
> +}

I don't understand this little dance with the computed style. You are using this from ComputedStyleExtractor, you should not get the style directly from the element, you take the style as an argument from the ComputedStyleExtractor.

The one taking the parent node is another problem, in that case you need to test "element" and "element->computedStyle()".

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1657
> +    result->append(CSSPrimitiveValue::create(itemPosition));
> +    if (itemPosition >= ItemPositionCenter && overflowAlignment != OverflowAlignmentDefault)
> +        result->append(CSSPrimitiveValue::create(overflowAlignment));

Why is this code avoiding the Value Pool?

> Source/WebCore/css/CSSParser.cpp:3085
> +static bool isBaselinePositionKeyword(CSSValueID id)

static inline

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1305
> +                // FIXME: File a bug about implementing that. The extended grammar
> +                // is not enabled by default so we shouldn't hit this codepath.

File the bug, put the bugzilla number here :)

> Source/WebCore/rendering/style/RenderStyle.h:1583
> +

Blank line.

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