We don't flatten or unflatten SkPaintOptionsAndroid. Reproduce and fix.

We don't flatten or unflatten SkPaintOptionsAndroid.  Reproduce and fix.

BUG=skia:1625
Committed: http://code.google.com/p/skia/source/detail?r=11472

[mtklein, 2013-09-17 21:30:32]

Think you can help me sort out how to update picture version.  I think I can
just grab a flag bit instead to avoid having to bump the version.  What's best?

[djsollen, 2013-09-18 12:50:30]

https://codereview.chromium.org/24075010/diff/1/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/24075010/diff/1/include/core/SkPicture.h#newc...
include/core/SkPicture.h:214: // TODO: mtklein sort this out
Are there other places that we can clean up (like the hinting scaling factor) if
we bump the version?

I'm not sure how we can make this CL work without bumping the picture version as
it will break any test that uses serialized pictures on all of our Android bots.

[mtklein, 2013-09-18 16:32:08]

On 2013/09/18 12:50:30, djsollen wrote:
> https://codereview.chromium.org/24075010/diff/1/include/core/SkPicture.h
> File include/core/SkPicture.h (right):
> 
>
https://codereview.chromium.org/24075010/diff/1/include/core/SkPicture.h#newc...
> include/core/SkPicture.h:214: // TODO: mtklein sort this out
> Are there other places that we can clean up (like the hinting scaling factor)
if
> we bump the version?
> 
> I'm not sure how we can make this CL work without bumping the picture version
as
> it will break any test that uses serialized pictures on all of our Android
bots.

Here's the version I was thinking of that doesn't need a version bump.

[djsollen, 2013-09-18 17:03:50]

I like this solution, but we need to document somewhere that we should remove it
the next time we need to bump the picture version.

[reed1, 2013-09-18 18:32:18]

lgtm

[reed1, 2013-09-18 18:37:39]

I withdraw the approval for now...

I think the reader code needs to always check that bit, and either process or
skip the bytes written to the stream.

https://codereview.chromium.org/24075010/diff/6001/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (right):

https://codereview.chromium.org/24075010/diff/6001/src/core/SkPaint.cpp#newco...
src/core/SkPaint.cpp:2195: #endif
need

#else // not android, need to eat these bytes
    uint32_t bytes = buffer.read32();
    buffer. skip(bytes);
#endif

[vandebo (ex-Chrome), 2013-09-18 18:39:29]

On 2013/09/18 18:37:39, reed1 wrote:
> I withdraw the approval for now...

FYI the way to tell the CQ that, is to say it does "not LGTM."

[reed1, 2013-09-18 18:39:42]

another suggestion:

- do we often have a "default" value in the options?
- if so, perhaps the bit means "I'm on android AND my options are non-default"

[mtklein, 2013-09-19 16:44:22]

On 2013/09/18 18:39:42, reed1 wrote:
> another suggestion:
> 
> - do we often have a "default" value in the options?
> - if so, perhaps the bit means "I'm on android AND my options are non-default"

Please take another look?

Patch set 3 makes sure we always read the Android paint options when they're
written.  It seemed like the cleanest thing to do was to make non-Android builds
aware of the class (and thus how to read its serialized form), but keep it so
that SkPaint only has an SkAndroidPaintOptions member when the
SK_BUILD_FOR_ANDROID is defined.

Patch set 4 changes the meaning of the bit to what Mike suggests, so we don't
serialize default values.  I'm of two minds about it.  It does save bytes but
makes both the flatten and unflatten logic more complicated.  If the use case
here is for things already big and slow, like serializing to a file, I think
this is probably not worth the code complexity.

[mtklein, 2013-09-19 16:46:43]

On 2013/09/19 16:44:22, mtklein wrote:
> On 2013/09/18 18:39:42, reed1 wrote:
> > another suggestion:
> > 
> > - do we often have a "default" value in the options?
> > - if so, perhaps the bit means "I'm on android AND my options are
non-default"
> 
> Please take another look?
> 
> Patch set 3 makes sure we always read the Android paint options when they're
> written.  It seemed like the cleanest thing to do was to make non-Android
builds
> aware of the class (and thus how to read its serialized form), but keep it so
> that SkPaint only has an SkAndroidPaintOptions member when the
> SK_BUILD_FOR_ANDROID is defined.
> 
> Patch set 4 changes the meaning of the bit to what Mike suggests, so we don't
> serialize default values.  I'm of two minds about it.  It does save bytes but
> makes both the flatten and unflatten logic more complicated.  If the use case
> here is for things already big and slow, like serializing to a file, I think
> this is probably not worth the code complexity.

Forgot to mention: I think now either way this bit needs to stick around through
the next picture version.

[djsollen, 2013-09-19 18:36:52]

https://codereview.chromium.org/24075010/diff/19001/tests/AndroidPaintTest.cpp
File tests/AndroidPaintTest.cpp (right):

https://codereview.chromium.org/24075010/diff/19001/tests/AndroidPaintTest.cp...
tests/AndroidPaintTest.cpp:12: return !(a != b);
why not just add the operator to SkPaintOptionsAndroid? I never did like the
fact that != was supported but == was not.

[mtklein, 2013-09-19 19:04:45]

https://codereview.chromium.org/24075010/diff/19001/tests/AndroidPaintTest.cpp
File tests/AndroidPaintTest.cpp (right):

https://codereview.chromium.org/24075010/diff/19001/tests/AndroidPaintTest.cp...
tests/AndroidPaintTest.cpp:12: return !(a != b);
On 2013/09/19 18:36:52, djsollen wrote:
> why not just add the operator to SkPaintOptionsAndroid? I never did like the
> fact that != was supported but == was not.

No reason.  Done.

[reed1, 2013-09-19 19:10:20]

https://codereview.chromium.org/24075010/diff/30001/include/core/SkPaint.h
File include/core/SkPaint.h (left):

https://codereview.chromium.org/24075010/diff/30001/include/core/SkPaint.h#ol...
include/core/SkPaint.h:18: #ifdef SK_BUILD_FOR_ANDROID
Why is this removed from the public header :(

[mtklein, 2013-09-19 19:14:23]

https://codereview.chromium.org/24075010/diff/30001/include/core/SkPaint.h
File include/core/SkPaint.h (left):

https://codereview.chromium.org/24075010/diff/30001/include/core/SkPaint.h#ol...
include/core/SkPaint.h:18: #ifdef SK_BUILD_FOR_ANDROID
On 2013/09/19 19:10:20, reed1 wrote:
> Why is this removed from the public header :(

Hrm, well, you're right, the header itself logically only needs the include if
SK_BUILD_FOR_ANDROID is defined.  We can have if #ifdef'd here if you like then
unconditionally #include it in the .cpp ?  The status quo is that the .cpp was
relying on the transitive include from this .h.

[mtklein, 2013-09-19 19:28:01]

https://codereview.chromium.org/24075010/diff/30001/include/core/SkPaint.h
File include/core/SkPaint.h (left):

https://codereview.chromium.org/24075010/diff/30001/include/core/SkPaint.h#ol...
include/core/SkPaint.h:18: #ifdef SK_BUILD_FOR_ANDROID
On 2013/09/19 19:14:24, mtklein wrote:
> On 2013/09/19 19:10:20, reed1 wrote:
> > Why is this removed from the public header :(
> 
> Hrm, well, you're right, the header itself logically only needs the include if
> SK_BUILD_FOR_ANDROID is defined.  We can have if #ifdef'd here if you like
then
> unconditionally #include it in the .cpp ?  The status quo is that the .cpp was
> relying on the transitive include from this .h.

Done.

[djsollen, 2013-09-19 19:59:29]

https://codereview.chromium.org/24075010/diff/38001/include/core/SkPaint.h
File include/core/SkPaint.h (right):

https://codereview.chromium.org/24075010/diff/38001/include/core/SkPaint.h#ne...
include/core/SkPaint.h:18: #if SK_BUILD_FOR_ANDROID
why change from ifdef?

[mtklein, 2013-09-19 20:55:59]

https://codereview.chromium.org/24075010/diff/38001/include/core/SkPaint.h
File include/core/SkPaint.h (right):

https://codereview.chromium.org/24075010/diff/38001/include/core/SkPaint.h#ne...
include/core/SkPaint.h:18: #if SK_BUILD_FOR_ANDROID
On 2013/09/19 19:59:30, djsollen wrote:
> why change from ifdef?

!!! typo!  fixed.

[djsollen, 2013-09-20 12:58:23]

lgtm

https://codereview.chromium.org/24075010/diff/49001/include/core/SkPaint.h
File include/core/SkPaint.h (right):

https://codereview.chromium.org/24075010/diff/49001/include/core/SkPaint.h#ne...
include/core/SkPaint.h:20: #endif  // SK_BUILD_FOR_ANDROID
in general I wouldn't bother editing this file just to add this, but it's just a
personal opinion.

[mtklein, 2013-09-20 13:18:46]

https://codereview.chromium.org/24075010/diff/49001/include/core/SkPaint.h
File include/core/SkPaint.h (right):

https://codereview.chromium.org/24075010/diff/49001/include/core/SkPaint.h#ne...
include/core/SkPaint.h:20: #endif  // SK_BUILD_FOR_ANDROID
On 2013/09/20 12:58:23, djsollen wrote:
> in general I wouldn't bother editing this file just to add this, but it's just
a
> personal opinion.

Agreed.  I wasn't looking at the end-to-end diff here.  Reverted.

[reed1, 2013-09-24 19:54:02]

lgtm

[commit-bot: I haz the power, 2013-09-26 15:05:11]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@google.com/24075010/55001

[commit-bot: I haz the power, 2013-09-26 15:16:18]

Change committed as 11472