[webkit-reviews] review canceled: [Bug 91638] [css3-text] Add support for text-decoration-color : [Attachment 163156] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Bruno Abinader
<bruno.abinader at basyskom.com>'s request for review:
Bug 91638: [css3-text] Add support for text-decoration-color
https://bugs.webkit.org/show_bug.cgi?id=91638

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

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


> Source/WebCore/ChangeLog:43
> +	   Text decoration color does not fallback in colorIncludingFallback
and
> +	   might return an invalid color in visitedDependentColor (checked in
> +	   decorationColor).

Explaining the 'why' here would help. I can't read minds, only what you have
written, and they don't give my any indication as to why this is a valid
assumption to make.

AFAICT there is no reason why 'text-decoration' shouldn't fallback to using
'color' like other text colors (see
http://www.w3.org/TR/2011/REC-css3-color-20110607/#foreground). Consistency
between the different text color properties is important (unless obvious the
spec says otherwise).

> Source/WebCore/rendering/RenderObject.cpp:2698
> +#if ENABLE(CSS3_TEXT_DECORATION)
> +    // Check for text decoration color first.
> +    Color result =
style->visitedDependentColor(CSSPropertyWebkitTextDecorationColor);
> +    if (result.isValid())
> +	   return result;
> +#else
>      Color result;
> +#endif // CSS3_TEXT_DECORATION

How about:

Color result;
#if ENABLE(CSS3_TEXT_DECORATION)
result = ...;
[snip]
#endif
(less lines and less #else)

Also what happened to the alpha() check?

The comment would be better if you explained 'why' you need to check text
decoration first. As it is written, it adds little value.