[webkit-reviews] review granted: [Bug 193171] Implement the css-color-4 behavior for inheritance of currentColor : [Attachment 395441] patch

Sat Apr 4 08:44:14 PDT 2020

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Antti Koivisto
<koivisto at iki.fi>'s request for review:
Bug 193171: Implement the css-color-4 behavior for inheritance of currentColor
https://bugs.webkit.org/show_bug.cgi?id=193171

Attachment 395441: patch

https://bugs.webkit.org/attachment.cgi?id=395441&action=review

--- Comment #14 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 395441
  --> https://bugs.webkit.org/attachment.cgi?id=395441
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395441&action=review

> LayoutTests/fast/borders/border-color-inherit-expected.html:1
> +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN">

Maybe just <!DOCTYPE html> (and in the test too).

> Source/WebCore/rendering/style/RenderStyle.h:1735
> +    static Color currentColor() { return { }; }

Maybe change initialTextEmphasisColor() to return curentColor().

> Source/WebCore/style/StyleBuilderState.cpp:324
> +    // FIXME: 'currentcolor' should be resolved at use time to make it
inherit correctly.

You should file a bug on the FIXME and reference it here. That seems fairly
important to fix.

> Source/WebCore/svg/SVGStopElement.cpp:97
> +    if (!stopColor.isValid())
> +	   stopColor = style.color();

A little helper function like RenderStyle::resolveCurrentColor or something
would make this much more understandable.