Use sk_sp-based picture recording APIs in Blink

Use sk_sp-based picture recording APIs in Blink

1) use SkPictureRecorder::finishRecordingAsPicture() over endRecordingAsPicture()

2) store as sk_sp<SkPicture> (when localized) or convert to (Pass)RefPtr (when retained in Blink).

(+ a couple of Chromium clients which were missed in prev CLs)

BUG=skia:5077, chromium:597634
Committed: https://crrev.com/9e89d089491acaa32fe1d83f60eae0511811e5a7
Cr-Commit-Position: refs/heads/master@{#383496}

[f(malita), 2016-03-24 17:58:07]

Description was changed from

==========
Use sk_sp-based picture recording APIs in Blink

WIP

BUG=skia:5077
==========

to

==========
Use sk_sp-based picture recording APIs in Blink

1) use SkPictureRecorder::finishRecordingAsPicture() over
endRecordingAsPicture()

2) store as sk_sp<SkPicture> (when localized) or convert to (Pass)RefPtr (when
retained in Blink).

BUG=skia:5077,chromium:597634
==========

[f(malita), 2016-03-24 17:58:07]

fmalita@chromium.org changed reviewers:
+ reed@google.com, senorblanco@chromium.org, tomhudson@chromium.org

[f(malita), 2016-03-24 18:04:09]

Description was changed from

==========
Use sk_sp-based picture recording APIs in Blink

1) use SkPictureRecorder::finishRecordingAsPicture() over
endRecordingAsPicture()

2) store as sk_sp<SkPicture> (when localized) or convert to (Pass)RefPtr (when
retained in Blink).

BUG=skia:5077,chromium:597634
==========

to

==========
Use sk_sp-based picture recording APIs in Blink

1) use SkPictureRecorder::finishRecordingAsPicture() over
endRecordingAsPicture()

2) store as sk_sp<SkPicture> (when localized) or convert to (Pass)RefPtr (when
retained in Blink).

(+ a couple of Chromium clients which were missed in prev CLs)

BUG=skia:5077,chromium:597634
==========

[f(malita), 2016-03-24 18:33:39]

I started off with a more ambitious approach (PS#1,2), but that created some
friction.

Probably makes sense to land the minimal version (update API boundaries and
local objects only), and then revisit whether it's worth exposing sk_sp in
higher Blink APIs.

PTAL, thanks!

[Stephen White, 2016-03-24 19:05:57]

LGTM

https://codereview.chromium.org/1823133002/diff/60001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/1823133002/diff/60001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:332: return
skia::AdoptRef(emptyRecorder.finishRecordingAsPicture());
Out of curiosity, are there plans to place skia::RefPtr<> with something else at
some point (sk_sp<>)? Or are we going to keep it?

[f(malita), 2016-03-24 19:17:10]

https://codereview.chromium.org/1823133002/diff/60001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/1823133002/diff/60001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:332: return
skia::AdoptRef(emptyRecorder.finishRecordingAsPicture());
On 2016/03/24 19:05:57, Stephen White wrote:
> Out of curiosity, are there plans to place skia::RefPtr<> with something else
at
> some point (sk_sp<>)? Or are we going to keep it?

Now that we have sk_sp I think it makes perfect sense to retire skia::RefPtr: no
reason to maintain a Skia-specific smart ptr when Skia provides one with
equivalent semantics.

(the process has already started - as we update Chromium to the new APIs, we're
also pushing sk_sp as replacement, e.g.
https://codereview.chromium.org/1819683002)

[Stephen White, 2016-03-24 19:42:54]

https://codereview.chromium.org/1823133002/diff/60001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/1823133002/diff/60001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:332: return
skia::AdoptRef(emptyRecorder.finishRecordingAsPicture());
On 2016/03/24 19:17:10, f(malita) wrote:
> On 2016/03/24 19:05:57, Stephen White wrote:
> > Out of curiosity, are there plans to place skia::RefPtr<> with something
else
> at
> > some point (sk_sp<>)? Or are we going to keep it?
> 
> Now that we have sk_sp I think it makes perfect sense to retire skia::RefPtr:
no
> reason to maintain a Skia-specific smart ptr when Skia provides one with
> equivalent semantics.
> 
> (the process has already started - as we update Chromium to the new APIs,
we're
> also pushing sk_sp as replacement, e.g.
> https://codereview.chromium.org/1819683002)
> 

In that case, should this function be returning an sk_sp<SkPicture>?

[tomhudson, 2016-03-24 19:52:39]

tomhudson@google.com changed reviewers:
+ tomhudson@google.com

[tomhudson, 2016-03-24 19:52:41]

https://codereview.chromium.org/1823133002/diff/60001/content/renderer/skia_b...
File content/renderer/skia_benchmarking_extension.cc (right):

https://codereview.chromium.org/1823133002/diff/60001/content/renderer/skia_b...
content/renderer/skia_benchmarking_extension.cc:43: skia::RefPtr<SkPicture>
picture;
We could change this to sk_sp<>?

https://codereview.chromium.org/1823133002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp
(right):

https://codereview.chromium.org/1823133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp:188:
sk_sp<SkPicture> picture = recorder.finishRecordingAsPicture();
We can fold these (two) calls into the drawPicture() calls and get rid of the
temps, right? I thought that was a pattern Mike was encouraging.

(Wasn't encouraging it in e.g. drawIntoSkPicture because I'm assuming they want
to test their invariants between picture recording and picture draw.)

[f(malita), 2016-03-24 20:22:41]

https://codereview.chromium.org/1823133002/diff/60001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/1823133002/diff/60001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:332: return
skia::AdoptRef(emptyRecorder.finishRecordingAsPicture());
On 2016/03/24 19:42:54, Stephen White wrote:
> On 2016/03/24 19:17:10, f(malita) wrote:
> > On 2016/03/24 19:05:57, Stephen White wrote:
> > > Out of curiosity, are there plans to place skia::RefPtr<> with something
> else
> > at
> > > some point (sk_sp<>)? Or are we going to keep it?
> > 
> > Now that we have sk_sp I think it makes perfect sense to retire
skia::RefPtr:
> no
> > reason to maintain a Skia-specific smart ptr when Skia provides one with
> > equivalent semantics.
> > 
> > (the process has already started - as we update Chromium to the new APIs,
> we're
> > also pushing sk_sp as replacement, e.g.
> > https://codereview.chromium.org/1819683002)
> > 
> 
> In that case, should this function be returning an sk_sp<SkPicture>?

It should, but I would rather not pull on this particular thread atm because a)
sk_sp is not assignable to skia::RefPtr (we should prolly add that to ease the
migration), and b) I'm hesitant to touch webview interfaces since I don't have a
good way to test locally (and am suspicious of bot coverage).

https://codereview.chromium.org/1823133002/diff/60001/content/renderer/skia_b...
File content/renderer/skia_benchmarking_extension.cc (right):

https://codereview.chromium.org/1823133002/diff/60001/content/renderer/skia_b...
content/renderer/skia_benchmarking_extension.cc:43: skia::RefPtr<SkPicture>
picture;
On 2016/03/24 19:52:40, tomhudson wrote:
> We could change this to sk_sp<>?

Good point, it's a local type.  Done.

https://codereview.chromium.org/1823133002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp
(right):

https://codereview.chromium.org/1823133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp:188:
sk_sp<SkPicture> picture = recorder.finishRecordingAsPicture();
On 2016/03/24 19:52:40, tomhudson wrote:
> We can fold these (two) calls into the drawPicture() calls and get rid of the
> temps, right? I thought that was a pattern Mike was encouraging.

Right, that's the optimal pattern - but with tests I tend to get lazy :)

Done.

[tomhudson, 2016-03-24 20:44:13]

lgtm

[f(malita), 2016-03-28 13:42:24]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2016-03-28 13:42:24]

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

[commit-bot: I haz the power, 2016-03-28 13:42:33]

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

[commit-bot: I haz the power, 2016-03-28 13:50:23]

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

[commit-bot: I haz the power, 2016-03-28 13:50:25]

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

[f(malita), 2016-03-28 14:14:51]

fmalita@chromium.org changed reviewers:
+ avi@chromium.org, boliu@chromium.org

[f(malita), 2016-03-28 14:15:55]

+ boliu & avi for android_webview, content/renderer RS.

[Avi (use Gerrit), 2016-03-28 15:22:29]

lgtm

stamp

[boliu, 2016-03-28 15:39:09]

rubberstamp lgtm

[f(malita), 2016-03-28 15:39:49]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2016-03-28 15:39:57]

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

[commit-bot: I haz the power, 2016-03-28 15:44:57]

Description was changed from

==========
Use sk_sp-based picture recording APIs in Blink

1) use SkPictureRecorder::finishRecordingAsPicture() over
endRecordingAsPicture()

2) store as sk_sp<SkPicture> (when localized) or convert to (Pass)RefPtr (when
retained in Blink).

(+ a couple of Chromium clients which were missed in prev CLs)

BUG=skia:5077,chromium:597634
==========

to

==========
Use sk_sp-based picture recording APIs in Blink

1) use SkPictureRecorder::finishRecordingAsPicture() over
endRecordingAsPicture()

2) store as sk_sp<SkPicture> (when localized) or convert to (Pass)RefPtr (when
retained in Blink).

(+ a couple of Chromium clients which were missed in prev CLs)

BUG=skia:5077,chromium:597634
==========

[commit-bot: I haz the power, 2016-03-28 15:44:59]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-28 15:46:14]

Description was changed from

==========
Use sk_sp-based picture recording APIs in Blink

1) use SkPictureRecorder::finishRecordingAsPicture() over
endRecordingAsPicture()

2) store as sk_sp<SkPicture> (when localized) or convert to (Pass)RefPtr (when
retained in Blink).

(+ a couple of Chromium clients which were missed in prev CLs)

BUG=skia:5077,chromium:597634
==========

to

==========
Use sk_sp-based picture recording APIs in Blink

1) use SkPictureRecorder::finishRecordingAsPicture() over
endRecordingAsPicture()

2) store as sk_sp<SkPicture> (when localized) or convert to (Pass)RefPtr (when
retained in Blink).

(+ a couple of Chromium clients which were missed in prev CLs)

BUG=skia:5077,chromium:597634

Committed: https://crrev.com/9e89d089491acaa32fe1d83f60eae0511811e5a7
Cr-Commit-Position: refs/heads/master@{#383496}
==========

[commit-bot: I haz the power, 2016-03-28 15:46:15]

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