Refactor ObjectPainter::paintOutline to take a paintOffset

Refactor ObjectPainter::paintOutline to take a paintOffset

This patch refactors ObjectPainter::paintOutline to take a visual
overflow rect and layout size (w/o paint offset), plus a paint offset.
This cleans up the callsites so each doesn't need to compute a moved
visual overflow rect, and prepares the paint code for paint offset
caching. This has been split off from https://codereview.chromium.org/1315993004.

A TODO has been added above outlineRectForSVG because the function
should not exist. paintInvalidationRectInLocalCoordinates should be
fixed to contain the outline extent.

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

[pdr., 2015-08-27 03:40:37]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, trchen@chromium.org, wangxianzhu@chromium.org

[pdr., 2015-08-27 03:40:41]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-27 03:40:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1321613002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1321613002/1

[chrishtr, 2015-08-27 04:20:41]

https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/BlockPain...
File Source/core/paint/BlockPainter.cpp (left):

https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/BlockPain...
Source/core/paint/BlockPainter.cpp:167: if
(!RuntimeEnabledFeatures::slimmingPaintEnabled())
This flag conditional is not needed any more?

https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/ObjectPai...
File Source/core/paint/ObjectPainter.cpp (right):

https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/ObjectPai...
Source/core/paint/ObjectPainter.cpp:20: LayoutRect
ObjectPainter::outlineRectForSVG(const LayoutSize& size, const ComputedStyle&
style)
This seems like an odd method to have, even temporarily. Why not just define it
on the root SVG object class or something?
Also I don't think it should be in paintInvalidationRectInLocalCoordinates. Or
that method's name should be changed to generalize.

[pdr., 2015-08-27 04:59:57]

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

[pdr., 2015-08-27 05:00:17]

https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/BlockPain...
File Source/core/paint/BlockPainter.cpp (left):

https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/BlockPain...
Source/core/paint/BlockPainter.cpp:167: if
(!RuntimeEnabledFeatures::slimmingPaintEnabled())
On 2015/08/27 at 04:20:41, chrishtr wrote:
> This flag conditional is not needed any more?

It was only present as a performance optimization to not compute the visual
overflow rect w/o slimming paint. Everything will still work without slimming
paint, we'll just compute this unnecessarily.

https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/ObjectPai...
File Source/core/paint/ObjectPainter.cpp (right):

https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/ObjectPai...
Source/core/paint/ObjectPainter.cpp:20: LayoutRect
ObjectPainter::outlineRectForSVG(const LayoutSize& size, const ComputedStyle&
style)
On 2015/08/27 at 04:20:41, chrishtr wrote:
> This seems like an odd method to have, even temporarily. Why not just define
it on the root SVG object class or something?
> Also I don't think it should be in paintInvalidationRectInLocalCoordinates. Or
that method's name should be changed to generalize.

SVG's paint invalidation rect concept needs a big overhaul but I'd like to save
that for a followup.

In the latest patchset I removed ObjectPainter::outlineRectForSVG(...) and added
it onto the root class, LayoutSVGModelObject, along with a TODO that this code
needs to go away.

[fs, 2015-08-27 10:23:17]

lgtm

https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/ObjectPai...
File Source/core/paint/ObjectPainter.cpp (right):

https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/ObjectPai...
Source/core/paint/ObjectPainter.cpp:20: LayoutRect
ObjectPainter::outlineRectForSVG(const LayoutSize& size, const ComputedStyle&
style)
On 2015/08/27 05:00:17, pdr wrote:
> On 2015/08/27 at 04:20:41, chrishtr wrote:
> > This seems like an odd method to have, even temporarily. Why not just define
> it on the root SVG object class or something?
> > Also I don't think it should be in paintInvalidationRectInLocalCoordinates.
Or
> that method's name should be changed to generalize.
> 
> SVG's paint invalidation rect concept needs a big overhaul but I'd like to
save
> that for a followup.

I can look into this if you like. I assume we want to move in the direction of
visualOverflowRect.

[Xianzhu, 2015-08-27 15:59:39]

lgtm.

This remembers me about my *low-priority* CLs:
https://codereview.chromium.org/1278543002/ and
https://codereview.chromium.org/1224933002/. Will appreciate if you can review
them when you have time :)

[pdr., 2015-08-27 17:42:36]

On 2015/08/27 at 10:23:17, fs wrote:
> lgtm
> 
>
https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/ObjectPai...
> File Source/core/paint/ObjectPainter.cpp (right):
> 
>
https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/ObjectPai...
> Source/core/paint/ObjectPainter.cpp:20: LayoutRect
ObjectPainter::outlineRectForSVG(const LayoutSize& size, const ComputedStyle&
style)
> On 2015/08/27 05:00:17, pdr wrote:
> > On 2015/08/27 at 04:20:41, chrishtr wrote:
> > > This seems like an odd method to have, even temporarily. Why not just
define
> > it on the root SVG object class or something?
> > > Also I don't think it should be in
paintInvalidationRectInLocalCoordinates. Or
> > that method's name should be changed to generalize.
> > 
> > SVG's paint invalidation rect concept needs a big overhaul but I'd like to
save
> > that for a followup.
> 
> I can look into this if you like. I assume we want to move in the direction of
visualOverflowRect.

That would be great!

[pdr., 2015-08-27 17:44:10]

On 2015/08/27 at 15:59:39, wangxianzhu wrote:
> lgtm.
> 
> This remembers me about my *low-priority* CLs:
https://codereview.chromium.org/1278543002/ and
https://codereview.chromium.org/1224933002/. Will appreciate if you can review
them when you have time :)

I was saving those for Chris but I know a bit about this code now. I'll take a
look.

[pdr., 2015-08-27 17:44:12]

The CQ bit was checked by pdr@chromium.org

[chrishtr, 2015-08-27 17:44:18]

On 2015/08/27 at 17:42:36, pdr wrote:
> On 2015/08/27 at 10:23:17, fs wrote:
> > lgtm
> > 
> >
https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/ObjectPai...
> > File Source/core/paint/ObjectPainter.cpp (right):
> > 
> >
https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/ObjectPai...
> > Source/core/paint/ObjectPainter.cpp:20: LayoutRect
ObjectPainter::outlineRectForSVG(const LayoutSize& size, const ComputedStyle&
style)
> > On 2015/08/27 05:00:17, pdr wrote:
> > > On 2015/08/27 at 04:20:41, chrishtr wrote:
> > > > This seems like an odd method to have, even temporarily. Why not just
define
> > > it on the root SVG object class or something?
> > > > Also I don't think it should be in
paintInvalidationRectInLocalCoordinates. Or
> > > that method's name should be changed to generalize.
> > > 
> > > SVG's paint invalidation rect concept needs a big overhaul but I'd like to
save
> > > that for a followup.
> > 
> > I can look into this if you like. I assume we want to move in the direction
of visualOverflowRect.
> 
> That would be great!

Cool! Please to file bug.

[commit-bot: I haz the power, 2015-08-27 17:44:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1321613002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1321613002/20001

[commit-bot: I haz the power, 2015-08-27 19:12:23]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201326

[pdr., 2015-08-27 23:37:05]

On 2015/08/27 at 10:23:17, fs wrote:
> lgtm
> 
>
https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/ObjectPai...
> File Source/core/paint/ObjectPainter.cpp (right):
> 
>
https://codereview.chromium.org/1321613002/diff/1/Source/core/paint/ObjectPai...
> Source/core/paint/ObjectPainter.cpp:20: LayoutRect
ObjectPainter::outlineRectForSVG(const LayoutSize& size, const ComputedStyle&
style)
> On 2015/08/27 05:00:17, pdr wrote:
> > On 2015/08/27 at 04:20:41, chrishtr wrote:
> > > This seems like an odd method to have, even temporarily. Why not just
define
> > it on the root SVG object class or something?
> > > Also I don't think it should be in
paintInvalidationRectInLocalCoordinates. Or
> > that method's name should be changed to generalize.
> > 
> > SVG's paint invalidation rect concept needs a big overhaul but I'd like to
save
> > that for a followup.
> 
> I can look into this if you like. I assume we want to move in the direction of
visualOverflowRect.

I now see that Xianzhu's https://codereview.chromium.org/1224933002 actually
cleans this up nicely by moving the outset code to addOutlineRects.

I've filed crbug.com/525833 to investigate cleaning up
paintInvalidationRectInLocalCoordinates. I'm not sure how far we should go in
this area.. some bikeshedding may be all that's needed.