[webkit-reviews] review denied: [Bug 59774] Add template parameter to ApplyPropertyColor to improve clarity by removing constructor parameter side effects. : [Attachment 91629] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 59774: Add template parameter to ApplyPropertyColor to improve clarity by
removing constructor parameter side effects.
https://bugs.webkit.org/show_bug.cgi?id=59774

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

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=91629&action=review

I like it, but a few nits:

> Source/WebCore/css/CSSStyleApplyProperty.cpp:132
> +template <bool inheritColorFromParent = false>

Can a template parameter be an enum, like enum InheritColor {
InheritColorFromParent, WhateverTheOtherValue };

> Source/WebCore/css/CSSStyleApplyProperty.cpp:343
> +    setPropertyValue(CSSPropertyColor, new
ApplyPropertyColor<true>(&RenderStyle::color, &RenderStyle::setColor, 0,
RenderStyle::initialColor));

This way, you could write this as new ApplyPropertyColor<IneritColorFromParent>
-- much more readable.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:404
> +    setPropertyValue(CSSPropertyBackgroundColor, new
ApplyPropertyColor<>(&RenderStyle::backgroundColor,
&RenderStyle::setBackgroundColor, 0));

And without the default value, this could look prettier too, explicitly
explaining what's happening.