Replace EncodeBitmap with an interface.

Replace EncodeBitmap with an interface.

Gives more flexibility to the caller to decide whether to use the
encoded data returned by refEncodedData().

Provides an implementation that supports the old version of
SkPicture::serialize().

TODO: Update Chrome, so we can remove SK_LEGACY_ENCODE_BITMAP entirely

BUG=skia:3190
Committed: https://skia.googlesource.com/skia/+/895c43b28b27bb3124db3d32efd0c7219eb4a3cb

[scroggo, 2014-12-07 21:05:04]

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

[scroggo, 2014-12-07 21:05:05]

https://codereview.chromium.org/784643002/diff/80001/dm/DMSerializeTask.cpp
File dm/DMSerializeTask.cpp (left):

https://codereview.chromium.org/784643002/diff/80001/dm/DMSerializeTask.cpp#o...
dm/DMSerializeTask.cpp:24: recorded->serialize(&wStream, NULL);
Changed due to ambiguity. I could add an ifdef so we keep being explicit.

https://codereview.chromium.org/784643002/diff/80001/gm/gmmain.cpp
File gm/gmmain.cpp (left):

https://codereview.chromium.org/784643002/diff/80001/gm/gmmain.cpp#oldcode1042
gm/gmmain.cpp:1042: src.serialize(&storage, NULL);
Ditto.

[reed1, 2014-12-08 15:51:15]

https://codereview.chromium.org/784643002/diff/130001/include/core/SkPixelSer...
File include/core/SkPixelSerializer.h (right):

https://codereview.chromium.org/784643002/diff/130001/include/core/SkPixelSer...
include/core/SkPixelSerializer.h:20: public:
Can we make these methods non-virtual, and have them call virtual onVersions? I
find that helps with changing the public signatures and/or adding helper
methods.

https://codereview.chromium.org/784643002/diff/130001/include/core/SkPixelSer...
include/core/SkPixelSerializer.h:25: virtual bool useEncodedData(SkData*) = 0;
perhaps this signature should be (const void*, size_t) to make it slightly more
general.

https://codereview.chromium.org/784643002/diff/130001/include/core/SkWriteBuf...
File include/core/SkWriteBuffer.h (right):

https://codereview.chromium.org/784643002/diff/130001/include/core/SkWriteBuf...
include/core/SkWriteBuffer.h:104: * be set to NULL in release and crash in
debug.
// Calls ref on the serializer.

wonder if this guy should return its arg, they way we do in paint for refcnted
pargs?

[scroggo, 2014-12-08 18:09:31]

https://codereview.chromium.org/784643002/diff/130001/include/core/SkPixelSer...
File include/core/SkPixelSerializer.h (right):

https://codereview.chromium.org/784643002/diff/130001/include/core/SkPixelSer...
include/core/SkPixelSerializer.h:20: public:
On 2014/12/08 15:51:15, reed1 wrote:
> Can we make these methods non-virtual, and have them call virtual onVersions?
I
> find that helps with changing the public signatures and/or adding helper
> methods.

Done.

https://codereview.chromium.org/784643002/diff/130001/include/core/SkPixelSer...
include/core/SkPixelSerializer.h:25: virtual bool useEncodedData(SkData*) = 0;
On 2014/12/08 15:51:15, reed1 wrote:
> perhaps this signature should be (const void*, size_t) to make it slightly
more
> general.

Done.

https://codereview.chromium.org/784643002/diff/130001/include/core/SkWriteBuf...
File include/core/SkWriteBuffer.h (right):

https://codereview.chromium.org/784643002/diff/130001/include/core/SkWriteBuf...
include/core/SkWriteBuffer.h:104: * be set to NULL in release and crash in
debug.
On 2014/12/08 15:51:15, reed1 wrote:
> // Calls ref on the serializer.

Done.
> 
> wonder if this guy should return its arg, they way we do in paint for refcnted
> pargs?

We could do that, but I think it only makes sense if we expect this to be called
directly by clients. My understanding is that the purpose of returning the arg
is so the caller can do:

writeBuffer.setPixelSerializer(serializer)->unref()

The only current use case is in SkPictureData, which did not have a ref on it,
so it has no reason to call unref().

[reed1, 2014-12-08 21:55:20]

may have further bikesheds, but lets start to try using it and see.

lgtm

[scroggo, 2014-12-08 21:56:27]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-12-08 21:56:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/784643002/190001

[commit-bot: I haz the power, 2014-12-09 13:23:19]

Committed patchset #11 (id:190001) as
https://skia.googlesource.com/skia/+/0c4aba6edb9900c597359dfa49d3ce4a41bc5dd1

[robertphillips, 2014-12-09 13:33:55]

A revert of this CL (patchset #11 id:190001) has been created in
https://codereview.chromium.org/787833002/ by robertphillips@google.com.

The reason for reverting is: Compilation is failing on some bots.

[scroggo, 2014-12-09 14:12:22]

scroggo@google.com changed reviewers:
+ rmistry@google.com

[scroggo, 2014-12-09 14:12:22]

On 2014/12/09 13:33:55, robertphillips wrote:
> A revert of this CL (patchset #11 id:190001) has been created in
> https://codereview.chromium.org/787833002/ by
mailto:robertphillips@google.com.
> 
> The reason for reverting is: Compilation is failing on some bots.

One example failure is:
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-Cl...

Strangely, the CQ tested on Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot, and it
succeeded (see
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-Cl...)

Ravi, is there something different between the compile CQ and the compile bot?
Or am I missing something?

[rmistry, 2014-12-09 14:18:15]

On 2014/12/09 14:12:22, scroggo wrote:
> On 2014/12/09 13:33:55, robertphillips wrote:
> > A revert of this CL (patchset #11 id:190001) has been created in
> > https://codereview.chromium.org/787833002/ by
> mailto:robertphillips@google.com.
> > 
> > The reason for reverting is: Compilation is failing on some bots.
> 
> One example failure is:
>
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-Cl...
> 
> Strangely, the CQ tested on Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot, and
it
> succeeded (see
>
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-Cl...)
> 
> Ravi, is there something different between the compile CQ and the compile bot?
> Or am I missing something?

+borenet

Eric may know if there is some subtle difference between the trybot and the real
bot.

[borenet, 2014-12-09 14:31:18]

On 2014/12/09 14:18:15, rmistry wrote:
> On 2014/12/09 14:12:22, scroggo wrote:
> > On 2014/12/09 13:33:55, robertphillips wrote:
> > > A revert of this CL (patchset #11 id:190001) has been created in
> > > https://codereview.chromium.org/787833002/ by
> > mailto:robertphillips@google.com.
> > > 
> > > The reason for reverting is: Compilation is failing on some bots.
> > 
> > One example failure is:
> >
>
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-Cl...
> > 
> > Strangely, the CQ tested on Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot, and
> it
> > succeeded (see
> >
>
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-Cl...)
> > 
> > Ravi, is there something different between the compile CQ and the compile
bot?
> > Or am I missing something?
> 
> +borenet
> 
> Eric may know if there is some subtle difference between the trybot and the
real
> bot.

The only difference between the trybot and the waterfall bot is in the update
process, where the trybot has the additional steps of "gclient revert" and
"apply_issue".  Possible issues I can think of:
- Applied patches aren't actually getting reverted. I don't think this is the
case, since "gclient revert" runs "git reset --hard" and "git clean -d -f"
recursively.
- We don't do clean builds on trybots (or waterfall bots). Maybe it's possible
that the dependencies weren't tracked properly and not everything that needed to
be rebuilt was rebuilt?

[scroggo, 2014-12-09 15:34:33]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-12-09 15:35:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/784643002/210001

[commit-bot: I haz the power, 2014-12-09 15:44:41]

Committed patchset #12 (id:210001) as
https://skia.googlesource.com/skia/+/02b217f80b01a7dda8493422e5257c36a9ce8464

[scroggo, 2014-12-09 16:26:47]

A revert of this CL (patchset #12 id:210001) has been created in
https://codereview.chromium.org/783393004/ by scroggo@google.com.

The reason for reverting is: Failing serialization tasks in DM:

http://build.chromium.org/p/client.skia/builders/Test-Win8-ShuttleA-GTX660-x8....

[scroggo, 2014-12-09 18:27:18]

scroggo@google.com changed reviewers:
- rmistry@google.com

[scroggo, 2014-12-09 22:25:00]

scroggo@google.com changed reviewers:
+ mtklein@google.com

[scroggo, 2014-12-09 22:25:01]

Only failing on Win8, when running dm.

mtklein@, any idea what could be different there?

[scroggo, 2014-12-11 18:38:24]

On 2014/12/09 22:25:01, scroggo wrote:
> Only failing on Win8, when running dm.
> 
> mtklein@, any idea what could be different there?

From discussion with mtklein:
This is likely a bug we have seen spuriously on Win8, where we draw the wrong
text. A hint that this is spurious is that the test failed inconsistently. e.g.
imageblur_565_nobbh failed but imageblur_8888_nobbh did not fail. We'll try
committing again, and if it fails regularly, we'll have a dependable test case.

[scroggo, 2014-12-11 18:38:30]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-12-11 18:39:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/784643002/230001

[commit-bot: I haz the power, 2014-12-11 18:39:35]

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

[commit-bot: I haz the power, 2014-12-11 18:39:36]

Failed to apply patch for gyp/skia_for_chromium_defines.gypi:
While running git apply --index -3 -p1;
  error: patch failed: gyp/skia_for_chromium_defines.gypi:15
  Falling back to three-way merge...
  Applied patch to 'gyp/skia_for_chromium_defines.gypi' with conflicts.
  U gyp/skia_for_chromium_defines.gypi

Patch:       gyp/skia_for_chromium_defines.gypi
Index: gyp/skia_for_chromium_defines.gypi
diff --git a/gyp/skia_for_chromium_defines.gypi
b/gyp/skia_for_chromium_defines.gypi
index
aee4112c805b11e232a15ff15931576f247a9db0..2c91d2969c3a264249252af3104b16b748674d7f
100644
--- a/gyp/skia_for_chromium_defines.gypi
+++ b/gyp/skia_for_chromium_defines.gypi
@@ -15,6 +15,8 @@
     'skia_for_chromium_defines': [
       'SK_SUPPORT_LEGACY_TEXTRENDERMODE',
       'SK_IGNORE_GPU_LAYER_HOISTING',
+      # Transition for skbug.com/3190
+      'SK_LEGACY_ENCODE_BITMAP',
     ],
   },
 }

[scroggo, 2014-12-11 18:44:00]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-12-11 18:44:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/784643002/250001

[commit-bot: I haz the power, 2014-12-11 18:54:03]

Committed patchset #14 (id:250001) as
https://skia.googlesource.com/skia/+/895c43b28b27bb3124db3d32efd0c7219eb4a3cb