Draw recorded content directly into the containing PaintCanvas, when possible.

Draw recorded content directly into the containing PaintCanvas, when possible.

For SPv2, this avoids the cost of replaying into a PaintRecord, then drawing
that into the parent canvas. Instead, we can draw the cc::DisplayItemList
directly into it with no additional cost.

This fixes one SPv2 test with SVG, because somehow content got culled out during
the journey through a PaintRecord.

BUG=665259, 703231
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2791043002
Cr-Commit-Position: refs/heads/master@{#461338}
Committed: https://chromium.googlesource.com/chromium/src/+/dad2300ece2848716c16c70bf46d8ffd64e3abee

[chrishtr, 2017-03-31 23:00:22]

Description was changed from

==========
none


none


none


none


none


none


none

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

to

==========
none


none


none


none


none


none


none

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

[chrishtr, 2017-03-31 23:01:20]

Description was changed from

==========
none


none


none


none


none


none


none

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

to

==========
Draw recorded content directly into the containing PaintCanvas, when possible.

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

[chrishtr, 2017-03-31 23:02:31]

Description was changed from

==========
Draw recorded content directly into the containing PaintCanvas, when possible.

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

to

==========
Draw recorded content directly into the containing PaintCanvas, when possible.

For SPv2, this avoids the cost of replaying into a PaintRecord, then drawing
that into the parent canvas. Instead, we can draw the cc::DisplayItemList
directly into it with no additional cost.

This fixes one SPv2 test with SVG, because somehow content got culled out during
the journey through a PaintRecord.

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

[chrishtr, 2017-03-31 23:04:09]

Description was changed from

==========
Draw recorded content directly into the containing PaintCanvas, when possible.

For SPv2, this avoids the cost of replaying into a PaintRecord, then drawing
that into the parent canvas. Instead, we can draw the cc::DisplayItemList
directly into it with no additional cost.

This fixes one SPv2 test with SVG, because somehow content got culled out during
the journey through a PaintRecord.

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

to

==========
Draw recorded content directly into the containing PaintCanvas, when possible.

For SPv2, this avoids the cost of replaying into a PaintRecord, then drawing
that into the parent canvas. Instead, we can draw the cc::DisplayItemList
directly into it with no additional cost.

This fixes one SPv2 test with SVG, because somehow content got culled out during
the journey through a PaintRecord.

BUG=665259,703231
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-03-31 23:04:15]

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

[wkorman, 2017-04-01 00:32:40]

https://codereview.chromium.org/2791043002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/2791043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:441: void
SVGImage::paintCurrentFrame(const FloatRect& bounds,
This looks identical to SVGImage::paintRecordForCurrentFrame (which I assume is
still needed/used for its callsite in applyShaderInternal) except for taking
canvas param, can we share the code in one method and use a default param w/
nullptr check or similar to just specialize endRecording?

https://codereview.chromium.org/2791043002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp
(right):

https://codereview.chromium.org/2791043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp:101:
std::unique_ptr<GeometryMapper> geometryMapper = GeometryMapper::create();
Separate from this change but commenting while reading -- why do we disallow
stack allocation of GeometryMapper which seems like it would be beneficial for
cases like this?

I think (conv w/ Vlad a while back) that at least parts of cc were moving away
from Create methods like we have currently for GeometryMapper where ctor is
simple like this and takes no params. Can discuss further in person sometime.

[chrishtr, 2017-04-01 04:05:25]

https://codereview.chromium.org/2791043002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp
(right):

https://codereview.chromium.org/2791043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp:101:
std::unique_ptr<GeometryMapper> geometryMapper = GeometryMapper::create();
On 2017/04/01 at 00:32:40, wkorman wrote:
> Separate from this change but commenting while reading -- why do we disallow
stack allocation of GeometryMapper which seems like it would be beneficial for
cases like this?
> 
> I think (conv w/ Vlad a while back) that at least parts of cc were moving away
from Create methods like we have currently for GeometryMapper where ctor is
simple like this and takes no params. Can discuss further in person sometime.

A good question. I don't think there is a good reason.

[chrishtr, 2017-04-01 04:05:47]

https://codereview.chromium.org/2791043002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/2791043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:441: void
SVGImage::paintCurrentFrame(const FloatRect& bounds,
On 2017/04/01 at 00:32:40, wkorman wrote:
> This looks identical to SVGImage::paintRecordForCurrentFrame (which I assume
is still needed/used for its callsite in applyShaderInternal) except for taking
canvas param, can we share the code in one method and use a default param w/
nullptr check or similar to just specialize endRecording?

Good idea, done.

[chrishtr, 2017-04-01 04:20:03]

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

[commit-bot: I haz the power, 2017-04-01 04:20:07]

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

[wkorman, 2017-04-01 05:25:55]

lgtm

[commit-bot: I haz the power, 2017-04-01 05:33:47]

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

[commit-bot: I haz the power, 2017-04-01 05:33:49]

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, 2017-04-01 16:19:52]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-01 16:19:53]

https://codereview.chromium.org/2791043002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2791043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:150: SkiaPaintCanvas
skiaPaintCanvas(surface->getCanvas());
Updated this method to draw directly into the bitmap canvas rather than
an intermediate PaintRecorder, which was unnecessary. It also fixed two
tests, because deviceBounds was an incorrect cull rect for the PaintRecorder
it appears.

[chrishtr, 2017-04-01 16:19:53]

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

[commit-bot: I haz the power, 2017-04-01 16:20:04]

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, 2017-04-01 16:21:53]

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

[commit-bot: I haz the power, 2017-04-01 16:21:55]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[chrishtr, 2017-04-01 17:06:35]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-01 17:06:36]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2791043002/#ps100001
(title: "none")

[commit-bot: I haz the power, 2017-04-01 17:06:46]

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, 2017-04-01 17:45:44]

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

[commit-bot: I haz the power, 2017-04-01 17:45:45]

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, 2017-04-01 18:17:35]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-01 18:17:35]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2791043002/#ps50015
(title: "none")

[commit-bot: I haz the power, 2017-04-01 18:17:44]

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, 2017-04-01 19:13:32]

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

[commit-bot: I haz the power, 2017-04-01 19:13:34]

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_...)

[chrishtr, 2017-04-02 03:45:33]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-02 03:45:33]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2791043002/#ps170001
(title: "none")

[commit-bot: I haz the power, 2017-04-02 03:45:41]

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, 2017-04-02 05:17:33]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1491104733513700,
"parent_rev": "6d41b82585204a17ce52137c71722122221be72b", "commit_rev":
"dad2300ece2848716c16c70bf46d8ffd64e3abee"}

[commit-bot: I haz the power, 2017-04-02 05:19:04]

Description was changed from

==========
Draw recorded content directly into the containing PaintCanvas, when possible.

For SPv2, this avoids the cost of replaying into a PaintRecord, then drawing
that into the parent canvas. Instead, we can draw the cc::DisplayItemList
directly into it with no additional cost.

This fixes one SPv2 test with SVG, because somehow content got culled out during
the journey through a PaintRecord.

BUG=665259,703231
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Draw recorded content directly into the containing PaintCanvas, when possible.

For SPv2, this avoids the cost of replaying into a PaintRecord, then drawing
that into the parent canvas. Instead, we can draw the cc::DisplayItemList
directly into it with no additional cost.

This fixes one SPv2 test with SVG, because somehow content got culled out during
the journey through a PaintRecord.

BUG=665259,703231
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2791043002
Cr-Commit-Position: refs/heads/master@{#461338}
Committed:
https://chromium.googlesource.com/chromium/src/+/dad2300ece2848716c16c70bf46d...
==========

[commit-bot: I haz the power, 2017-04-02 05:19:06]

Committed patchset #10 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/dad2300ece2848716c16c70bf46d...