[webkit-reviews] review denied: [Bug 65643] Repaint issues with -webkit-svg-shadow used on a container : [Attachment 104713] previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 65643: Repaint issues with -webkit-svg-shadow used on a container
https://bugs.webkit.org/show_bug.cgi?id=65643

Attachment 104713: previous patch plus make inTransparencyLayer/increment stuff
generic, plus move hasSVGShadow into SVG renderers instead of RenderObject
https://bugs.webkit.org/attachment.cgi?id=104713&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104713&action=review


Looks great, some further comments from my side and Darin lead to r- for now:

> Source/WebCore/platform/graphics/GraphicsContext.cpp:338
> +    m_transparencyCount++;

Use ++m_transparencyCount as micro optimization.

> Source/WebCore/platform/graphics/GraphicsContext.cpp:345
> +    m_transparencyCount--;

Use --m_transparencyCount as micro optimization.

>> Source/WebCore/platform/graphics/GraphicsContext.cpp:350
>> +#if (OS(WINCE) && !PLATFORM(QT)) || PLATFORM(WX)
> 
> It seems highly likely  this list of platforms will get out of sync with the
underlying machinery. Maybe instead there should be a private static inline
member function that returns false or true that you could || with the
transparency count.

Agreed!

> Source/WebCore/platform/graphics/GraphicsContext.h:546
> +	   int m_transparencyCount;

unsigned should be sufficient.

> Source/WebCore/rendering/RenderObject.h:403
> +    virtual void setHasSVGShadow(bool shadowedParent) {
UNUSED_PARAM(shadowedParent); }

I'd rather just use setHasSVGShadow(bool) { } here.