Add Sky framework support for drawing images

Add Sky framework support for drawing images

R=abarth@chromium.org, abarth

Committed: https://chromium.googlesource.com/external/mojo/+/bc772d9a7df7e05baac80daa89682c42d2a86af4

[eseidel, 2015-05-29 22:13:49]

eseidel@chromium.org changed reviewers:
+ eseidel@chromium.org

[eseidel, 2015-05-29 22:13:51]

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
File sky/engine/core/loader/NewImageLoader.cpp (right):

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
sky/engine/core/loader/NewImageLoader.cpp:14: 
Did you mean to leave spaces here?

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
sky/engine/core/loader/NewImageLoader.cpp:24: m_fetcher = adoptPtr(new
MojoFetcher(this, src));
So we load one img at a time it seems?  We presumably have one loader per img.

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
sky/engine/core/loader/NewImageLoader.cpp:47:
ImageDecoder::create(*buffer.get(),
You must be using tabs.  This indent seems confused.  Are you sure you need
*.get()?

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
sky/engine/core/loader/NewImageLoader.cpp:50: bool allDataReceived = TRUE;
true?

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
sky/engine/core/loader/NewImageLoader.cpp:55: ImageFrame *frame =
decoder->frameBufferAtIndex(0);
ImageFrame*, no space.

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
File sky/engine/core/loader/NewImageLoader.h (right):

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
sky/engine/core/loader/NewImageLoader.h:34: void load(const KURL& src);
Load(const GURL& src)?  New code should be chromium style and probably wants to
use gurl.

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
sky/engine/core/loader/NewImageLoader.h:44: NewImageLoaderClient* m_client;
Strange indent change here.  Also not sure if you wanted a newline between all
these?

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
sky/engine/core/loader/NewImageLoader.h:46: OwnPtr<MojoFetcher> m_fetcher;
Chromium style is fetcher_;

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/painting/Ca...
File sky/engine/core/painting/CanvasImage.cpp (right):

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/painting/Ca...
sky/engine/core/painting/CanvasImage.cpp:34: {
{ on the same line as the if in both Blink and Chromium styles.  'git cl format'
would probably help you.

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/painting/Ca...
File sky/engine/core/painting/CanvasImage.h (right):

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/painting/Ca...
sky/engine/core/painting/CanvasImage.h:33: void setBitmap(const SkBitmap
&bitmap) { m_bitmap = bitmap; }
& attaches to the type in both Blink and Chromium styles.

https://codereview.chromium.org/1151753009/diff/1/sky/engine/platform/graphic...
File sky/engine/platform/graphics/Image.h (right):

https://codereview.chromium.org/1151753009/diff/1/sky/engine/platform/graphic...
sky/engine/platform/graphics/Image.h:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
Don't change existing copyrights plz. :)

[eseidel, 2015-05-29 22:14:28]

One complication with git cl format, is that the idl generator assumes Blink
style, so idl-interfacing files will still need to use blinkCaps instead of
ChromiumCaps.

[jackson, 2015-05-29 22:54:33]

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
File sky/engine/core/loader/NewImageLoader.cpp (right):

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
sky/engine/core/loader/NewImageLoader.cpp:24: m_fetcher = adoptPtr(new
MojoFetcher(this, src));
On 2015/05/29 22:13:50, eseidel wrote:
> So we load one img at a time it seems?  We presumably have one loader per img.

Yes, currently we do

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
sky/engine/core/loader/NewImageLoader.cpp:47:
ImageDecoder::create(*buffer.get(),
On 2015/05/29 22:13:50, eseidel wrote:
> You must be using tabs.  This indent seems confused.  Are you sure you need
> *.get()?

Yes, *.get() is to turn the smart pointer into an actual SharedBuffer instance,
that ImageDecoder::create needs. But the line above can be removed, I'll fix.

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
File sky/engine/core/loader/NewImageLoader.h (right):

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
sky/engine/core/loader/NewImageLoader.h:34: void load(const KURL& src);
On 2015/05/29 22:13:51, eseidel wrote:
> Load(const GURL& src)?  New code should be chromium style and probably wants
to
> use gurl.

Load yes, will punt the GURL stuff for now since MojoFetcher wants a KURL

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
sky/engine/core/loader/NewImageLoader.h:44: NewImageLoaderClient* m_client;
On 2015/05/29 22:13:50, eseidel wrote:
> Strange indent change here.  Also not sure if you wanted a newline between all
> these?

Acknowledged.

https://codereview.chromium.org/1151753009/diff/1/sky/engine/core/loader/NewI...
sky/engine/core/loader/NewImageLoader.h:46: OwnPtr<MojoFetcher> m_fetcher;
On 2015/05/29 22:13:51, eseidel wrote:
> Chromium style is fetcher_;

Acknowledged.

https://codereview.chromium.org/1151753009/diff/1/sky/engine/platform/graphic...
File sky/engine/platform/graphics/Image.h (right):

https://codereview.chromium.org/1151753009/diff/1/sky/engine/platform/graphic...
sky/engine/platform/graphics/Image.h:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
On 2015/05/29 22:13:51, eseidel wrote:
> Don't change existing copyrights plz. :)

Good catch, I meant to put this in a different file

[abarth-chromium, 2015-05-29 23:04:11]

lgtm

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
File sky/engine/core/loader/NewImageLoader.cpp (right):

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.cpp:13: fetcher_ = adoptPtr(new
MojoFetcher(this, src));
What happens if someone calls |Load| a second time?  We seem to null out client_
after the load is complete, which means we'll crash on a null pointer...  Maybe
we shouldn't null out the client pointer or maybe we should take the URL as a
constructor parameter.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.cpp:39: ImageFrame* frame =
decoder->frameBufferAtIndex(0);
Is it possible for an image to have zero frames?  Should we check the frame
count before grabbing frame 0?

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.cpp:45: }
}  // namespace blink

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
File sky/engine/core/loader/NewImageLoader.h (right):

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.h:20: virtual void
notifyLoadFinished(const SkBitmap& result) = 0;
I wonder if we should use the "On" pattern of naming these callbacks.  It's not
a big deal either way.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.h:31: }
Is there a reason to have a |create| function?  Why not just make the
constructor public and let the caller decide whether they want to hold the
object by value or by pointer?

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.h:32: virtual ~NewImageLoader() {}
Implementations of virtual functions should be out-of-line.  (The compiler can't
do anything useful with them in the header because they can't be inlined
anyway.)

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.h:38: void OnDataComplete();
Please put the |override| keyword on these virtual function overrides.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.h:40: protected:
Remove?

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.h:49: }
}  // namespace blink

Also, once you make that change, |git cl format| will want a blank line before
this line.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
File sky/engine/core/painting/CanvasImage.cpp (right):

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
sky/engine/core/painting/CanvasImage.cpp:19: KURL newSrcURL = KURL(KURL(), url);
Please add a TODO comment about using the proper base URL here.  If don't have
base URLs quite figured out yet, so it's not something we need to fix right now.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
sky/engine/core/painting/CanvasImage.cpp:27: bitmap_ = result;
We'll eventually need a notification pathway for when the image load is complete
so that we know to repaint, etc.  Maybe add a TODO comment for now?

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
sky/engine/core/painting/CanvasImage.cpp:29: }
}  // namespace blink

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
File sky/engine/core/painting/CanvasImage.h (right):

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
sky/engine/core/painting/CanvasImage.h:21: 
We usually omit this blank line.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
sky/engine/core/painting/CanvasImage.h:23: ~CanvasImage() override {}
Please move implementations of virtual function out of line.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
sky/engine/core/painting/CanvasImage.h:32: SkBitmap bitmap() const { return
bitmap_; }
const SkBitmap&

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
sky/engine/core/painting/CanvasImage.h:36: CanvasImage() :
imageLoader_(NewImageLoader::create(this)) {}
I'd move this out-of-line as well.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
sky/engine/core/painting/CanvasImage.h:39: void notifyLoadFinished(const
SkBitmap& result);
override.

[abarth-chromium, 2015-05-29 23:07:29]

The reason we continue to use KURL in this code is because it integrates with
WTF::String.  In particular, KURL can share its underlying string with
WTF::String whereas GURL always makes a copy.  Similarly, WTF::String can share
data with DartVM, which means KURL can end up sharing with DartVM too.  GURL is
designed for a std::string world, which can't share with anyone.

[jackson, 2015-05-29 23:35:44]

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
File sky/engine/core/loader/NewImageLoader.cpp (right):

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.cpp:13: fetcher_ = adoptPtr(new
MojoFetcher(this, src));
On 2015/05/29 23:04:10, abarth wrote:
> What happens if someone calls |Load| a second time?  We seem to null out
client_
> after the load is complete, which means we'll crash on a null pointer... 
Maybe
> we shouldn't null out the client pointer or maybe we should take the URL as a
> constructor parameter.

OK, I changed it to not null the client

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.cpp:39: ImageFrame* frame =
decoder->frameBufferAtIndex(0);
On 2015/05/29 23:04:10, abarth wrote:
> Is it possible for an image to have zero frames?  Should we check the frame
> count before grabbing frame 0?

Looks like maybe. I'll add a guard

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.cpp:45: }
On 2015/05/29 23:04:10, abarth wrote:
> }  // namespace blink

Acknowledged.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
File sky/engine/core/loader/NewImageLoader.h (right):

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.h:20: virtual void
notifyLoadFinished(const SkBitmap& result) = 0;
On 2015/05/29 23:04:10, abarth wrote:
> I wonder if we should use the "On" pattern of naming these callbacks.  It's
not
> a big deal either way.

Acknowledged.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.h:31: }
On 2015/05/29 23:04:10, abarth wrote:
> Is there a reason to have a |create| function?  Why not just make the
> constructor public and let the caller decide whether they want to hold the
> object by value or by pointer?

Acknowledged.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.h:32: virtual ~NewImageLoader() {}
On 2015/05/29 23:04:10, abarth wrote:
> Implementations of virtual functions should be out-of-line.  (The compiler
can't
> do anything useful with them in the header because they can't be inlined
> anyway.)

Acknowledged.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.h:38: void OnDataComplete();
On 2015/05/29 23:04:10, abarth wrote:
> Please put the |override| keyword on these virtual function overrides.

Acknowledged.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.h:40: protected:
On 2015/05/29 23:04:10, abarth wrote:
> Remove?

Acknowledged.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/loader/...
sky/engine/core/loader/NewImageLoader.h:49: }
On 2015/05/29 23:04:10, abarth wrote:
> }  // namespace blink
> 
> Also, once you make that change, |git cl format| will want a blank line before
> this line.

Acknowledged.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
File sky/engine/core/painting/CanvasImage.cpp (right):

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
sky/engine/core/painting/CanvasImage.cpp:19: KURL newSrcURL = KURL(KURL(), url);
On 2015/05/29 23:04:11, abarth wrote:
> Please add a TODO comment about using the proper base URL here.  If don't have
> base URLs quite figured out yet, so it's not something we need to fix right
now.

Acknowledged.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
sky/engine/core/painting/CanvasImage.cpp:27: bitmap_ = result;
On 2015/05/29 23:04:11, abarth wrote:
> We'll eventually need a notification pathway for when the image load is
complete
> so that we know to repaint, etc.  Maybe add a TODO comment for now?

Acknowledged.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
File sky/engine/core/painting/CanvasImage.h (right):

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
sky/engine/core/painting/CanvasImage.h:21: 
On 2015/05/29 23:04:11, abarth wrote:
> We usually omit this blank line.

Acknowledged.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
sky/engine/core/painting/CanvasImage.h:23: ~CanvasImage() override {}
On 2015/05/29 23:04:11, abarth wrote:
> Please move implementations of virtual function out of line.

Acknowledged.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
sky/engine/core/painting/CanvasImage.h:32: SkBitmap bitmap() const { return
bitmap_; }
On 2015/05/29 23:04:11, abarth wrote:
> const SkBitmap&

Acknowledged.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
sky/engine/core/painting/CanvasImage.h:36: CanvasImage() :
imageLoader_(NewImageLoader::create(this)) {}
On 2015/05/29 23:04:11, abarth wrote:
> I'd move this out-of-line as well.

Acknowledged.

https://codereview.chromium.org/1151753009/diff/60001/sky/engine/core/paintin...
sky/engine/core/painting/CanvasImage.h:39: void notifyLoadFinished(const
SkBitmap& result);
On 2015/05/29 23:04:11, abarth wrote:
> override.

Acknowledged.

[jackson, 2015-05-29 23:45:47]

Committed patchset #8 (id:130001) manually as
bc772d9a7df7e05baac80daa89682c42d2a86af4 (presubmit successful).