SkRemote: some images, many TODOs

SkRemote: some images, many TODOs


You can see from the diffs this is a step forward, but with bugs to fix:

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

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/479f54dd94a8f468fbd51763943528ec6cf3146f

[mtklein, 2015-11-02 20:28:24]

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

[commit-bot: I haz the power, 2015-11-02 20:28:26]

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

[commit-bot: I haz the power, 2015-11-02 20:29:19]

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

[commit-bot: I haz the power, 2015-11-02 20:29:19]

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

[mtklein, 2015-11-02 20:31:19]

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

[commit-bot: I haz the power, 2015-11-02 20:31:22]

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

[commit-bot: I haz the power, 2015-11-02 20:32:24]

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

[commit-bot: I haz the power, 2015-11-02 20:32:24]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)

[mtklein, 2015-11-02 20:34:35]

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

[commit-bot: I haz the power, 2015-11-02 20:34:44]

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

[commit-bot: I haz the power, 2015-11-02 20:46:04]

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

[commit-bot: I haz the power, 2015-11-02 20:46:05]

Dry run: This issue passed the CQ dry run.

[mtklein, 2015-11-02 20:56:54]

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

[commit-bot: I haz the power, 2015-11-02 20:57:02]

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

[commit-bot: I haz the power, 2015-11-02 21:07:46]

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

[commit-bot: I haz the power, 2015-11-02 21:07:46]

Dry run: This issue passed the CQ dry run.

[mtklein_C, 2015-11-02 21:12:36]

mtklein@chromium.org changed reviewers:
+ halcanary@google.com
- mtklein@google.com

[mtklein, 2015-11-02 21:15:33]

Description was changed from

==========
SkRemote: some images, many TODOs

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

to

==========
SkRemote: some images, many TODOs


You can see from the diffs this is a step forward, but with bugs to fix:

https://gold.skia.org/search2?issue=1418483006&unt=true&query=source_type%3Dg...

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

[hal.canary, 2015-11-03 15:04:01]

https://codereview.chromium.org/1418483006/diff/80001/src/core/SkRemote.cpp
File src/core/SkRemote.cpp (right):

https://codereview.chromium.org/1418483006/diff/80001/src/core/SkRemote.cpp#n...
src/core/SkRemote.cpp:335: // TODO
    SkAutoTUnref<SkImage> image(SkImage::NewFromBitmap(bitmap));
    this->onDrawImageNine(image, center, dst, paint);

https://codereview.chromium.org/1418483006/diff/80001/src/core/SkRemote.cpp#n...
src/core/SkRemote.cpp:342: // TODO
    SkNinePatchIter iter(image->width(), image->height(), center, dst);
    SkRect srcR, dstR;
    while (iter.next(&srcR, &dstR)) {
        this->onDrawImageRect(image, &srcR, dstR, paint,
                              SkCanvas::kStrict_SrcRectConstraint);
    }

https://codereview.chromium.org/1418483006/diff/80001/src/core/SkRemote.cpp#n...
src/core/SkRemote.cpp:374: this->INHERITED::onClipRect(rect, op, edgeStyle);
// Please add comment explaining why we need this.

[hal.canary, 2015-11-03 15:09:56]

Things that make me sad:

*  Too many image draw calls!

*  The default implementations don't do what you want!

https://codereview.chromium.org/1418483006/diff/80001/src/core/SkRemote.cpp
File src/core/SkRemote.cpp (right):

https://codereview.chromium.org/1418483006/diff/80001/src/core/SkRemote.cpp#n...
src/core/SkRemote.cpp:234: // TODO
Isn't 

     this->INHERITED::onDrawPicture(pic, matrix, paint);

better than dropping this call (for now)?

Or simpler:

    #include "SkCanvasPriv.h"
    SkAutoCanvasMatrixPaint acmp(this, matrix, paint, picture->cullRect());
    picture->playback(this);

[mtklein, 2015-11-03 15:24:30]

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

[mtklein, 2015-11-03 15:24:30]

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

[mtklein, 2015-11-03 15:24:31]

https://codereview.chromium.org/1418483006/diff/80001/src/core/SkRemote.cpp
File src/core/SkRemote.cpp (right):

https://codereview.chromium.org/1418483006/diff/80001/src/core/SkRemote.cpp#n...
src/core/SkRemote.cpp:234: // TODO
On 2015/11/03 at 15:09:56, Hal Canary wrote:
> Isn't 
> 
>      this->INHERITED::onDrawPicture(pic, matrix, paint);
> 
> better than dropping this call (for now)?
> 
> Or simpler:
> 
>     #include "SkCanvasPriv.h"
>     SkAutoCanvasMatrixPaint acmp(this, matrix, paint, picture->cullRect());
>     picture->playback(this);

Done.

https://codereview.chromium.org/1418483006/diff/80001/src/core/SkRemote.cpp#n...
src/core/SkRemote.cpp:335: // TODO
On 2015/11/03 at 15:04:01, Hal Canary wrote:
> SkAutoTUnref<SkImage> image(SkImage::NewFromBitmap(bitmap));
>     this->onDrawImageNine(image, center, dst, paint);

Done.

https://codereview.chromium.org/1418483006/diff/80001/src/core/SkRemote.cpp#n...
src/core/SkRemote.cpp:342: // TODO
On 2015/11/03 at 15:04:01, Hal Canary wrote:
> SkNinePatchIter iter(image->width(), image->height(), center, dst);
>     SkRect srcR, dstR;
>     while (iter.next(&srcR, &dstR)) {
>         this->onDrawImageRect(image, &srcR, dstR, paint,
>                               SkCanvas::kStrict_SrcRectConstraint);
>     }

Done.

https://codereview.chromium.org/1418483006/diff/80001/src/core/SkRemote.cpp#n...
src/core/SkRemote.cpp:374: this->INHERITED::onClipRect(rect, op, edgeStyle);
On 2015/11/03 at 15:04:01, Hal Canary wrote:
> // Please add comment explaining why we need this.

// Done.

[mtklein, 2015-11-03 15:24:31]

https://codereview.chromium.org/1418483006/diff/80001/src/core/SkRemote.cpp
File src/core/SkRemote.cpp (right):

https://codereview.chromium.org/1418483006/diff/80001/src/core/SkRemote.cpp#n...
src/core/SkRemote.cpp:335: // TODO
On 2015/11/03 at 15:04:01, Hal Canary wrote:
> SkAutoTUnref<SkImage> image(SkImage::NewFromBitmap(bitmap));
>     this->onDrawImageNine(image, center, dst, paint);

Done.

https://codereview.chromium.org/1418483006/diff/80001/src/core/SkRemote.cpp#n...
src/core/SkRemote.cpp:342: // TODO
On 2015/11/03 at 15:04:01, Hal Canary wrote:
> SkNinePatchIter iter(image->width(), image->height(), center, dst);
>     SkRect srcR, dstR;
>     while (iter.next(&srcR, &dstR)) {
>         this->onDrawImageRect(image, &srcR, dstR, paint,
>                               SkCanvas::kStrict_SrcRectConstraint);
>     }

Done.

https://codereview.chromium.org/1418483006/diff/80001/src/core/SkRemote.cpp#n...
src/core/SkRemote.cpp:374: this->INHERITED::onClipRect(rect, op, edgeStyle);
On 2015/11/03 at 15:04:01, Hal Canary wrote:
> // Please add comment explaining why we need this.

// Done.

[mtklein, 2015-11-03 15:24:37]

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

[commit-bot: I haz the power, 2015-11-03 15:24:45]

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

[hal.canary, 2015-11-03 15:25:32]

lgtm

[mtklein, 2015-11-03 15:25:52]

The CQ bit was unchecked by mtklein@google.com

[mtklein, 2015-11-03 15:25:55]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2015-11-03 15:26:02]

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

[commit-bot: I haz the power, 2015-11-03 15:35:32]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/479f54dd94a8f468fbd51763943528ec6cf3146f