Deprecate SaveFlags use in the public SkCanvas API.

Deprecate SaveFlags use in the public SkCanvas API.

Because we still have internal users for now (to support the deprecated
mode), this CL introduces an external-only variant of deprecation.

Chromium is no longer using the deprecated methods, but Android may need
to suppress SK_ATTR_EXTERNALLY_DEPRECATED warnings.

R=reed@google.com,robertphillips@google.com,scroggo@google.com

Committed: http://code.google.com/p/skia/source/detail?r=14367

[f(malita), 2014-04-23 16:46:10]

Alternative to https://codereview.chromium.org/249253003/.

[reed1, 2014-04-23 17:06:00]

lgtm

In a future CL, could we actually rename the deprecated methods, to make them
clearer to find in the future? 

e.g.
deprecated_safeWithFlags(...)

[mtklein, 2014-04-23 17:43:12]

lgtm

I guess we do this first, then wean the existing callers, finally ending up with
something like the !SK_SUPPORT_LEGACY_SAVEFLAGS state of crrev.com/249253003 ?

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

https://codereview.chromium.org/246023008/diff/1/src/core/SkCanvas.cpp#newcod...
src/core/SkCanvas.cpp:929: // Overriding classes may return false to signal that
we don't need to create a layer.
...return kNoLayer_SaveLayerStrategy...?

Or just remove the comment?

[scroggo, 2014-04-23 17:55:40]

IIRC correctly, having Android call a deprecated function will break the build.
If you add SK_INTERNAL to gyp/common_conditions.gypi, that will fix Android,
though it seems to be a bit of an abuse of 'internal'...

I'm also not sure how we'll get Android to switch, since their public API (which
they are very resistant to changing) relies on the flags we exposed.

[f(malita), 2014-04-23 18:08:24]

On 2014/04/23 17:55:40, scroggo wrote:
> IIRC correctly, having Android call a deprecated function will break the
build.
> If you add SK_INTERNAL to gyp/common_conditions.gypi, that will fix Android,
> though it seems to be a bit of an abuse of 'internal'...

The way Chromium gets around deprecation warnings is by defining
SK_ATTR_DEPRECATED=SK_NOTHING_ARG1.

Maybe that would work (ideally on a per-file basis) for Android?

Alternatively, we could also pragma-disable the warning at call sites only.


> I'm also not sure how we'll get Android to switch, since their public API
(which
> they are very resistant to changing) relies on the flags we exposed.

That's still up in the air I think.

1) deprecate these methods in the Android public API, give them some time, then
remove. Depends heavily on Android API policies.

2) emulate the deprecated behavior using shims (similarly to
SK_SUPPORT_LEGACY_CLIPTOLAYERFLAG IIUC).

I'll be looking at #2 myself (if I can get Android access). I think supporting
kClip_SaveFlag is trivial, but kMatrix_SaveFlag (which requires resetting the
clip) may turn out to be a PITA.

[scroggo, 2014-04-23 18:19:32]

On 2014/04/23 18:08:24, Florin Malita wrote:
> On 2014/04/23 17:55:40, scroggo wrote:
> > IIRC correctly, having Android call a deprecated function will break the
> build.
> > If you add SK_INTERNAL to gyp/common_conditions.gypi, that will fix Android,
> > though it seems to be a bit of an abuse of 'internal'...
> 
> The way Chromium gets around deprecation warnings is by defining
> SK_ATTR_DEPRECATED=SK_NOTHING_ARG1.
> 
> Maybe that would work (ideally on a per-file basis) for Android?
> 
> Alternatively, we could also pragma-disable the warning at call sites only.

That seems like it would work to me. Per file basis would be much better than
for all of Android, and at the call sites even better (should just be in 
Canvas.cpp).

> 
> 
> > I'm also not sure how we'll get Android to switch, since their public API
> (which
> > they are very resistant to changing) relies on the flags we exposed.
> 
> That's still up in the air I think.
> 
> 1) deprecate these methods in the Android public API, give them some time,
then
> remove. Depends heavily on Android API policies.
> 
> 2) emulate the deprecated behavior using shims (similarly to
> SK_SUPPORT_LEGACY_CLIPTOLAYERFLAG IIUC).
> 
> I'll be looking at #2 myself (if I can get Android access). I think supporting
> kClip_SaveFlag is trivial, but kMatrix_SaveFlag (which requires resetting the
> clip) may turn out to be a PITA.

What about kHasAlphaLayer_SaveFlag and kFullColorLayer_SaveFlag?

[reed1, 2014-04-23 18:23:11]

On 2014/04/23 18:19:32, scroggo wrote:
> On 2014/04/23 18:08:24, Florin Malita wrote:
> > On 2014/04/23 17:55:40, scroggo wrote:
> > > IIRC correctly, having Android call a deprecated function will break the
> > build.
> > > If you add SK_INTERNAL to gyp/common_conditions.gypi, that will fix
Android,
> > > though it seems to be a bit of an abuse of 'internal'...
> > 
> > The way Chromium gets around deprecation warnings is by defining
> > SK_ATTR_DEPRECATED=SK_NOTHING_ARG1.
> > 
> > Maybe that would work (ideally on a per-file basis) for Android?
> > 
> > Alternatively, we could also pragma-disable the warning at call sites only.
> 
> That seems like it would work to me. Per file basis would be much better than
> for all of Android, and at the call sites even better (should just be in 
> Canvas.cpp).
> 
> > 
> > 
> > > I'm also not sure how we'll get Android to switch, since their public API
> > (which
> > > they are very resistant to changing) relies on the flags we exposed.
> > 
> > That's still up in the air I think.
> > 
> > 1) deprecate these methods in the Android public API, give them some time,
> then
> > remove. Depends heavily on Android API policies.
> > 
> > 2) emulate the deprecated behavior using shims (similarly to
> > SK_SUPPORT_LEGACY_CLIPTOLAYERFLAG IIUC).
> > 
> > I'll be looking at #2 myself (if I can get Android access). I think
supporting
> > kClip_SaveFlag is trivial, but kMatrix_SaveFlag (which requires resetting
the
> > clip) may turn out to be a PITA.
> 
> What about kHasAlphaLayer_SaveFlag and kFullColorLayer_SaveFlag?

Those two are optimization hints, and should be able to always be ignored : i.e.
if the layer is always ARGB32, everything should work as promised ( I think )

[f(malita), 2014-04-24 20:39:22]

Updated to disable deprecation warnings for skia_android_framework. Leon PTAL.
Thanks!

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

https://codereview.chromium.org/246023008/diff/1/src/core/SkCanvas.cpp#newcod...
src/core/SkCanvas.cpp:929: // Overriding classes may return false to signal that
we don't need to create a layer.
On 2014/04/23 17:43:12, mtklein wrote:
> ...return kNoLayer_SaveLayerStrategy...?
> 
> Or just remove the comment?

Done.

[scroggo, 2014-04-24 20:41:25]

lgtm

[f(malita), 2014-04-24 20:45:59]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-04-24 20:46:31]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/fmalita@chromium.org/246023008/20001

[f(malita), 2014-04-24 20:46:57]

On 2014/04/23 17:06:00, reed1 wrote:
> lgtm
> 
> In a future CL, could we actually rename the deprecated methods, to make them
> clearer to find in the future? 
> 
> e.g.
> deprecated_safeWithFlags(...)

Will do, as soon as I get android-chrome-access so I can patch all clients.

[commit-bot: I haz the power, 2014-04-24 21:52:09]

Change committed as 14367