[webkit-reviews] review denied: [Bug 89148] Null-pointer crash when a derived color like -webkit-activelink is set in a gradient stop : [Attachment 148054] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied
dstockwell at chromium.org's request for review:
Bug 89148: Null-pointer crash when a derived color like -webkit-activelink is
set in a gradient stop
https://bugs.webkit.org/show_bug.cgi?id=89148

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=148054&action=review


> Source/WebCore/css/CSSGradientValue.cpp:108
> +PassRefPtr<CSSGradientValue> CSSGradientValue::resolveStyles(StyleResolver*
styleResolver)

It's a bit odd that a method called resolveStyles resturns a gradient. Maybe
gradientWithStylesResolved()?

It's also odd that this method changes |this| yet returns a clone.

> Source/WebCore/css/CSSGradientValue.cpp:112
> +	   derived = derived ||
styleResolver->isElementDerivedColor(m_stops[i].m_color.get());

derived |= 
Putting m_stops[i].m_color in a local variable would be a minor win here.

> Source/WebCore/css/StyleResolver.h:224
> +    static bool isElementDerivedColor(CSSPrimitiveValue*);

This name is hard to read. Is it "is element-derived color", or "is element
derived-color"? It's also not being called on a color-related class.