[webkit-reviews] review denied: [Bug 65643] Repaint issues with -webkit-svg-shadow used on a container : [Attachment 104597] last patch plus image results that I missed

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied  review:
Bug 65643: Repaint issues with -webkit-svg-shadow used on a container
https://bugs.webkit.org/show_bug.cgi?id=65643

Attachment 104597: last patch plus image results that I missed
https://bugs.webkit.org/attachment.cgi?id=104597&action=review

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


It's getting better! Still some questions/suggestions.

> Source/WebCore/platform/graphics/mac/FontMac.mm:225
> +    bool hasSimpleShadow = context->textDrawingMode() == TextModeFill &&
shadowColor.isValid() && !shadowBlur && !platformData.isColorBitmapFont() &&
(!context->shadowsIgnoreTransforms() ||
context->getCTM().isIdentityOrTranslationOrFlipped()) &&
!context->inTransparencyLayer();

Aren't other platforms affected by this as well?
Why is this needed? Can you explain a bit more?

> Source/WebCore/rendering/RenderObject.cpp:1789
> +	   setHasSVGShadow(TRUE);

s/TRUE/true/

> Source/WebCore/rendering/RenderObject.h:919
> +#if ENABLE(SVG)
> +    bool m_hasSVGShadow;
> +#endif

This bit of information is needed by all renderers which use
intersectRepaintRectWithResources: RenderSVG(Container|Image|Path|Root|Text).

RenderSVG(Container|Image|Path) all inherit from RenderSVGModelObject.
RenderSVGRoot inherits from RenderBox, RenderSVGText from RenderSVGBlock, and
thus from RenderBlock.

I'd rather store m_hasSVGShadow in RenderSVGModelObject, RenderSVGRoot and
RenderSVGText, to avoid consuming more memory for HTML use cases.
hasSVGShadow could still be a virtual method in RenderObject, just like it's
done for eg. nodeAtFloatPoint, and we'd override the getters/setters in
RenderSVGModelObject/Root/Text.

What do you think?

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:287
> +
> +	   if (!style)
> +	       continue;

IIRC neither style nor svgStyle can be null, please double-check.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:301
> +	   if (!currentObject->hasSVGShadow())
> +	       return;
> +    } while (currentObject);

while (currentObject && currentObject->hasSVGShadow() ?