[Webkit-unassigned] [Bug 35020] Move SVGResources to Renderers, starting with Masker

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=35020


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #48911|review?                     |review-
               Flag|                            |




--- Comment #8 from Nikolas Zimmermann <zimmermann at kde.org>  2010-02-17 12:21:17 PST ---
(From update of attachment 48911)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.