[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 #48867|review?                     |review-
               Flag|                            |




--- Comment #5 from Nikolas Zimmermann <zimmermann at kde.org>  2010-02-17 06:53:23 PST ---
(From update of attachment 48867)
Moin Dirk,

okay, the ChangeLog really needs a rewrite :-) I'd like to see more comments,
what happened per file.
Explain the RenderSVGResource concept, that it will serve as base class for all
upcoming RenderSVGResourceXXX objects, that it will replace SVGResource - that
we need to keep SVGResource until the transition is finished, leading to a bit
confusing code, until the move is done :-)

> Index: WebCore/Android.mk
> +    rendering/RenderSVGResourceMasker.cpp \

Tabs.

> Index: WebCore/rendering/RenderSVGResource.h
> +enum RenderSVGResourceType {
> +    // Painting mode
> +    MaskerResourceType = 0
> +};

The comment is confusing, please remove. Also the 0 default is not needed.

> +
> +class RenderSVGResource : public RenderSVGHiddenContainer {
> +
> +public:

One newline too much

> +    virtual const char* renderName() const { return "RenderSVGResource"; }

I'd either not implement renderName() or make it pure virtual. As
RenderSVGResource is not constructable, this doesn't make much sense.

> Index: WebCore/rendering/SVGRenderSupport.cpp
> -    SVGResourceClipper* clipper = getClipperById(document, clipperId, object);
> -    SVGResourceMasker* masker = getMaskerById(document, maskerId, object);
> -
> +    // apply Masker
> +    RenderSVGResourceMasker* masker = getRenderSVGResourceById<RenderSVGResourceMasker>(document, maskerId);
>      if (masker) {
Please fold the statment above into the if().

> +    SVGResourceClipper* clipper = getClipperById(document, clipperId, object);
>      if (clipper) {

Ditto.

>  FloatRect SVGRenderBase::maskerBoundingBoxForRenderer(const RenderObject* object) const
>  {
> -    SVGResourceMasker* masker = getMaskerById(object->document(), object->style()->svgStyle()->maskElement(), object);
> +    RenderSVGResourceMasker* masker = getRenderSVGResourceById<RenderSVGResourceMasker>(object->document(), object->style()->svgStyle()->maskElement());
>      if (masker)
> -        return masker->maskerBoundingBox(object->objectBoundingBox());

Ditto.

> Index: WebCore/rendering/SVGRenderTreeAsText.cpp
> +void writeSVGResource(TextStream& ts, const RenderObject& object, int indent)

> +
> +    RenderSVGResource* resource = const_cast<RenderObject&>(object).toRenderSVGResource();
> +    if (resource->resourceType() == MaskerResourceType) {
> +        RenderSVGResourceMasker* masker = static_cast<RenderSVGResourceMasker*>(resource);
> +        if (!masker)
> +            return;

You want to ASSERT(masker) instead of early-return.

> +    // FIXME: Add the other SVGResources here.
Please change to comment to something like "Handle other RenderSVGResource*
classes, as soon as they are converted from SVGResource*"

> +void writeRenderSVGResource(TextStream& ts, const RenderObject& object, int indent)
How about naming the function writeResources instaed of writeRenderSVGResource?

> +
> +    if (!svgStyle->maskElement().isEmpty()) {
> +        writeIndent(ts, indent);
> +        ts << " ";
> +        writeNameAndQuotedValue(ts, "masker", svgStyle->maskElement());
> +        RenderSVGResourceMasker* masker = getRenderSVGResourceById<RenderSVGResourceMasker>(object.document(), svgStyle->maskElement());
> +        if (masker) {
> +            ts << " ";
> +            writeStandardPrefix(ts, *masker, 0);
> +            ts << " " << masker->resourceBoundingBox(object.objectBoundingBox()) << "\n";
> +        }
> +    }

Hmm this seems flawed, we shouldn't dump "masker=".. anything, if the
maskElement() does NOT point to an actual mask element, but say a clipPath.
Fold the RenderSVGResourceMasker* masker = get... line with the if() and only
writeIndent & writeNameAndQuotedValue inside the if statment.

> +    // FIXME: Add the other SVGResources here.
Same comment change needed.

> Index: WebCore/svg/SVGMaskElement.cpp

> -        invalidateCanvasResources();
> +        if (RenderSVGResource* resource = renderer()->toRenderSVGResource())
> +            resource->invalidateClients();
>  }

> -    invalidateCanvasResources();
> +
> +    if (!renderer())
> +        return;
> +
> +    if (RenderSVGResource* resource = renderer()->toRenderSVGResource())
> +        resource->invalidateClients();
>  }

Why not leave the invalidateCanvasResource() function, and move your code
querying toRenderSVGResource() there?
I'd even propose to move that code into
SVGStyledElement::invalidateCanvasResources(), as it's resource-type agnostic,
and will work for clippers etc.. as well.


> 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)) {
> +            RenderObject* renderer = styledElement->renderer();
> +            if (renderer && renderer->isSVGResource())
> +                renderer->toRenderSVGResource()->invalidateClients();
>              styledElement->invalidateCanvasResources();
> +        }

If you follow my suggestion from above, moving the
toRenderSVGResource()->invalidateclients() stuff into
invalidateCanvasResources, then you can save the change above as well.

> Index: WebCore/svg/SVGUnitTypes.h
> ===================================================================
> --- WebCore/svg/SVGUnitTypes.h	(revision 54874)
> +++ WebCore/svg/SVGUnitTypes.h	(working copy)
> @@ -27,6 +27,7 @@
>  namespace WebCore {
>  
>  class SVGUnitTypes : public RefCounted<SVGUnitTypes> {
> +
>  public:

Newline too much.

> +static inline SVGUnitTypes::SVGUnitType toUnitType(int input) { return static_cast<SVGUnitTypes::SVGUnitType>(input); }
s/input/type/

>  } // namespace WebCore
>  
>  #endif // ENABLE(SVG)

Remmove the visual noise.

Ok, still some issues, even after the 115th iteration ;-) But the concept is
sound, the code is fine, just some nitpicking :-)
Happy to review the next iteration..

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