Enable flattening of SkRecordedDrawable

Enable flattening of SkRecordedDrawable

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1913843002

Committed: https://skia.googlesource.com/skia/+/d636950e8ecba89c0021ce4e76037c67aa6ee2cd

Committed: https://skia.googlesource.com/skia/+/95416f477d2ca264f2fb065455e95778a2289a1d

[msarett, 2016-04-25 13:50:14]

Description was changed from

==========
Enable flattening of SkRecordedDrawable

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

to

==========
Enable flattening of SkRecordedDrawable

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-04-25 14:06:50]

msarett@google.com changed reviewers:
+ djsollen@google.com, mtklein@google.com

[msarett, 2016-04-25 14:06:51]

https://codereview.chromium.org/1913843002/diff/1/src/core/SkRecordedDrawable...
File src/core/SkRecordedDrawable.cpp (right):

https://codereview.chromium.org/1913843002/diff/1/src/core/SkRecordedDrawable...
src/core/SkRecordedDrawable.cpp:55: void
SkRecordedDrawable::flatten(SkWriteBuffer& buffer) const {
flatten() and CreateProc() are the new functions - the others are moved from
SkPictureRecorder.cpp.

[msarett, 2016-04-25 16:40:38]

https://codereview.chromium.org/1913843002/diff/40001/src/core/SkReadBuffer.cpp
File src/core/SkReadBuffer.cpp (right):

https://codereview.chromium.org/1913843002/diff/40001/src/core/SkReadBuffer.c...
src/core/SkReadBuffer.cpp:427: static sk_sp<SkFlattenable>
unflatten_recorded_drawable(SkReadBuffer& buffer) {
Maybe this function belongs in a different file?

[msarett, 2016-04-26 21:29:32]

Any thoughts on this?

[djsollen, 2016-04-27 13:05:40]

https://codereview.chromium.org/1913843002/diff/40001/tests/FlattenDrawableTe...
File tests/FlattenDrawableTest.cpp (right):

https://codereview.chromium.org/1913843002/diff/40001/tests/FlattenDrawableTe...
tests/FlattenDrawableTest.cpp:282: readBuffer.initDrawableFactories();
seems awkward that I need to need to register factories for classes that already
live in Skia.  Why not have SkRecordedDrawable in the global registry?

[msarett, 2016-04-27 13:24:17]

https://codereview.chromium.org/1913843002/diff/40001/tests/FlattenDrawableTe...
File tests/FlattenDrawableTest.cpp (right):

https://codereview.chromium.org/1913843002/diff/40001/tests/FlattenDrawableTe...
tests/FlattenDrawableTest.cpp:282: readBuffer.initDrawableFactories();
On 2016/04/27 13:05:40, djsollen wrote:
> seems awkward that I need to need to register factories for classes that
already
> live in Skia.  Why not have SkRecordedDrawable in the global registry?

That was original approach in Patch Sets 1 and 2.  I think this is a good
solution, I'm happy with it as well.

The idea behind Patch Set 3 is that we don't need to depend on the global
registry now that we have another way to register unflatten procs.  And I think
we ought to prefer the new approach to using the global registry when we can. 
Though I'm not sure we'll be able to drop the global registry completely.

Another alternative would be to register SkRecordedDrawable on construction of
the SkReadBuffer.  This has the advantage that you don't have to make a function
call to register anything, but it means that all users of SkReadBuffer
(including those that don't care about SkDrawable) will get SkRecordedDrawable's
create proc.

[msarett, 2016-04-27 13:24:46]

msarett@google.com changed reviewers:
+ reed@google.com

[msarett, 2016-04-27 13:24:46]

+reed

[msarett, 2016-04-27 13:40:21]

Patch Set 4 is the same as Patch Set 1, it uses the global registry.

As Derek and I discussed, let's try to stay consistent with how we register Skia
classes.  If we want to make a change, we ought to consider it for all
flattenables (rather than having drawables be a separate speical case).

[djsollen, 2016-04-27 13:55:13]

lgtm

[msarett, 2016-04-27 14:18:31]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 14:18:33]

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

[commit-bot: I haz the power, 2016-04-27 14:19:43]

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

[commit-bot: I haz the power, 2016-04-27 14:19:44]

Dry run: Try jobs failed on following builders:
  Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)

[msarett, 2016-04-27 14:25:59]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 14:26:03]

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

[commit-bot: I haz the power, 2016-04-27 14:28:33]

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

[commit-bot: I haz the power, 2016-04-27 14:28:34]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[msarett, 2016-04-27 14:35:07]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 14:35:09]

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

[commit-bot: I haz the power, 2016-04-27 14:36:40]

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

[commit-bot: I haz the power, 2016-04-27 14:36:41]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[msarett, 2016-04-27 15:42:21]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 15:42:31]

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

[commit-bot: I haz the power, 2016-04-27 15:58:42]

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

[commit-bot: I haz the power, 2016-04-27 15:58:44]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-04-27 16:02:12]

The CQ bit was checked by msarett@google.com

[msarett, 2016-04-27 16:02:12]

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1913843002/#ps140001
(title: "Avoid double free")

[commit-bot: I haz the power, 2016-04-27 16:02:21]

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

[commit-bot: I haz the power, 2016-04-27 16:03:19]

Description was changed from

==========
Enable flattening of SkRecordedDrawable

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Enable flattening of SkRecordedDrawable

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/d636950e8ecba89c0021ce4e76037c67aa6ee2cd
==========

[commit-bot: I haz the power, 2016-04-27 16:03:20]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/d636950e8ecba89c0021ce4e76037c67aa6ee2cd

[mtklein, 2016-04-27 16:29:27]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1923393002/ by mtklein@google.com.

The reason for reverting is: Release mode bots appear to be crashing while
loading .skp files in nanobench..

[reed1, 2016-04-27 16:29:43]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1925583005/ by reed@google.com.

The reason for reverting is: nanobench failing (skps ?).

[mtklein, 2016-04-27 16:30:31]

On 2016/04/27 at 16:29:27, mtklein wrote:
> A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1923393002/ by mtklein@google.com.
> 
> The reason for reverting is: Release mode bots appear to be crashing while
loading .skp files in nanobench..

In Debug mode we're hitting an assert:

../../src/core/SkReader32.h:87: fatal error: ""fCurr <= fStop""

[msarett, 2016-04-27 20:01:26]

Description was changed from

==========
Enable flattening of SkRecordedDrawable

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/d636950e8ecba89c0021ce4e76037c67aa6ee2cd
==========

to

==========
Enable flattening of SkRecordedDrawable

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/d636950e8ecba89c0021ce4e76037c67aa6ee2cd
==========

[msarett, 2016-04-27 20:33:40]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 20:33:45]

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

[commit-bot: I haz the power, 2016-04-27 20:49:35]

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

[commit-bot: I haz the power, 2016-04-27 20:49:36]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-04-27 20:50:20]

The CQ bit was checked by msarett@google.com

[msarett, 2016-04-27 20:50:21]

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1913843002/#ps160001
(title: "Fix backward compatibility")

[commit-bot: I haz the power, 2016-04-27 20:50:23]

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

[commit-bot: I haz the power, 2016-04-27 20:51:23]

Description was changed from

==========
Enable flattening of SkRecordedDrawable

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/d636950e8ecba89c0021ce4e76037c67aa6ee2cd
==========

to

==========
Enable flattening of SkRecordedDrawable

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/d636950e8ecba89c0021ce4e76037c67aa6ee2cd

Committed:
https://skia.googlesource.com/skia/+/95416f477d2ca264f2fb065455e95778a2289a1d
==========

[commit-bot: I haz the power, 2016-04-27 20:51:24]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/95416f477d2ca264f2fb065455e95778a2289a1d