[webkit-reviews] review granted: [Bug 126296] Remove hardcoded composition background color; use RenderStyle::compositionFillColor() : [Attachment 222250] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 126296: Remove hardcoded composition background color; use
RenderStyle::compositionFillColor()
https://bugs.webkit.org/show_bug.cgi?id=126296

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=222250&action=review


> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2640
> +	   case CSSPropertyWebkitCompositionFillColor: {
> +	       if (!style->compositionFillColor().isValid())
> +		   return
cssValuePool().createColorValue(m_node->document().renderView()->theme().compos
itionFillColor().rgb());
> +	       return
cssValuePool().createColorValue(style->compositionFillColor().rgb());
> +	   }

No need for these braces.

I can’t find any other code in this function that dereferences renderView(); do
we have a solid guarantee that it’s non-null?

Is there a good reason to use m_node->document() rather than
styledNode->document()?

> Source/WebCore/platform/graphics/Color.h:-156
> -    static const RGBA32 tap = 0x4D1A1A1A;

Obviously didn’t belong here, but where is the tap color now?

> Source/WebCore/platform/graphics/Color.h:156
>  
>  private:

Looks like we are leaving an extra blank line behind here.

> Source/WebCore/rendering/style/RenderStyle.h:1446
> +    void setCompositionFillColor(const Color &c) {
SET_VAR(rareInheritedData, compositionFillColor, c); }

Move the "&" next to Color, please.