[webkit-reviews] review denied: [Bug 102491] [css3-text] Add parsing support for text-underline-position : [Attachment 189797] Parse text-underline-position

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Lamarque V. Souza
<Lamarque.Souza at basyskom.com>'s request for review:
Bug 102491: [css3-text] Add parsing support for text-underline-position
https://bugs.webkit.org/show_bug.cgi?id=102491

Attachment 189797: Parse text-underline-position
https://bugs.webkit.org/attachment.cgi?id=189797&action=review

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


You are on the right tracks. This will need another round as you allow cases
that are not compliant (and not tested).

> Source/WebCore/ChangeLog:24
> +	   (WebCore):

These entries are not useful and should be removed.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1361
> +    if (list->length())
> +	   return list;
> +
> +    ASSERT_NOT_REACHED();
> +    return cssValuePool().createExplicitInitialValue();

We should probably just have:

ASSERT(list->length());
return list;

> Source/WebCore/css/CSSParser.cpp:9100
> +	   m_valueList->next();

Do you need this?

> Source/WebCore/css/CSSParser.cpp:9104
> +    if (value->id == CSSValueAlphabetic) {

This could be added to the 'if' above.

> Source/WebCore/css/CSSParser.cpp:9106
> +	   m_valueList->next();

Ditto.

> Source/WebCore/css/CSSParser.cpp:9111
> +    bool isValid = true;

I don't think we need this |isValid| variable: if you are not valid, I would
expect you to bail out from the function as there is no point in continuing.

> Source/WebCore/css/CSSParser.cpp:9113
> +    while (isValid && value) {

We could probably get away without the loop here as we really expect to parse
only 2 values but in any order.

> Source/WebCore/css/CSSParser.cpp:9132
> +

You would wrongly accept:
* -webkit-text-underline-position: under under;
* -webkit-text-underline-position: under under under;
* -webkit-text-underline-position: left under under;

These cases should be added to your test.

> Source/WebCore/css/CSSValueKeywords.in:949
> +#if !(defined(ENABLE_SVG) && ENABLE_SVG) && defined(ENABLE_CSS3_TEXT) &&
ENABLE_CSS3_TEXT

It's probably a bad idea to have the same property defined in 2 places. I would
rather see it defined here only.

> Source/WebCore/css/CSSValueKeywords.in:952
> +#endif // CSS3_TEXT

Nit: No need for the "// CSS3_TEXT", especially since it's wrong.

> Source/WebCore/css/StyleBuilder.cpp:1960
> +    setPropertyHandler(CSSPropertyWebkitTextUnderlinePosition,
ApplyPropertyTextUnderlinePosition::createHandler());

If you handle the value here (which is fine), there is a giant switch to update
in StyleResolver.cpp to say that you do so. Unfortunately, it doesn't break the
build because of SVG defining the 'default' case.

> Source/WebCore/rendering/style/RenderStyleConstants.h:368
> +inline TextUnderlinePosition operator| (TextUnderlinePosition a,
TextUnderlinePosition b) { return TextUnderlinePosition(int(a) | int(b)); }
> +inline TextUnderlinePosition& operator|=(TextUnderlinePosition& a,
TextUnderlinePosition b) { return a = a | b; }

Not a huge fan of these operators for 2 reasons:
* What you get is *not* a TextUnderlinePosition as it isn't a defined value.
* This doesn't match our current style, which is to use flags.

It's a smart way of handling this situation but I think a simple

typedef unsigned TextUnderlinePositionFlags;
(or a plain unsigned)

would do a better job.

>
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/g
etComputedStyle-text-underline-position.js:99
> +

Missing the bad cases: left left, left right, right right.