ViaAndroidSDK for DM

ViaAndroidSDK for DM

Make a Via for DM which transforms a set of draws to be more like what
we'd see through the Android Framework's HWUI API. Only built inside
Android's framework because we depend on HWUI classes for half of
those transformations.

Tested with --config androidsdk-8888 and --config androidsdk-hwui.

R=djsollen@google.com,mtklein@google.com,reed@google.com
BUG=skia:

Committed: https://skia.googlesource.com/skia/+/64de1e179012302d5f3b805d0736a583ad91c6a2

[tomhudson, 2015-03-03 15:05:57]

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

[tomhudson, 2015-03-03 15:05:58]

PTAL.

reed@: Android-Framework-only public API changes in effects we discussed last
week.

mtklein@: my next task is doing something similar to these two CLs for
performance, so I'll need more review cycles from you this week, and if you want
to influence architecture we should chat today.

[reed1, 2015-03-03 15:25:51]

api lgtm

[mtklein, 2015-03-03 15:55:14]

This looks good.  Beyond the unneeded picture, this is only really nitty stuff.

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp
File dm/DMSrcSinkAndroid.cpp (right):

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:40: inline void checkShader(SkPaint* paint, SkShader*
shader) {
Just FYI, this inline is redundant here.

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:40: inline void checkShader(SkPaint* paint, SkShader*
shader) {
I think this can be static void CheckShader(SkPaint* paint) ?  Passing shader
alongside makes it seems like it's another shader than paint->geShader().  If
you need to keep the second argument, let's pass it by const SkShader&, to make
it clear we're relying on the results of the null check in filter().

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:56: virtual bool filter(SkPaint* paint, Type drawType)
SK_OVERRIDE {
We've been using just SK_OVERRIDE for overrides lately.

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:67: SkShader * shader = paint->getShader();
Can't decide where to put your *s?

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:68: if (shader) {
Might want to write these checks like this, to scope the new variable even
tighter:

if (SkShader* shader = paint->getShader()) {
  this->checkShader(paint, shader);
}

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:69: checkShader(paint, shader);
this->

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:260: android::Canvas* ac =
android::Canvas::create_canvas(canvas);
SkAutoTDelete?

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:267: scProxy->drawPicture(fPic);
Let's try fSrc.draw(scProxy) and drop the picture.

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.h
File dm/DMSrcSinkAndroid.h (right):

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.h#newcode43
dm/DMSrcSinkAndroid.h:43: ViaAndroidSDK(Sink*);
Can't hurt to make this explicit.

[djsollen, 2015-03-03 16:50:53]

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.h
File dm/DMSrcSinkAndroid.h (right):

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.h#newcode51
dm/DMSrcSinkAndroid.h:51: SkAutoTDelete<SkDrawFilter> fFilter;
just put the filter on the stack.  No need to reuse the same one each time as it
is a stateless object.

[tomhudson, 2015-03-05 14:53:52]

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp
File dm/DMSrcSinkAndroid.cpp (right):

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:40: inline void checkShader(SkPaint* paint, SkShader*
shader) {
On 2015/03/03 15:55:14, mtklein wrote:
> Just FYI, this inline is redundant here.

Done.

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:40: inline void checkShader(SkPaint* paint, SkShader*
shader) {
On 2015/03/03 15:55:14, mtklein wrote:
> I think this can be static void CheckShader(SkPaint* paint) ?  Passing shader
> alongside makes it seems like it's another shader than paint->geShader().  If
> you need to keep the second argument, let's pass it by const SkShader&, to
make
> it clear we're relying on the results of the null check in filter().

Yeah, I was offended by calling SkPaint::getShader() once in each context, but
it's really not bad.

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:56: virtual bool filter(SkPaint* paint, Type drawType)
SK_OVERRIDE {
On 2015/03/03 15:55:14, mtklein wrote:
> We've been using just SK_OVERRIDE for overrides lately.

Done.

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:67: SkShader * shader = paint->getShader();
On 2015/03/03 15:55:14, mtklein wrote:
> Can't decide where to put your *s?

Done.

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:68: if (shader) {
On 2015/03/03 15:55:14, mtklein wrote:
> Might want to write these checks like this, to scope the new variable even
> tighter:
> 
> if (SkShader* shader = paint->getShader()) {
>   this->checkShader(paint, shader);
> }

Acknowledged.

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:69: checkShader(paint, shader);
On 2015/03/03 15:55:14, mtklein wrote:
> this->

Acknowledged.

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:260: android::Canvas* ac =
android::Canvas::create_canvas(canvas);
On 2015/03/03 15:55:14, mtklein wrote:
> SkAutoTDelete?

Done.

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.cpp#newc...
dm/DMSrcSinkAndroid.cpp:267: scProxy->drawPicture(fPic);
On 2015/03/03 15:55:14, mtklein wrote:
> Let's try fSrc.draw(scProxy) and drop the picture.

Done.

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.h
File dm/DMSrcSinkAndroid.h (right):

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.h#newcode43
dm/DMSrcSinkAndroid.h:43: ViaAndroidSDK(Sink*);
On 2015/03/03 15:55:14, mtklein wrote:
> Can't hurt to make this explicit.

Done.

https://codereview.chromium.org/974913002/diff/1/dm/DMSrcSinkAndroid.h#newcode51
dm/DMSrcSinkAndroid.h:51: SkAutoTDelete<SkDrawFilter> fFilter;
On 2015/03/03 16:50:52, djsollen wrote:
> just put the filter on the stack.  No need to reuse the same one each time as
it
> is a stateless object.

Done.

[djsollen, 2015-03-05 15:06:01]

looking good.  I'll let mtklein make sure I'm not missing anything structural
before this lands.

https://codereview.chromium.org/974913002/diff/40001/dm/DMSrcSinkAndroid.cpp
File dm/DMSrcSinkAndroid.cpp (right):

https://codereview.chromium.org/974913002/diff/40001/dm/DMSrcSinkAndroid.cpp#...
dm/DMSrcSinkAndroid.cpp:30: /* These functions are should only be compiled in
the Android Framework. */
These functions are only...

https://codereview.chromium.org/974913002/diff/40001/dm/DMSrcSinkAndroid.cpp#...
dm/DMSrcSinkAndroid.cpp:34: void CheckShader(SkPaint* paint) {
comment on what this function does?

https://codereview.chromium.org/974913002/diff/40001/dm/DMSrcSinkAndroid.cpp#...
dm/DMSrcSinkAndroid.cpp:74: // GlopBuilder only supports mode & matrix color
filters
Just say "Android SDK only supports..."

[mtklein, 2015-03-05 15:19:39]

lgtm

[tomhudson, 2015-03-05 15:37:14]

Derek: done

[djsollen, 2015-03-05 15:44:42]

lgtm

[tomhudson, 2015-03-05 15:49:45]

The CQ bit was checked by tomhudson@google.com

[tomhudson, 2015-03-05 15:49:46]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com,
mtklein@google.com
Link to the patchset: https://codereview.chromium.org/974913002/#ps60001 (title:
"Derek's comment changes")

[commit-bot: I haz the power, 2015-03-05 15:50:44]

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

[commit-bot: I haz the power, 2015-03-05 16:01:09]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/64de1e179012302d5f3b805d0736a583ad91c6a2