Take NativeImageSkia out behind the woodshed.

Take NativeImageSkia out behind the woodshed.

Since NativeImageSkia is now little more than a wrapper class around SkBitmap, use SkBitmap directly instead. The two convenience functions NativeImageSkia::draw() and NativeImageSkia::drawPattern() have been moved to Image.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191946

[Stephen White, 2015-03-14 20:43:24]

senorblanco@chromium.org changed reviewers:
+ fmalita@chromium.org, schenney@chromium.org

[Stephen White, 2015-03-14 20:43:41]

Stephen and/or Florin: PTAL. Thanks!

[f(malita), 2015-03-15 00:32:45]

Awesome, LGTM!

https://codereview.chromium.org/1001703003/diff/180001/Source/platform/DragIm...
File Source/platform/DragImageTest.cpp (right):

https://codereview.chromium.org/1001703003/diff/180001/Source/platform/DragIm...
Source/platform/DragImageTest.cpp:79: return SkBitmap();
Prolly not worth touching in this CL, but I don't think the zero-size special
case is needed here.

https://codereview.chromium.org/1001703003/diff/180001/Source/platform/graphi...
File Source/platform/graphics/Image.cpp (right):

https://codereview.chromium.org/1001703003/diff/180001/Source/platform/graphi...
Source/platform/graphics/Image.cpp:231: drawBitmapPattern(context, bitmap,
adjustForNegativeSize(floatSrcRect), scale, phase, op, destRect, repeatSpacing);
I think we could inline drawBitmapPattern here - I don't see any other callers.

https://codereview.chromium.org/1001703003/diff/180001/Source/platform/graphi...
File Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/1001703003/diff/180001/Source/platform/graphi...
Source/platform/graphics/Image.h:36: #include
"third_party/skia/include/core/SkBitmap.h"
Could this be a fwdecl if we move bitmapForCurrentFrame's impl to the cpp file?

[Stephen White, 2015-03-15 16:49:31]

Thanks for your review.

https://codereview.chromium.org/1001703003/diff/180001/Source/platform/DragIm...
File Source/platform/DragImageTest.cpp (right):

https://codereview.chromium.org/1001703003/diff/180001/Source/platform/DragIm...
Source/platform/DragImageTest.cpp:79: return SkBitmap();
On 2015/03/15 00:32:45, f(malita) wrote:
> Prolly not worth touching in this CL, but I don't think the zero-size special
> case is needed here.

Acknowledged.

https://codereview.chromium.org/1001703003/diff/180001/Source/platform/graphi...
File Source/platform/graphics/Image.cpp (right):

https://codereview.chromium.org/1001703003/diff/180001/Source/platform/graphi...
Source/platform/graphics/Image.cpp:231: drawBitmapPattern(context, bitmap,
adjustForNegativeSize(floatSrcRect), scale, phase, op, destRect, repeatSpacing);
On 2015/03/15 00:32:45, f(malita) wrote:
> I think we could inline drawBitmapPattern here - I don't see any other
callers.

Done. Also inlined drawBitmap() into BitmapImage, since that's its only caller.

https://codereview.chromium.org/1001703003/diff/180001/Source/platform/graphi...
File Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/1001703003/diff/180001/Source/platform/graphi...
Source/platform/graphics/Image.h:36: #include
"third_party/skia/include/core/SkBitmap.h"
On 2015/03/15 00:32:45, f(malita) wrote:
> Could this be a fwdecl if we move bitmapForCurrentFrame's impl to the cpp
file?

Sadly no, since SkBitmap is returned by value. Out-of-lined it anyway, since
inline
virtuals are usually silly.

[jbroman, 2015-03-15 19:42:16]

I like this CL, but suggest seeing what reed@ thinks.

When I was going to do something like this a few months ago, I was told that we
should be switching to SkImage instead.
https://codereview.chromium.org/685333003#msg4

[f(malita), 2015-03-16 03:05:06]

On 2015/03/15 19:42:16, jbroman wrote:
> I like this CL, but suggest seeing what reed@ thinks.
> 
> When I was going to do something like this a few months ago, I was told that
we
> should be switching to SkImage instead.
> https://codereview.chromium.org/685333003#msg4

I believe everyone's on board with the SkImage plan, but I see removing
NativeImageSkia as mostly unrelated (moar s/SkBitmap/SkImage subs
notwithstanding :)

https://codereview.chromium.org/1001703003/diff/180001/Source/platform/graphi...
File Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/1001703003/diff/180001/Source/platform/graphi...
Source/platform/graphics/Image.h:36: #include
"third_party/skia/include/core/SkBitmap.h"
On 2015/03/15 16:49:31, Stephen White wrote:
> On 2015/03/15 00:32:45, f(malita) wrote:
> > Could this be a fwdecl if we move bitmapForCurrentFrame's impl to the cpp
> file?
> 
> Sadly no, since SkBitmap is returned by value.

I think incomplete return types are allowed in declarations (ran into it just
the other day and thought it was surprising).

[reed1, 2015-03-16 14:34:30]

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

[reed1, 2015-03-16 14:34:31]

general lgtm

Moving to SkImage will be an even nicer evolution, at least when those are
read-only...

[Stephen White, 2015-03-16 14:48:05]

Note that I have another flavour of this change which is more "Skia-like", in
progress at https://codereview.chromium.org/1008243003/

It uses 

bool bitmapForCurrentFrame(SkBitmap* bitmap);

It avoids returning the SkBitmap by value, and only fills in the outparam on
success.

Let me know which version you folks prefer. I'm leaning towards the other
version.

[Stephen White, 2015-03-16 16:56:52]

schenney@: could you take a look for Source/web? Thanks!

Florin: I've uploaded the version with the Skia-style API here. Could you take
another look?

[Stephen Chennney, 2015-03-16 17:01:18]

On 2015/03/16 16:56:52, Stephen White wrote:
> schenney@: could you take a look for Source/web? Thanks!
> 
> Florin: I've uploaded the version with the Skia-style API here. Could you take
> another look?

LGTM for web. I think I have that power.

[f(malita), 2015-03-16 18:01:27]

On 2015/03/16 16:56:52, Stephen White wrote:
> Florin: I've uploaded the version with the Skia-style API here. Could you take
> another look?

Still LGTM.

[Stephen White, 2015-03-16 18:42:13]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2015-03-16 18:42:14]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1001703003/#ps240001
(title: "Switch to skia-style API (return bool instead of SkBitmap)")

[commit-bot: I haz the power, 2015-03-16 18:42:31]

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

[commit-bot: I haz the power, 2015-03-16 20:50:33]

Committed patchset #13 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191946