Move SVG shape painting code to SVGShapePainter

Move SVG shape painting code to SVGShapePainter

This patch moves all SVG shape (path,rect,ellipse,circle,polyline,line,
polygon) painting code to SVGShapePainter.

This megapatch is primarily a code move without changes. The two big
changes are:
1) All painting code for RenderSVG{Shape,Path,Rect,Ellipse} has been
moved to SVGShapePainter. The original designer of this code went
OOP crazy and created a delicious spaghetti of fillShape and
strokeShape. These virtual functions have been replaced with two new
virtuals that are a bit easier to digest: useRectRenderingFastPath and
useEllipseRenderingFastPath.

2) The marker code only applies to "markable elements"[1] and is only
needed on RenderSVGPath. The marker data and functions have been moved
to RenderSVGPath which shrinks RenderSVG{Rect,Ellipse} by a pointer
and cleans up RenderSVGShape.

[1] https://svgwg.org/svg2-draft/single-page.html#intro-TermMarkableElement

BUG=412088
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184487

[pdr., 2014-10-25 07:13:31]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, fs@opera.com, schenney@chromium.org

[pdr., 2014-10-25 20:16:26]

pdr@chromium.org changed reviewers:
+ fmalita@chromium.org

[chrishtr, 2014-10-27 03:34:12]

https://codereview.chromium.org/678863002/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGShape.h (right):

https://codereview.chromium.org/678863002/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGShape.h:62: virtual FloatRect
paintInvalidationRectInLocalCoordinates() const override final { return
m_paintInvalidationBoundingBox; }
Can this one be un-virtualized?

https://codereview.chromium.org/678863002/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGShape.h:62: virtual FloatRect
paintInvalidationRectInLocalCoordinates() const override final { return
m_paintInvalidationBoundingBox; }
Can this one be un-virtualized?

[fs, 2014-10-27 12:36:27]

LGTM w/ a suggestion and a bunch of trivial nits.

https://codereview.chromium.org/678863002/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGPath.h (right):

https://codereview.chromium.org/678863002/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGPath.h:40: virtual const Vector<FloatPoint>*
zeroLengthLineCaps() const override { return &m_zeroLengthLinecapLocations; };
Nit: Redundant ';'.

https://codereview.chromium.org/678863002/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGShape.h (right):

https://codereview.chromium.org/678863002/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGShape.h:62: virtual FloatRect
paintInvalidationRectInLocalCoordinates() const override final { return
m_paintInvalidationBoundingBox; }
On 2014/10/27 03:34:12, chrishtr wrote:
> Can this one be un-virtualized?

Not quite yet I think (still a few places calling this through RenderObject.)
Given the 'final' it ought to be devirtualized in the context of paint/ though.

https://codereview.chromium.org/678863002/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGShape.h:74: virtual bool
useEllipseRenderingFastPath() const { return false; }
Why two extension points for this? They'll be mutually exclusive after all, so
something like:

enum GeometryCodePath {
  ...
};

virtual GeometryCodePath geometryCodePath() const { return ...; }

might do as well. (To go entirely crazy, that could just be stored in the
bitfield.)

https://codereview.chromium.org/678863002/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGShape.h:75: virtual const Vector<FloatPoint>*
zeroLengthLineCaps() const { return 0; };
Nit: Redundant ';'.

https://codereview.chromium.org/678863002/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGShape.h:86: virtual void
processMarkerPositions() { };
Nit: Redundant ';'

[pdr., 2014-10-27 22:29:42]

The CQ bit was checked by pdr@chromium.org

[pdr., 2014-10-27 22:29:44]

Thanks for the reviews!

Looking forward to devirtualizing paintInvalidationRectInLocalCoordinates soon.

https://codereview.chromium.org/678863002/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGPath.h (right):

https://codereview.chromium.org/678863002/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGPath.h:40: virtual const Vector<FloatPoint>*
zeroLengthLineCaps() const override { return &m_zeroLengthLinecapLocations; };
On 2014/10/27 at 12:36:27, fs wrote:
> Nit: Redundant ';'.

Done

https://codereview.chromium.org/678863002/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGShape.h (right):

https://codereview.chromium.org/678863002/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGShape.h:74: virtual bool
useEllipseRenderingFastPath() const { return false; }
On 2014/10/27 at 12:36:27, fs wrote:
> Why two extension points for this? They'll be mutually exclusive after all, so
something like:
> 
> enum GeometryCodePath {
>   ...
> };
> 
> virtual GeometryCodePath geometryCodePath() const { return ...; }
> 
> might do as well. (To go entirely crazy, that could just be stored in the
bitfield.)

Good idea, done.

https://codereview.chromium.org/678863002/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGShape.h:75: virtual const Vector<FloatPoint>*
zeroLengthLineCaps() const { return 0; };
On 2014/10/27 at 12:36:27, fs wrote:
> Nit: Redundant ';'.

Done.

[commit-bot: I haz the power, 2014-10-27 22:30:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/678863002/80001

[commit-bot: I haz the power, 2014-10-28 00:12:37]

Committed patchset #5 (id:80001) as 184487