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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Feb 17 06:53:23 PST 2010


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 48867: Patch
https://bugs.webkit.org/attachment.cgi?id=48867&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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..


More information about the webkit-reviews mailing list