[webkit-reviews] review denied: [Bug 35020] Move SVGResources to Renderers, starting with Masker : [Attachment 48911] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 35020: Move SVGResources to Renderers, starting with Masker
https://bugs.webkit.org/show_bug.cgi?id=35020

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
I'm tempted to r+ it, though still some things to be changed: most 
> Index: WebCore/Android.mk
> +    rendering/RenderSVGResourceMasker.cpp \

Tab still there.

> Index: WebCore/rendering/SVGRenderTreeAsText.cpp
> +    switch (unitType) {
> +    case SVGUnitTypes::SVG_UNIT_TYPE_UNKNOWN :
> +    case SVGUnitTypes::SVG_UNIT_TYPE_USERSPACEONUSE :
> +    case SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX :

There's a trailing space after the identifier, please remove.

> +void writeResourcesToObject(TextStream& ts, const RenderObject& object, int
indent)
writeReosurcesToObject?? That naming is quite confusing. I'd vote for just
writeResources().
There's already writeRenderResources, that will die soon.

> Index: WebCore/svg/SVGMaskElement.cpp
> -	   invalidateCanvasResources();
> +	   SVGStyledElement::invalidateCanvasResources();

You can omit SVGStyledElement:: here.

> -    invalidateCanvasResources();
> +    SVGStyledElement::invalidateCanvasResources();

Ditto.

Can you please also fix the two include issues reported by the style bot?

I'd r+ it, though we need to upload a new patch anyways (for possible roll-outs
etc.., you'll never know...)



> Index: WebCore/svg/SVGStyledElement.cpp
> -	   if (SVGStyledElement* styledElement =
static_cast<SVGStyledElement*>(element->isStyled() ? element : 0))
> +	   if (SVGStyledElement* styledElement =
static_cast<SVGStyledElement*>(element->isStyled() ? element : 0)) 

Please remove the trailing whitespace.


>  void SVGStyledElement::invalidateCanvasResources()
>  {
> -    if (SVGResource* resource = canvasResource(renderer()))
> +    RenderObject* object = renderer();

Please ASSERT(object); here.

> +    if (object->isSVGResource())
> +	   object->toRenderSVGResource()->invalidateClients();
> +

Please add a comment here, stating that the lines below will be removed soon.

> +    if (SVGResource* resource = canvasResource(object))
>	   resource->invalidate();

Ah and I think no one else is overriding invalidatecanvasResources(), so you
can de-virtualize it, see SVGStyledElement.h