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

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied 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 162491: Patch
https://bugs.webkit.org/attachment.cgi?id=162491&action=review

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


r- for the code duplication but the patch is fine apart from that.

> Source/WebCore/rendering/RenderObject.cpp:2693
> +    return
style->visitedDependentColor(CSSPropertyWebkitTextDecorationColor);

This line made me think a long time about whether it was correct. It turns out
that it is but it's not the proper way of adding what you want.

> Source/WebCore/rendering/style/RenderStyle.cpp:1397
> +    // Text decoration color has a four-way fallback (text decoration color,
then text stroke color
> +    // if stroke width is non-zero or text fill color, which falls back to
color if invalid.
> +    if (colorProperty == CSSPropertyWebkitTextDecorationColor) {
> +	   if (result.alpha() && result.isValid())
> +	       return result;
> +	   if (textStrokeWidth() > 0) {
> +	       // Prefer stroke color if possible but not if it's fully
transparent.
> +	       result =
colorIncludingFallback(CSSPropertyWebkitTextStrokeColor, visitedLink);
> +	       if (result.alpha())
> +		   return result;
> +	   }
> +	   return colorIncludingFallback(CSSPropertyWebkitTextFillColor,
visitedLink);
> +    }

This should go into decorationColor. colorIncludingFallback shouldn't include
paint fallbacks as it is used outside painting (f.e. collapsed borders
resolution). Look at what we do for text-stroke-color or text-fill-color.

On top of that, it would avoid duplicating some of the existing fallback logic.