Clean up calls to ToSkPicture

Clean up calls to ToSkPicture

In the future, PaintRecord is going to become a real class, and so a
function that takes in a raw PaintRecord* and returns an SkPicture*
doesn't make much sense, as it'll really be an ownership transfer.
Therefore, remove the callers that use the raw pointer version.

ToSkPicture in the future is going to be a playback instead of just
a cast and will not be free.  So, this also cleans up two callsites
that do not have to use drawPicture(ToSkPicture) and could instead use
PaintRecord::playback(SkCanvas*).

However, drawPicture has some optional parameters, but there's only
one callsite that uses it (!!!) so just hoist a single saveLayer call
up and make the PaintCavnas API more clean.  This is baking what
SkAutoCanvasMatrixPaint does in SkCanvas.cpp into that callsite
in PaintGeneratedImage.

BUG=671433
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[enne (OOO), 2017-03-21 23:05:28]

Description was changed from

==========
Clean up calls to ToSkPicture

In the future, PaintRecord is going to become a real class, and so a
function that takes in a raw PaintRecord* and returns an SkPicture*
doesn't make much sense, as it'll really be an ownership transfer.
Therefore, remove the callers that use the raw pointer version.

ToSkPicture in the future is going to be a playback instead of just
a cast and will not be free.  So, this also cleans up two callsites
that do not have to use drawPicture(ToSkPicture) and could instead use
PaintRecord::playback(SkCanvas*).

However, drawPicture has some optional parameters, but there's only
one callsite that uses it (!!!) so just hoist a single saveLayer call
up and make the PaintCavnas API more clean.  This is baking what
SkAutoCanvasMatrixPaint does in SkCanvas.cpp into that callsite
in PaintGeneratedImage.

BUG=671433
==========

to

==========
Clean up calls to ToSkPicture

In the future, PaintRecord is going to become a real class, and so a
function that takes in a raw PaintRecord* and returns an SkPicture*
doesn't make much sense, as it'll really be an ownership transfer.
Therefore, remove the callers that use the raw pointer version.

ToSkPicture in the future is going to be a playback instead of just
a cast and will not be free.  So, this also cleans up two callsites
that do not have to use drawPicture(ToSkPicture) and could instead use
PaintRecord::playback(SkCanvas*).

However, drawPicture has some optional parameters, but there's only
one callsite that uses it (!!!) so just hoist a single saveLayer call
up and make the PaintCavnas API more clean.  This is baking what
SkAutoCanvasMatrixPaint does in SkCanvas.cpp into that callsite
in PaintGeneratedImage.

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

[enne (OOO), 2017-03-21 23:05:29]

Description was changed from

==========
Clean up calls to ToSkPicture

In the future, PaintRecord is going to become a real class, and so a
function that takes in a raw PaintRecord* and returns an SkPicture*
doesn't make much sense, as it'll really be an ownership transfer.
Therefore, remove the callers that use the raw pointer version.

ToSkPicture in the future is going to be a playback instead of just
a cast and will not be free.  So, this also cleans up two callsites
that do not have to use drawPicture(ToSkPicture) and could instead use
PaintRecord::playback(SkCanvas*).

However, drawPicture has some optional parameters, but there's only
one callsite that uses it (!!!) so just hoist a single saveLayer call
up and make the PaintCavnas API more clean.  This is baking what
SkAutoCanvasMatrixPaint does in SkCanvas.cpp into that callsite
in PaintGeneratedImage.

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

to

==========
Clean up calls to ToSkPicture

In the future, PaintRecord is going to become a real class, and so a
function that takes in a raw PaintRecord* and returns an SkPicture*
doesn't make much sense, as it'll really be an ownership transfer.
Therefore, remove the callers that use the raw pointer version.

ToSkPicture in the future is going to be a playback instead of just
a cast and will not be free.  So, this also cleans up two callsites
that do not have to use drawPicture(ToSkPicture) and could instead use
PaintRecord::playback(SkCanvas*).

However, drawPicture has some optional parameters, but there's only
one callsite that uses it (!!!) so just hoist a single saveLayer call
up and make the PaintCavnas API more clean.  This is baking what
SkAutoCanvasMatrixPaint does in SkCanvas.cpp into that callsite
in PaintGeneratedImage.

BUG=671433
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-03-21 23:05:48]

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

[commit-bot: I haz the power, 2017-03-21 23:06:36]

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

[enne (OOO), 2017-03-21 23:08:10]

enne@chromium.org changed reviewers:
+ danakj@chromium.org, pdr@chromium.org, vmpstr@chromium.org

[enne (OOO), 2017-03-21 23:08:11]

https://codereview.chromium.org/2769533002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/PaintGeneratedImage.cpp
(right):

https://codereview.chromium.org/2769533002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/PaintGeneratedImage.cpp:27:
canvas->saveLayer(&bounds, &flags);
Tested this change via LayoutTests/csspaint.

https://codereview.chromium.org/2769533002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItem.cpp
(right):

https://codereview.chromium.org/2769533002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItem.cpp:56:
static bool recordsEqual(sk_sp<const PaintRecord> record1,
danakj: Back to where we were before in the other patch, sorry, but if it's
going to ToSkPicture, then at least for now it needs to be an sk_sp and not a
raw pointer, at least so far as I can see.

[commit-bot: I haz the power, 2017-03-21 23:57:47]

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

[commit-bot: I haz the power, 2017-03-21 23:57:48]

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

[vmpstr, 2017-03-22 21:00:12]

cc lgtm

https://codereview.chromium.org/2769533002/diff/1/cc/paint/display_item_list.cc
File cc/paint/display_item_list.cc (right):

https://codereview.chromium.org/2769533002/diff/1/cc/paint/display_item_list....
cc/paint/display_item_list.cc:105: item.picture->playback(canvas, callback);
IIRC this was there as a performance optimization, so look out for any possible
regressions. But, tbh, I don't think this would make much difference.

[enne (OOO), 2017-03-23 16:43:11]

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

[enne (OOO), 2017-03-23 16:43:12]

https://codereview.chromium.org/2769533002/diff/1/cc/paint/display_item_list.cc
File cc/paint/display_item_list.cc (right):

https://codereview.chromium.org/2769533002/diff/1/cc/paint/display_item_list....
cc/paint/display_item_list.cc:105: item.picture->playback(canvas, callback);
On 2017/03/22 at 21:00:11, vmpstr wrote:
> IIRC this was there as a performance optimization, so look out for any
possible regressions. But, tbh, I don't think this would make much difference.

I think it does when the SkCanvas is recording or intercepting things.  However,
for raster, as far as I can tell SkCanvas::onDrawPicture just calls
SkPicture::playback.

[commit-bot: I haz the power, 2017-03-23 16:43:39]

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, 2017-03-23 19:28:29]

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

[commit-bot: I haz the power, 2017-03-23 19:28:30]

Dry run: This issue passed the CQ dry run.

[pdr., 2017-03-23 20:37:41]

On 2017/03/23 at 16:43:12, enne wrote:
>
https://codereview.chromium.org/2769533002/diff/1/cc/paint/display_item_list.cc
> File cc/paint/display_item_list.cc (right):
> 
>
https://codereview.chromium.org/2769533002/diff/1/cc/paint/display_item_list....
> cc/paint/display_item_list.cc:105: item.picture->playback(canvas, callback);
> On 2017/03/22 at 21:00:11, vmpstr wrote:
> > IIRC this was there as a performance optimization, so look out for any
possible regressions. But, tbh, I don't think this would make much difference.
> 
> I think it does when the SkCanvas is recording or intercepting things. 
However, for raster, as far as I can tell SkCanvas::onDrawPicture just calls
SkPicture::playback.

LGTM

[enne (OOO), 2017-03-23 20:40:02]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2017-03-23 20:40:03]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/2769533002/#ps20001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-23 20:40: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, 2017-03-23 20:46:48]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490301602822660,
"parent_rev": "2f5d0019eb2af26b459bf0b7e42c74107aa3987a", "commit_rev":
"dffda5006efcf5d473530268ff2da14f43e41cc3"}

[commit-bot: I haz the power, 2017-03-23 20:47:34]

Description was changed from

==========
Clean up calls to ToSkPicture

In the future, PaintRecord is going to become a real class, and so a
function that takes in a raw PaintRecord* and returns an SkPicture*
doesn't make much sense, as it'll really be an ownership transfer.
Therefore, remove the callers that use the raw pointer version.

ToSkPicture in the future is going to be a playback instead of just
a cast and will not be free.  So, this also cleans up two callsites
that do not have to use drawPicture(ToSkPicture) and could instead use
PaintRecord::playback(SkCanvas*).

However, drawPicture has some optional parameters, but there's only
one callsite that uses it (!!!) so just hoist a single saveLayer call
up and make the PaintCavnas API more clean.  This is baking what
SkAutoCanvasMatrixPaint does in SkCanvas.cpp into that callsite
in PaintGeneratedImage.

BUG=671433
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Clean up calls to ToSkPicture

In the future, PaintRecord is going to become a real class, and so a
function that takes in a raw PaintRecord* and returns an SkPicture*
doesn't make much sense, as it'll really be an ownership transfer.
Therefore, remove the callers that use the raw pointer version.

ToSkPicture in the future is going to be a playback instead of just
a cast and will not be free.  So, this also cleans up two callsites
that do not have to use drawPicture(ToSkPicture) and could instead use
PaintRecord::playback(SkCanvas*).

However, drawPicture has some optional parameters, but there's only
one callsite that uses it (!!!) so just hoist a single saveLayer call
up and make the PaintCavnas API more clean.  This is baking what
SkAutoCanvasMatrixPaint does in SkCanvas.cpp into that callsite
in PaintGeneratedImage.

BUG=671433
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2017-03-23 20:47:35]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/dffda5006efcf5d473530268ff2d...