Move SVG container paint code to SVGContainerPainter

Move SVG container paint code to SVGContainerPainter

This patch moves the direct painting code (through paint(...)) to
the new SVGContainerPainter. The resource container subclasses of
RenderSVGContainer have a second painting codepath through draw()
functions which will be moved to a painter class in a separate patch.

This patch is primarily a straight code move but contains the following
non-trivial changes:
1) RenderSVGViewport::applyViewportClip was not implemented directly
but RenderSVGViewportContainer and RenderSVGResourceMarker override it
for clipping. Instead of using applyViewportClip,
SVGContainerPainter::paint now directly handles the clipping of
RenderSVGViewportContainer. RenderSVGResourceMarker's viewport clip
function has been moved in RenderSVGResourceMarker::draw.

2) paintInvalidationRectInLocalCoordinates and selfWillPaint have been
made public on RenderSVGViewport.

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

[pdr., 2014-09-25 04:10:25]

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

[fs, 2014-09-25 11:00:44]

fs@opera.com changed reviewers:
+ fs@opera.com

[fs, 2014-09-25 11:00:45]

https://codereview.chromium.org/601093002/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGResourceMarker.cpp (right):

https://codereview.chromium.org/601093002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGResourceMarker.cpp:145:
info.context->clip(m_viewport);
If transform.isIdentity(), this clip could now "leak" AFAICS.

[f(malita), 2014-09-25 12:54:00]

I would prefer to keep any functional changes out of these
move-paint-code-to-painter CLs, they are much easier to review in isolation.

[f(malita), 2014-09-25 12:55:53]

On 2014/09/25 12:54:00, Florin Malita wrote:
> I would prefer to keep any functional changes out of these
> move-paint-code-to-painter CLs, they are much easier to review in isolation.

(it would also make the move CL review process more rubber-stampy)

[f(malita), 2014-09-25 13:42:50]

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

[f(malita), 2014-09-25 13:42:51]

https://codereview.chromium.org/601093002/diff/1/Source/core/paint/SVGContain...
File Source/core/paint/SVGContainerPainter.cpp (right):

https://codereview.chromium.org/601093002/diff/1/Source/core/paint/SVGContain...
Source/core/paint/SVGContainerPainter.cpp:43: if
(m_renderSVGContainer.isSVGViewportContainer() &&
SVGRenderSupport::isOverflowHidden(&m_renderSVGContainer))
If we're going to do this, can we handle it in
RenderSVGViewportContainer::paint() instead to avoid adding subclass-specific
exceptions at this level?

Saves us a virtual call, and I think overflow:hidden is rare enough in SVG that
we don't have to worry about an extra save()/restore() frame.

https://codereview.chromium.org/601093002/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGContainer.h (left):

https://codereview.chromium.org/601093002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGContainer.h:77: virtual void
applyViewportClip(PaintInfo&) { }
Can you explain why de-encapsulating applyViewport is desirable? We're saving a
vtable entry but beyond that I find the new approach slightly less clear.

[pdr., 2014-10-08 20:45:31]

Thank you for the reviews! I finally got a chance to revisit this patch. PTAL

https://codereview.chromium.org/601093002/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGContainer.h (left):

https://codereview.chromium.org/601093002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGContainer.h:77: virtual void
applyViewportClip(PaintInfo&) { }
On 2014/09/25 at 13:42:50, Florin Malita wrote:
> Can you explain why de-encapsulating applyViewport is desirable? We're saving
a vtable entry but beyond that I find the new approach slightly less clear.

Our goal here is to move all paint-related code into painting/ (part of paint
slimming). I agree that this approach is a small step backwards in terms of
readability but I think this will be temporary and we'll be able to clean this
up once all paint-related code is in one place.

https://codereview.chromium.org/601093002/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGResourceMarker.cpp (right):

https://codereview.chromium.org/601093002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGResourceMarker.cpp:145:
info.context->clip(m_viewport);
On 2014/09/25 at 11:00:45, fs wrote:
> If transform.isIdentity(), this clip could now "leak" AFAICS.

Previously this code has an isIndentity check a couple of lines above but that's
been refactored away. I think the updated patch will be clean, but please
double-check in case I've misunderstood.

[chrishtr, 2014-10-08 21:56:38]

https://codereview.chromium.org/601093002/diff/20001/Source/core/paint/SVGCon...
File Source/core/paint/SVGContainerPainter.cpp (right):

https://codereview.chromium.org/601093002/diff/20001/Source/core/paint/SVGCon...
Source/core/paint/SVGContainerPainter.cpp:36: if
(isSVGSVGElement(*m_renderSVGContainer.element()) &&
toSVGSVGElement(*m_renderSVGContainer.element()).hasEmptyViewBox())
Are you sure adding this here rather than the subclass doesn't change semantics?
Is it because the parent class is never directly instantiated?

https://codereview.chromium.org/601093002/diff/20001/Source/core/paint/SVGCon...
Source/core/paint/SVGContainerPainter.cpp:43: if
(m_renderSVGContainer.isSVGViewportContainer() &&
SVGRenderSupport::isOverflowHidden(&m_renderSVGContainer))
Same question here.

[pdr., 2014-10-08 23:12:28]

https://codereview.chromium.org/601093002/diff/20001/Source/core/paint/SVGCon...
File Source/core/paint/SVGContainerPainter.cpp (right):

https://codereview.chromium.org/601093002/diff/20001/Source/core/paint/SVGCon...
Source/core/paint/SVGContainerPainter.cpp:36: if
(isSVGSVGElement(*m_renderSVGContainer.element()) &&
toSVGSVGElement(*m_renderSVGContainer.element()).hasEmptyViewBox())
On 2014/10/08 at 21:56:38, chrishtr wrote:
> Are you sure adding this here rather than the subclass doesn't change
semantics? Is it because the parent class is never directly instantiated?

SVGSVGElement can create one of two kinds of renderers: RenderSVGRoot or
RenderSVGViewportContainer. We'll only get here in the second case so this
should be functionally equivalent.

https://codereview.chromium.org/601093002/diff/20001/Source/core/paint/SVGCon...
Source/core/paint/SVGContainerPainter.cpp:43: if
(m_renderSVGContainer.isSVGViewportContainer() &&
SVGRenderSupport::isOverflowHidden(&m_renderSVGContainer))
On 2014/10/08 at 21:56:38, chrishtr wrote:
> Same question here.

This one is a little more contentious and fmalita wasn't sure either.

What I did was to inline applyViewportClip here for RenderSVGViewportContainer,
and I inlined applyViewportClip into the callsite in
RenderSVGResourceMarker::draw. In a followup, the draw() functions will be moved
into paint and we can clean that up.

The alternatives aren't great. I'd like move applyViewportClip out of the
renderers but we lose the OO hierarchy when we do that. Creating an
applyViewportClip helper in SVGContainerPainter with a switch on the subclass
type seems even worse.

[chrishtr, 2014-10-08 23:23:16]

lgtm

https://codereview.chromium.org/601093002/diff/20001/Source/core/paint/SVGCon...
File Source/core/paint/SVGContainerPainter.cpp (right):

https://codereview.chromium.org/601093002/diff/20001/Source/core/paint/SVGCon...
Source/core/paint/SVGContainerPainter.cpp:43: if
(m_renderSVGContainer.isSVGViewportContainer() &&
SVGRenderSupport::isOverflowHidden(&m_renderSVGContainer))
On 2014/10/08 at 23:12:28, pdr wrote:
> On 2014/10/08 at 21:56:38, chrishtr wrote:
> > Same question here.
> 
> This one is a little more contentious and fmalita wasn't sure either.
> 
> What I did was to inline applyViewportClip here for
RenderSVGViewportContainer, and I inlined applyViewportClip into the callsite in
RenderSVGResourceMarker::draw. In a followup, the draw() functions will be moved
into paint and we can clean that up.
> 
> The alternatives aren't great. I'd like move applyViewportClip out of the
renderers but we lose the OO hierarchy when we do that. Creating an
applyViewportClip helper in SVGContainerPainter with a switch on the subclass
type seems even worse.

Ah I didn't see the isSVGViewportContainer() in the patch.

[pdr., 2014-10-09 02:39:42]

Lets hold off on committing this until tomorrow to give fs and fmalita a chance
to respond.

[fs, 2014-10-09 07:02:03]

LGTM, but I expect to see the issue below taken care of (in one way or another.)

https://codereview.chromium.org/601093002/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGResourceMarker.cpp (right):

https://codereview.chromium.org/601093002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGResourceMarker.cpp:145:
info.context->clip(m_viewport);
On 2014/10/08 20:45:31, pdr wrote:
> On 2014/09/25 at 11:00:45, fs wrote:
> > If transform.isIdentity(), this clip could now "leak" AFAICS.
> 
> Previously this code has an isIndentity check a couple of lines above but
that's
> been refactored away. I think the updated patch will be clean, but please
> double-check in case I've misunderstood.

The isIdentity-check just moved into PaintInfo::applyTransform, so I think this
still needs a "info.saveIfNeeded()" before it. (Arguably this case - isIdentity
- will not be very common, so dropping the ", false" from the stateSaver would
probably be good enough too.)

[pdr., 2014-10-09 17:31:53]

The CQ bit was checked by pdr@chromium.org

[pdr., 2014-10-09 17:31:53]

Thanks for the reivews!

[commit-bot: I haz the power, 2014-10-09 17:32:53]

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

[commit-bot: I haz the power, 2014-10-09 19:32:00]

Committed patchset #3 (id:40001) as 183488