[webkit-reviews] review granted: [Bug 99439] [CSS3] Parsing the property, text-align-last. : [Attachment 172497] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Dongwoo Joshua Im (dwim)
<dw.im at samsung.com>'s request for review:
Bug 99439: [CSS3] Parsing the property, text-align-last.
https://bugs.webkit.org/show_bug.cgi?id=99439

Attachment 172497: Patch
https://bugs.webkit.org/attachment.cgi?id=172497&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=172497&action=review


Please don't forget to regenerate the ChangeLog after the removal and don't
downplay its importance (ie it needs to be filled with important information).

> Source/WebCore/ChangeLog:9
> +	   This patch implements the "text-align-last" property specified in
CSS3
> +	   working draft, with "-webkit-" prefix, under ENABLE_CSS3_TEXT flag.

This is inaccurate: you implement only the parsing side of "text-align-last".

> Source/WebCore/ChangeLog:45
> +	   (StyleRareInheritedData):

Please fill these entries too.

> Source/WebCore/css/StyleBuilder.cpp:1158
> +#if ENABLE(CSS3_TEXT)
> +class ApplyPropertyTextAlignLast {
> +public:
> +    static void applyInheritValue(CSSPropertyID, StyleResolver*
styleResolver)
> +    {
> +	  
styleResolver->style()->setTextAlignLast(styleResolver->parentStyle()->textAlig
nLast());
> +    }
> +
> +    static void applyInitialValue(CSSPropertyID, StyleResolver*
styleResolver)
> +    {
> +	  
styleResolver->style()->setTextAlignLast(RenderStyle::initialTextAlignLast());
> +    }
> +
> +    static void applyValue(CSSPropertyID, StyleResolver* styleResolver,
CSSValue* value)
> +    {
> +	   if (!value->isPrimitiveValue())
> +	       return;
> +
> +	   CSSPrimitiveValue* primitiveValue =
static_cast<CSSPrimitiveValue*>(value);
> +	   styleResolver->style()->setTextAlignLast(*primitiveValue);
> +    }
> +
> +    static PropertyHandler createHandler() { return
PropertyHandler(&applyInheritValue, &applyInitialValue, &applyValue); }
> +
> +};
> +#endif // CSS3_TEXT

This is dead code now. Please remove it.

> Source/WebCore/rendering/style/RenderStyleConstants.h:351
> +    TextAlignLastAUTO, TextAlignLastSTART, TextAlignLastEND,
TextAlignLastLEFT, TextAlignLastRIGHT, TextAlignLastCENTER,
TextAlignLastJUSTIFY

This is not proper camel-case. Please don't capitalize the last word as it
doesn't match WebKit style.

>
LayoutTests/fast/css3-text/css3-text-align-last/getComputedStyle/script-tests/g
etComputedStyle-text-align-last.js:3
> +    if (type != null) {

This check always passes AFAICT and you never check against null.

> LayoutTests/platform/gtk/TestExpectations:45
> +webkit.org/b/76173 fast/css3-text/css3-text-align-last [ Missing ]

This line is wrong. [Missing] is about missing expected files which you have
here. It should be [ Skipped ] or nothing (they are equivalent). It should also
be located with the other fast/css3-text entry like the other changes.