[webkit-reviews] review denied: [Bug 90959] [css3-text] Add support for text-decoration-line : [Attachment 156829] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Bruno Abinader
<bruno.abinader at basyskom.com>'s request for review:
Bug 90959: [css3-text] Add support for text-decoration-line
https://bugs.webkit.org/show_bug.cgi?id=90959

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=156829&action=review


> Source/WebCore/ChangeLog:9
> +	   working draft, with "-webkit-" prefix. The specification can be
found below:

as agreed upon?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1194
> +    // Blink value is ignored

commends ends with a punctuation mark of some kind. (just add a dot :-)) It
would be better if you said why.

> Source/WebCore/css/CSSParser.cpp:2099
>	   // none | [ underline || overline || line-through || blink ] |
inherit

he it says blink!

> Source/WebCore/css/CSSParser.cpp:2100
> +	   // Note: blink value is not accepted by -webkit-text-decoration-line


Here you note it is not accepted?

> Source/WebCore/css/CSSParser.cpp:7911
> +	   // Skip if -webkit-text-decoration-line was previously set

+ .

Was set previously. (I would say)

> Source/WebCore/css/CSSParser.cpp:7914
> +	   for (unsigned i = 0; i < m_parsedProperties.size(); ++i)
> +	       if (m_parsedProperties[i].id() ==
CSSPropertyWebkitTextDecorationLine)
> +		   return;

That for loop has 2 actual lines as the content (not logical lines) so it must
have braces.

Is this how it is done elsewhere? It seems like a common thing to check for

> Source/WebCore/css/CSSParser.cpp:7932
> +	   case CSSValueBlink:
> +	       isValid = propId != CSSPropertyWebkitTextDecorationLine;

Maybe the comment is more valuable here