[webkit-reviews] review denied: [Bug 65769] New renderer for SVGRectElement : [Attachment 109287] Propsed patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 3 04:21:08 PDT 2011


Nikolas Zimmermann <zimmermann at kde.org> has denied  review:
Bug 65769: New renderer for SVGRectElement
https://bugs.webkit.org/show_bug.cgi?id=65769

Attachment 109287: Propsed patch
https://bugs.webkit.org/attachment.cgi?id=109287&action=review

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


Okay, I'm revoking Dirks r+. I want to see 0 failures from cr-linux pixel test
EWS first. This patch is too important.
I also have some more comments, and like to see a final patch w/o any commented
code etc.

> Source/WebCore/rendering/svg/RenderSVGRect.h:38
> +    explicit RenderSVGRect(SVGStyledTransformableElement*);

Shouldn't this be SVGRectElement* ?

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:-103
> -

Unncessary change.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:-271
> -

Ditto.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:-296
> -    

Ditto.

> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:268
> +	       if (path)
> +		   context->fillPath(*path);
> +	       else if (shape)
> +		   shape->fillShape(context);

I guess at some point, we want to stop passing const Path* path to
postApplyResource completly and only pass/use the RenderSVGShape object right?
This currently looks a bit weird. Maybe a FIXME would clarify it.

> Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:187
> +void RenderSVGResourcePattern::postApplyResource(RenderObject*,
GraphicsContext*& context, unsigned short resourceMode, const Path* path, const
RenderSVGShape* shape)

Same here.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:66
> +	m_path = adoptPtr(path);

Can't this be written as , m_path(adopPtr(path)) ?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:75
> +    m_path = adoptPtr(new Path);

Can m_path already be set here? Can we assert m_path is null before

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:147
> +	   if (!m_path)
> +	       RenderSVGShape::createShape();

Aha, so I think we can safely ASSERT(!m_path) in createShape, right?

> Source/WebCore/rendering/svg/RenderSVGShape.h:55
> +    void strokeStyle(GraphicsContext* gc)

s/gc/context/


More information about the webkit-reviews mailing list