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

[bugzilla-daemon at webkit.org]

Renata Hodovan <reni at webkit.org> has asked  for review:
Bug 65769: New renderer for SVGRectElement
https://bugs.webkit.org/show_bug.cgi?id=65769

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

------- Additional Comments from Renata Hodovan <reni at webkit.org>
> > Source/WebCore/ChangeLog:6
> > +	     Adding a new framework to support the rendering of SVG shapes.
> 
> I'd propose: Adding a new framework to support more efficient rendering of
SVG shapes.
OK.

> > Source/WebCore/ChangeLog:8
> > +	     The main logic takes place in RenderSVGShape class and every
renderer will
> > +	     inherit from it. Since its code base is mostly moved from
RenderSVGPath it's suitable to
> 
> Well this is not correct. RenderSVGEllipse/Rect etc will inherit from it, but
not RenderSVGText etc. It's worded a bit unfortunate, please fix it :-)
OK.

> > Source/WebCore/ChangeLog:10
> > +
> 
> In general this ChangeLog should be more descriptive, this is a heavy change
and thus deserves a good explaination.
> 
> > Source/WebCore/platform/graphics/FloatRect.h:149
> > +	 bool comprises(const FloatPoint&) const;
> 
> Interssting, never heard the term "comprises" before. The only difference to
contains is < -> <= right?
> Couldn't this be an enum passed to contains() named "ContainsMode"?
I don't think so it would be a good idea. Contains() is used in many places but
compraises() just twice. What about if I give it a better name, eg.:
isInsideRect() ?

> > Source/WebCore/rendering/RenderObject.h:359
> > +	 virtual bool isSVGLine() const { return false; }
> 
> Line? This is not yet in, is it?
Ooops, I've started to work on SVGLines and this snippet remained here.
Deleted.

> > Source/WebCore/rendering/svg/RenderSVGPath.h:35
> > +// FIXME: RenderSVGPath will be removed when all SVG shapes will be
rendered via RenderSVGShape.
> > +class RenderSVGPath : public RenderSVGShape {
> 
> Huh, really? I thought it would still stay. It would be confusing if
RenderSVGShape would be the main renderer for <path>, and RenderSVGRect
inheriting from RenderSVGShape for <rect>.
OK.

> > Source/WebCore/rendering/svg/RenderSVGRect.cpp:49
> > +	 // Clear m_boundingBox.
> 
> Does this serve a comment? It's not really helpful, it doesn't state a
reason.
OK.

> > Source/WebCore/rendering/svg/RenderSVGRect.cpp:56
> > +	 bool hasRx = rect->hasAttribute(SVGNames::rxAttr);
> > +	 bool hasRy = rect->hasAttribute(SVGNames::ryAttr);
> > +	 // Fallback to RenderSVGShape if rect has special features.
> > +	 if (hasRx || hasRy) {
> 
> I'd rather see the fallback code decision in a seperated function:
> bool shouldFallbackToPathRendering().
This function would be used from only this point. I could add an inline
function what got two boolean and made a decison according to them. It seems
useless, so I left out this modification now, but the comment above is updated.


> Hm, I thought Dirk mentioned more corner cases, you're not handling them
here?
There are handled in different places. We talked about it before. We
distinguish two different fallback cases. One for painting and an other for
pointer-events. Painting fallback is needed just by rounded corners and pointer
events by rounded rects and special strokes.

> > Source/WebCore/rendering/svg/RenderSVGRect.cpp:86
> > +	 if (isFillFallback() &&
!style()->svgStyle()->strokeDashArray().isEmpty())
> 
> Why that extra condition? If the strokeDashArray is empty and isFillFallback
is true, shouldn't RenderSVGshape be asked for its stroke bbox??
StrokeBoundingBox() is called by handling pointer events and as I mentioned in
my previous comment we need to fallback both of rounded corners and special
strokes.
 
> > Source/WebCore/rendering/svg/RenderSVGRect.cpp:98
> > +	 context->strokeRect(m_boundingBox, this->strokeWidth());
> 
> s/this->//
Ok.

> > Source/WebCore/rendering/svg/RenderSVGRect.h:46
> > +	 virtual bool isEmpty() const { return hasPath() ?
RenderSVGShape::isEmpty() : m_boundingBox.isEmpty(); };
> 
> Here hasPath() is used not isFillFallback(), is that on purpose?
> You should make this a multi-line function, and/or move in the cpp file.
I have to refer again to the two different fallback cases. It's possible that
we don't have fallback by painting, because we don't have rounded rect. At the
same time we have dashed stroke with pointer event, so m_path is filled.

> > Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:191
> > +	 if (/*path && */!(resourceMode & ApplyToTextMode)) {
> 
> You can remove the comment.
OK.

> > Source/WebCore/rendering/svg/RenderSVGResourceSolidColor.cpp:83
> > +	 if (!(resourceMode & ApplyToTextMode)) {
> 
> We could add an early return here.
OK. 

> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:139
> > +	     if (!m_path.get())
> 
> The .get() is superfluous.
OK.

> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:140
> > +		 RenderSVGShape::createShape();
> 
> This should be encapsulated in a descriptive named function, otherwhise we'll
wonder why only these conditions are checked.
> 
> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:141
> > +	     return RenderSVGShape::shapeDependentStrokeContains(point);
> 
> You want to avoid calling the shapeDependent function from the derived class,
thus that RenderSVGShape:: prefix here?
Exactly! In this case I want to be sure that the ancestor is called because the
derived class isn't able to handle the problem (we need fallback). The same
situation like by your previous comment.

> This is easy to misread.
> The whole stuff should be commented more, hard to follow at the moment.
> 
> > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:156
> > +		
filter->postApplyResource(static_cast<RenderSVGShape*>(object),
paintInfo.context, ApplyToDefaultMode, 0);
> 
> That's not a good idea, to blindly cast here!
> This is also used for eg. RenderSVGContainer or Text who call
finish/preparerenderSVGContent.
> How is this supposed to work?
You are right. It wasn't common enough. It's updated.