Use GeometryMapper in PaintInvalidator

Use GeometryMapper in PaintInvalidator

BUG=510908
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/20b2f4f14209404d0b124c56ccd26ea5bbfc6c3b
Cr-Commit-Position: refs/heads/master@{#412877}

[Xianzhu, 2016-08-11 23:30:48]

Description was changed from

==========
Update FlagExpectations/enable-slimming-paint-v2

Add 4 tests that constantly fail on try bots.

x


GeometryMapper

BUG=
==========

to

==========
Update FlagExpectations/enable-slimming-paint-v2

Add 4 tests that constantly fail on try bots.

x


GeometryMapper

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-08-11 23:35:11]

Description was changed from

==========
Update FlagExpectations/enable-slimming-paint-v2

Add 4 tests that constantly fail on try bots.

x


GeometryMapper

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Use GeometryMapper in PaintInvalidator

BUG=510908
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-08-12 01:13:38]

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

[commit-bot: I haz the power, 2016-08-12 01:14:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 02:04:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 02:04:58]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[Xianzhu, 2016-08-12 16:14:50]

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

[Xianzhu, 2016-08-12 16:14:51]

Ptal.

The layout test failures on spv2 try bot are not trivial to fix in this CL. Will
file bugs and update spv2 test expectations:
- Filter and reflection extends are applied in mapToVisualRectInAncestorSpace()
in old path. We need to find a new way to apply them in new path.
- We need a property tree way to represent SVGRoot viewport clip.

[pdr., 2016-08-12 17:59:58]

Looks good overall. Those SVG failures are a known issue, sorry you had to find
them :/ Can you update FlagExpectations/slimming-paint-v2-enabled?

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGRootPainter.cpp (right):

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGRootPainter.cpp:32: if (!size.isEmpty())
{
If the size is empty, should we be painting anything? Could we change the
early-out at the beginning from:
if (pixelSnappedSize(paintOffset).isEmpty())
to:
if (m_layoutSVGRoot.size())

?

[chrishtr, 2016-08-12 18:20:53]

Sweet!

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:18: #include
"core/paint/PaintPropertyTreePrinter.h"
Remove

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:42: result =
LayoutRect(GeometryMapper().mapToVisualRectInDestinationSpace(rect,
currentTreeState, containerTreeState, success));
This does use GeometryMapper, but it's not efficient. Note that in a comment
this is just an intermediate step?

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:53: rect =
blink::mapLocalRectToPaintInvalidationBacking(object, FloatRect(rect), *this);
What's the point of not inlining blink::mapLocalRectToPaintInvalidationBacking
here?

[Xianzhu, 2016-08-12 18:22:39]

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGRootPainter.cpp (right):

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGRootPainter.cpp:32: if (!size.isEmpty())
{
On 2016/08/12 17:59:58, pdr. wrote:
> If the size is empty, should we be painting anything? Could we change the
> early-out at the beginning from:
> if (pixelSnappedSize(paintOffset).isEmpty())
> to:
> if (m_layoutSVGRoot.size())
> 
> ?

I wonder if the early return in SVGRootPaint::paint() is correct. What if the
svg root doesn't clip children even if its size is empty?

The added check is for the callsite in
PaintPropertyTreeBuilder::updateSvgLocalToBorderBoxTransform() which doesn't
have the early return like paint().

[pdr., 2016-08-12 18:23:54]

On 2016/08/12 at 18:22:39, wangxianzhu wrote:
>
https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/SVGRootPainter.cpp (right):
> 
>
https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/SVGRootPainter.cpp:32: if
(!size.isEmpty()) {
> On 2016/08/12 17:59:58, pdr. wrote:
> > If the size is empty, should we be painting anything? Could we change the
> > early-out at the beginning from:
> > if (pixelSnappedSize(paintOffset).isEmpty())
> > to:
> > if (m_layoutSVGRoot.size())
> > 
> > ?
> 
> I wonder if the early return in SVGRootPaint::paint() is correct. What if the
svg root doesn't clip children even if its size is empty?
> 
> The added check is for the callsite in
PaintPropertyTreeBuilder::updateSvgLocalToBorderBoxTransform() which doesn't
have the early return like paint().

OK, LGTM % chrishtr's comments

[Xianzhu, 2016-08-12 18:37:51]

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:18: #include
"core/paint/PaintPropertyTreePrinter.h"
On 2016/08/12 18:20:53, chrishtr wrote:
> Remove

Done.

(BTW do you know how to make the functions declared in
PaintPropertyTreePrinter.h available to gdb? I tried them (with and without
blink:: prefix) but gdb can't find the functions. Wondering why the compiler/gdb
treat them differently than showLayoutTree() etc.)

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:42: result =
LayoutRect(GeometryMapper().mapToVisualRectInDestinationSpace(rect,
currentTreeState, containerTreeState, success));
On 2016/08/12 18:20:53, chrishtr wrote:
> This does use GeometryMapper, but it's not efficient. Note that in a comment
> this is just an intermediate step?

Oh I didn't think this is an intermediate step. What's the efficient way?

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:53: rect =
blink::mapLocalRectToPaintInvalidationBacking(object, FloatRect(rect), *this);
On 2016/08/12 18:20:53, chrishtr wrote:
> What's the point of not inlining blink::mapLocalRectToPaintInvalidationBacking
> here?

Two reasons:

1. This method is for non-SVG path of code shared between old and new code which
provide a LayoutRect instead of a FloatRect. If changed the type to FloatRect,
we would have to change the legacy code to also convert LayoutRect to FloatRect.

2. This method is temporary and will be removed when we remove
PaintInvalidationState.

[chrishtr, 2016-08-12 18:40:28]

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:42: result =
LayoutRect(GeometryMapper().mapToVisualRectInDestinationSpace(rect,
currentTreeState, containerTreeState, success));
On 2016/08/12 at 18:37:51, Xianzhu wrote:
> On 2016/08/12 18:20:53, chrishtr wrote:
> > This does use GeometryMapper, but it's not efficient. Note that in a comment
> > this is just an intermediate step?
> 
> Oh I didn't think this is an intermediate step. What's the efficient way?

All of the caching in GeometryMapper is stored as instance variables. So you
should allocate one GeometryMapper object and use it throughout the paint
invalidation tree walk.

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:53: rect =
blink::mapLocalRectToPaintInvalidationBacking(object, FloatRect(rect), *this);
On 2016/08/12 at 18:37:51, Xianzhu wrote:
> On 2016/08/12 18:20:53, chrishtr wrote:
> > What's the point of not inlining
blink::mapLocalRectToPaintInvalidationBacking
> > here?
> 
> Two reasons:
> 
> 1. This method is for non-SVG path of code shared between old and new code
which provide a LayoutRect instead of a FloatRect. If changed the type to
FloatRect, we would have to change the legacy code to also convert LayoutRect to
FloatRect.
> 
> 2. This method is temporary and will be removed when we remove
PaintInvalidationState.

ok

[Xianzhu, 2016-08-12 19:15:00]

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

[commit-bot: I haz the power, 2016-08-12 19:15:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-08-12 19:16:29]

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2238853005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:42: result =
LayoutRect(GeometryMapper().mapToVisualRectInDestinationSpace(rect,
currentTreeState, containerTreeState, success));
On 2016/08/12 18:40:28, chrishtr wrote:
> On 2016/08/12 at 18:37:51, Xianzhu wrote:
> > On 2016/08/12 18:20:53, chrishtr wrote:
> > > This does use GeometryMapper, but it's not efficient. Note that in a
comment
> > > this is just an intermediate step?
> > 
> > Oh I didn't think this is an intermediate step. What's the efficient way?
> 
> All of the caching in GeometryMapper is stored as instance variables. So you
> should allocate one GeometryMapper object and use it throughout the paint
> invalidation tree walk.

Done.

[chrishtr, 2016-08-12 19:46:12]

https://codereview.chromium.org/2238853005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2238853005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:81: point =
GeometryMapper().mapRectToDestinationSpace(FloatRect(point, FloatSize()),
currentTreeState, containerTreeState, success).location();
What about this one?

[commit-bot: I haz the power, 2016-08-12 20:25:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 20:25:42]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[chrishtr, 2016-08-12 23:07:07]

Feel free to commit without my LGTM since I'll be on vacation. Just wanted an
answer to that
last question.

[Xianzhu, 2016-08-13 00:51:16]

https://codereview.chromium.org/2238853005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2238853005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:81: point =
GeometryMapper().mapRectToDestinationSpace(FloatRect(point, FloatSize()),
currentTreeState, containerTreeState, success).location();
On 2016/08/12 19:46:11, chrishtr - OOO wrote:
> What about this one?

Sorry, missed this one. Change to m_geometryMapper.

[Xianzhu, 2016-08-13 00:51:22]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-08-13 00:51:22]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2238853005/#ps80001
(title: ".")

[commit-bot: I haz the power, 2016-08-13 00:51:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-13 00:53:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-13 00:54:01]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2016-08-13 18:12:27]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-08-13 18:12:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2238853005/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-13 18:12:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-13 19:00:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-13 19:00:33]

Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[Xianzhu, 2016-08-15 23:11:00]

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

[commit-bot: I haz the power, 2016-08-15 23:11:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 00:29:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 00:29:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2016-08-17 18:41:35]

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

[commit-bot: I haz the power, 2016-08-17 18:42:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 19:41:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 19:41:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2016-08-17 20:44:48]

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

[commit-bot: I haz the power, 2016-08-17 20:45:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 23:45:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 23:45:46]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Xianzhu, 2016-08-18 16:03:20]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-08-18 16:03:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2238853005/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-18 16:03:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 17:36:53]

Description was changed from

==========
Use GeometryMapper in PaintInvalidator

BUG=510908
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Use GeometryMapper in PaintInvalidator

BUG=510908
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-08-18 17:36:54]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-18 17:41:26]

Description was changed from

==========
Use GeometryMapper in PaintInvalidator

BUG=510908
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Use GeometryMapper in PaintInvalidator

BUG=510908
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/20b2f4f14209404d0b124c56ccd26ea5bbfc6c3b
Cr-Commit-Position: refs/heads/master@{#412877}
==========

[commit-bot: I haz the power, 2016-08-18 17:41:28]

Patchset 9 (id:??) landed as
https://crrev.com/20b2f4f14209404d0b124c56ccd26ea5bbfc6c3b
Cr-Commit-Position: refs/heads/master@{#412877}