Adds an iOS implementation of gfx::Image.

Adds an iOS implementation of gfx::Image.

BUG=None
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156787

[rohitrao (ping after 24h), 2012-09-11 22:19:58]

Let's start with rsesek and see how he feels, and then we'll move on to an OWNER
in skia/.

[rohitrao (ping after 24h), 2012-09-12 21:52:08]

noyau@ pointed out this morning that the code in skia_utils_ios.mm is not
thread-safe.  I based it on the Mac implementation, which appears to also not be
thread-safe, so I'm going to leave this code as-is for now.  It's very likely
that I will revisit this later.

[Robert Sesek, 2012-09-13 16:14:24]

http://codereview.chromium.org/10928093/diff/8001/skia/ext/skia_utils_ios.h
File skia/ext/skia_utils_ios.h (right):

http://codereview.chromium.org/10928093/diff/8001/skia/ext/skia_utils_ios.h#n...
skia/ext/skia_utils_ios.h:26: CGColorSpaceRef colorSpace);
naming: color_space

http://codereview.chromium.org/10928093/diff/8001/skia/ext/skia_utils_ios.h#n...
skia/ext/skia_utils_ios.h:29: // DEPRECATED, use
SkBitmapToUIImageWithColorSpace() instead.
Why are you adding deprecated code?

http://codereview.chromium.org/10928093/diff/8001/skia/ext/skia_utils_ios.mm
File skia/ext/skia_utils_ios.mm (right):

http://codereview.chromium.org/10928093/diff/8001/skia/ext/skia_utils_ios.mm#...
skia/ext/skia_utils_ios.mm:36: CGBitmapContextCreate(data, size.width,
size.height, 8, size.width*4,
nit: continuations are indented 4 from the previous line

http://codereview.chromium.org/10928093/diff/8001/skia/ext/skia_utils_ios.mm#...
skia/ext/skia_utils_ios.mm:51: // TODO(rohitrao): We can use CG to make this
thread safe, but the mac code
There was a discussion about the threaded usage of gfx::Image on chromium-dev a
few weeks ago.

http://codereview.chromium.org/10928093/diff/8001/skia/ext/skia_utils_ios.mm#...
skia/ext/skia_utils_ios.mm:54: UIGraphicsPushContext(context);
I'm surprised you guys don't have a scoper for this.

http://codereview.chromium.org/10928093/diff/8001/skia/ext/skia_utils_ios.mm#...
skia/ext/skia_utils_ios.mm:64: CGColorSpaceRef colorSpace) {
naming: color_space and throughout

http://codereview.chromium.org/10928093/diff/8001/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

http://codereview.chromium.org/10928093/diff/8001/ui/gfx/image/image.h#newcod...
ui/gfx/image/image.h:151: operator UIImage*() const;
Remove. We're trying to kill the operators.

http://codereview.chromium.org/10928093/diff/8001/ui/gfx/image/image_ios.mm
File ui/gfx/image/image_ios.mm (right):

http://codereview.chromium.org/10928093/diff/8001/ui/gfx/image/image_ios.mm#n...
ui/gfx/image/image_ios.mm:20: if (!data || [data length] == 0)
Don't need the !data check since you can message nil.

http://codereview.chromium.org/10928093/diff/8001/ui/gfx/image/image_ios.mm#n...
ui/gfx/image/image_ios.mm:24: [data getBytes:(&png->at(0)) length:[data
length]];
nit: extra parens unnecessary

http://codereview.chromium.org/10928093/diff/8001/ui/gfx/image/image_ios.mm#n...
ui/gfx/image/image_ios.mm:72: 
nit: extra blank

http://codereview.chromium.org/10928093/diff/8001/ui/gfx/image/image_skia_uti...
File ui/gfx/image/image_skia_util_ios.h (right):

http://codereview.chromium.org/10928093/diff/8001/ui/gfx/image/image_skia_uti...
ui/gfx/image/image_skia_util_ios.h:22: // Converts to UIImage from ImageSkia.
Who owns the result?

http://codereview.chromium.org/10928093/diff/8001/ui/gfx/image/image_util_ios.mm
File ui/gfx/image/image_util_ios.mm (right):

http://codereview.chromium.org/10928093/diff/8001/ui/gfx/image/image_util_ios...
ui/gfx/image/image_util_ios.mm:14: Image* ImageFromPNGEncodedData(const unsigned
char* input, size_t input_size) {
Wait, why this and gfx::internal::CreateUIImageFromPNG?

http://codereview.chromium.org/10928093/diff/8001/ui/gfx/image/image_util_ios...
ui/gfx/image/image_util_ios.mm:24: bool PNGEncodedDataFromImage(const Image&
image,
And why this and gfx::internal::PNGFromUIImage?

http://codereview.chromium.org/10928093/diff/8001/ui/gfx/image/image_util_ios...
ui/gfx/image/image_util_ios.mm:28: if (!data || [data length] == 0)
nit: extra check, and on line 41

http://codereview.chromium.org/10928093/diff/8001/ui/gfx/image/image_util_ios...
ui/gfx/image/image_util_ios.mm:32: [data getBytes:(&dst->at(0)) length:[data
length]];
nit: extra parens, and on line 45

[rohitrao (ping after 24h), 2012-09-13 17:57:56]

https://chromiumcodereview.appspot.com/10928093/diff/8001/skia/ext/skia_utils...
File skia/ext/skia_utils_ios.h (right):

https://chromiumcodereview.appspot.com/10928093/diff/8001/skia/ext/skia_utils...
skia/ext/skia_utils_ios.h:26: CGColorSpaceRef colorSpace);
On 2012/09/13 16:14:24, rsesek wrote:
> naming: color_space

Done.

https://chromiumcodereview.appspot.com/10928093/diff/8001/skia/ext/skia_utils...
skia/ext/skia_utils_ios.h:29: // DEPRECATED, use
SkBitmapToUIImageWithColorSpace() instead.
On 2012/09/13 16:14:24, rsesek wrote:
> Why are you adding deprecated code?

Gotta be just like Mac!  (Removed.)

https://chromiumcodereview.appspot.com/10928093/diff/8001/skia/ext/skia_utils...
File skia/ext/skia_utils_ios.mm (right):

https://chromiumcodereview.appspot.com/10928093/diff/8001/skia/ext/skia_utils...
skia/ext/skia_utils_ios.mm:36: CGBitmapContextCreate(data, size.width,
size.height, 8, size.width*4,
On 2012/09/13 16:14:24, rsesek wrote:
> nit: continuations are indented 4 from the previous line

Done.

https://chromiumcodereview.appspot.com/10928093/diff/8001/skia/ext/skia_utils...
skia/ext/skia_utils_ios.mm:36: CGBitmapContextCreate(data, size.width,
size.height, 8, size.width*4,
On 2012/09/13 16:14:24, rsesek wrote:
> nit: continuations are indented 4 from the previous line

Done.

https://chromiumcodereview.appspot.com/10928093/diff/8001/skia/ext/skia_utils...
skia/ext/skia_utils_ios.mm:51: // TODO(rohitrao): We can use CG to make this
thread safe, but the mac code
On 2012/09/13 16:14:24, rsesek wrote:
> There was a discussion about the threaded usage of gfx::Image on chromium-dev
a
> few weeks ago.

There was an assertion that ImageSkia is currently not thread-safe, and soon
will be even less thread-safe, and then it devolved into a discussion about
which threads it was safe to load images on.

I will probably come back to this and replace it with CG calls, but I want to
get favicons working first so that I can visually see if I screw up flippedness
=)

(Plus the mac code is already not thread-safe.)

https://chromiumcodereview.appspot.com/10928093/diff/8001/skia/ext/skia_utils...
skia/ext/skia_utils_ios.mm:54: UIGraphicsPushContext(context);
On 2012/09/13 16:14:24, rsesek wrote:
> I'm surprised you guys don't have a scoper for this.

I didn't see one in any of the obvious places.  I've made myself a note to
upstream one and use it as needed.

https://chromiumcodereview.appspot.com/10928093/diff/8001/skia/ext/skia_utils...
skia/ext/skia_utils_ios.mm:64: CGColorSpaceRef colorSpace) {
On 2012/09/13 16:14:24, rsesek wrote:
> naming: color_space and throughout

Done.

https://chromiumcodereview.appspot.com/10928093/diff/8001/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

https://chromiumcodereview.appspot.com/10928093/diff/8001/ui/gfx/image/image....
ui/gfx/image/image.h:151: operator UIImage*() const;
On 2012/09/13 16:14:24, rsesek wrote:
> Remove. We're trying to kill the operators.

I probably need to keep this in the downstream tree until all our callers stop
using it, but that's no reason to upstream it.  Removed.

https://chromiumcodereview.appspot.com/10928093/diff/8001/ui/gfx/image/image_...
File ui/gfx/image/image_ios.mm (right):

https://chromiumcodereview.appspot.com/10928093/diff/8001/ui/gfx/image/image_...
ui/gfx/image/image_ios.mm:20: if (!data || [data length] == 0)
On 2012/09/13 16:14:24, rsesek wrote:
> Don't need the !data check since you can message nil.

Done.

https://chromiumcodereview.appspot.com/10928093/diff/8001/ui/gfx/image/image_...
ui/gfx/image/image_ios.mm:24: [data getBytes:(&png->at(0)) length:[data
length]];
On 2012/09/13 16:14:24, rsesek wrote:
> nit: extra parens unnecessary

Done.

https://chromiumcodereview.appspot.com/10928093/diff/8001/ui/gfx/image/image_...
ui/gfx/image/image_ios.mm:72: 
On 2012/09/13 16:14:24, rsesek wrote:
> nit: extra blank

Done.

https://chromiumcodereview.appspot.com/10928093/diff/8001/ui/gfx/image/image_...
File ui/gfx/image/image_skia_util_ios.h (right):

https://chromiumcodereview.appspot.com/10928093/diff/8001/ui/gfx/image/image_...
ui/gfx/image/image_skia_util_ios.h:22: // Converts to UIImage from ImageSkia.
On 2012/09/13 16:14:24, rsesek wrote:
> Who owns the result?

Per convention, returns an autoreleased UIImage.  Modified the comment to
clarify this.

https://chromiumcodereview.appspot.com/10928093/diff/8001/ui/gfx/image/image_...
File ui/gfx/image/image_util_ios.mm (right):

https://chromiumcodereview.appspot.com/10928093/diff/8001/ui/gfx/image/image_...
ui/gfx/image/image_util_ios.mm:14: Image* ImageFromPNGEncodedData(const unsigned
char* input, size_t input_size) {
On 2012/09/13 16:14:24, rsesek wrote:
> Wait, why this and gfx::internal::CreateUIImageFromPNG?

Legacy code.  These util functions were written before ImagePNG existed, and
they're still used by chrome/ code.

I think in this situation, providing iOS implementations of cross-platform
functions is important, even if they are somewhat deprecated.  The correct
solution is to remove uses of these functions from chrome/ and then get rid of
them completely.

I've switched the PNG functions to just create and return ImagePNGs.  I can't
quite drop this file in favor of image_util.mm because we don't compile the JPEG
decoder on iOS yet, so the JPEG conversion still needs to go through UIImage
functions.

https://chromiumcodereview.appspot.com/10928093/diff/8001/ui/gfx/image/image_...
ui/gfx/image/image_util_ios.mm:28: if (!data || [data length] == 0)
On 2012/09/13 16:14:24, rsesek wrote:
> nit: extra check, and on line 41

Done.

https://chromiumcodereview.appspot.com/10928093/diff/8001/ui/gfx/image/image_...
ui/gfx/image/image_util_ios.mm:32: [data getBytes:(&dst->at(0)) length:[data
length]];
On 2012/09/13 16:14:24, rsesek wrote:
> nit: extra parens, and on line 45

Done.

[Robert Sesek, 2012-09-13 20:37:48]

https://chromiumcodereview.appspot.com/10928093/diff/1018/ui/gfx/image/image_...
File ui/gfx/image/image_util_ios.mm (right):

https://chromiumcodereview.appspot.com/10928093/diff/1018/ui/gfx/image/image_...
ui/gfx/image/image_util_ios.mm:14: Image* ImageFromPNGEncodedData(const unsigned
char* input, size_t input_size) {
So now how is this different from the non-iOS impl? Same with
PNGEncodedDataFromImage.

https://chromiumcodereview.appspot.com/10928093/diff/1018/ui/gfx/image/image_...
ui/gfx/image/image_util_ios.mm:18: 
nit: extra blank

[rohitrao (ping after 24h), 2012-09-13 20:53:53]

Per IRC conversation, using the PNG function definitions in image_util.cc, but
falling back to image_util_ios.mm for the JPEG functions.

[Robert Sesek, 2012-09-13 21:06:11]

LGTM

https://chromiumcodereview.appspot.com/10928093/diff/8006/skia/ext/skia_utils...
File skia/ext/skia_utils_ios.h (right):

https://chromiumcodereview.appspot.com/10928093/diff/8006/skia/ext/skia_utils...
skia/ext/skia_utils_ios.h:25: SK_API UIImage*
SkBitmapToUIImageWithColorSpace(const SkBitmap& icon,
naming: skia_bitmap like the .mm

https://chromiumcodereview.appspot.com/10928093/diff/8006/skia/ext/skia_utils...
File skia/ext/skia_utils_ios.mm (right):

https://chromiumcodereview.appspot.com/10928093/diff/8006/skia/ext/skia_utils...
skia/ext/skia_utils_ios.mm:72: // Now convert to NSImage.
nit: UIImage

[rohitrao (ping after 24h), 2012-09-14 00:13:27]

+reed for OWNERS approval in skia/.

+thakis for OWNERS approval for ui/base/layout.cc and ui/*.gyp.

[Nico, 2012-09-14 00:52:19]

https://codereview.chromium.org/10928093/diff/19002/ui/base/layout.cc
File ui/base/layout.cc (right):

https://codereview.chromium.org/10928093/diff/19002/ui/base/layout.cc#newcode73
ui/base/layout.cc:73: #if defined(OS_MACOSX) && !defined(OS_IOS) &&
defined(ENABLE_HIDPI)
How does HiDPI code work on iOS? Seems like you'd always wanted to add 200P on
iOS?

https://codereview.chromium.org/10928093/diff/19002/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

https://codereview.chromium.org/10928093/diff/19002/ui/ui_unittests.gypi#newc...
ui/ui_unittests.gypi:138: '../base/test/run_all_unittests.cc',
Why this change?

[Nico, 2012-09-14 00:53:54]

https://codereview.chromium.org/10928093/diff/19002/skia/skia.gyp
File skia/skia.gyp (right):

https://codereview.chromium.org/10928093/diff/19002/skia/skia.gyp#newcode453
skia/skia.gyp:453: 'ext/skia_utils_ios.h',
Can't you add that to the global source list? The filename rules should exclude
them on non-ios

[rohitrao (ping after 24h), 2012-09-14 01:07:18]

http://codereview.chromium.org/10928093/diff/19002/skia/skia.gyp
File skia/skia.gyp (right):

http://codereview.chromium.org/10928093/diff/19002/skia/skia.gyp#newcode453
skia/skia.gyp:453: 'ext/skia_utils_ios.h',
On 2012/09/14 00:53:54, Nico wrote:
> Can't you add that to the global source list? The filename rules should
exclude
> them on non-ios

The mac trybots failed trying to compile these files, so I moved them down into
an iOS section.  I'm still trying to work out what's happening.  This gyp file
pulls in common.gypi, right?

http://codereview.chromium.org/10928093/diff/19002/ui/base/layout.cc
File ui/base/layout.cc (right):

http://codereview.chromium.org/10928093/diff/19002/ui/base/layout.cc#newcode73
ui/base/layout.cc:73: #if defined(OS_MACOSX) && !defined(OS_IOS) &&
defined(ENABLE_HIDPI)
On 2012/09/14 00:52:19, Nico wrote:
> How does HiDPI code work on iOS? Seems like you'd always wanted to add 200P on
> iOS?

This bit is TBD; I still need to work out exactly which scale factors I want. 
Was planning to do this in a future CL.

(The scale factor on iOS will never change at runtime, so what I really want is
only 100P on some devices, and only 200P on others.  Will anything break if I
don't add 100P as a supported scale factor?)

http://codereview.chromium.org/10928093/diff/19002/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

http://codereview.chromium.org/10928093/diff/19002/ui/ui_unittests.gypi#newco...
ui/ui_unittests.gypi:138: '../base/test/run_all_unittests.cc',
On 2012/09/14 00:52:19, Nico wrote:
> Why this change?

The real solution is to get ui/test/test_suite.cc and
ui/test/run_all_unittests.cc compiling, but they hit a pak-related DCHECK. 
Using the base version of run_all_unittests is a stopgap measure to get some
important test setup bits (like icu) working enough to let tests pass.

Added a TODO.

[Nico, 2012-09-14 01:13:37]

lgtm with the stuff below.

(I think I'm in skia/OWNERS too)

http://codereview.chromium.org/10928093/diff/19002/skia/skia.gyp
File skia/skia.gyp (right):

http://codereview.chromium.org/10928093/diff/19002/skia/skia.gyp#newcode453
skia/skia.gyp:453: 'ext/skia_utils_ios.h',
On 2012/09/14 01:07:18, rohitrao wrote:
> On 2012/09/14 00:53:54, Nico wrote:
> > Can't you add that to the global source list? The filename rules should
> exclude
> > them on non-ios
> 
> The mac trybots failed trying to compile these files, so I moved them down
into
> an iOS section.  I'm still trying to work out what's happening.  This gyp file
> pulls in common.gypi, right?

Hm, looks like it doesn't set chromium_code at the top, so it doesn't pull in
common.gypi. Not sure why.

It looks like the other platforms include their own file pattern rules further
down. Do the same for ios for now; I'll look into converting this to common.gypi
later.

http://codereview.chromium.org/10928093/diff/19002/ui/base/layout.cc
File ui/base/layout.cc (right):

http://codereview.chromium.org/10928093/diff/19002/ui/base/layout.cc#newcode73
ui/base/layout.cc:73: #if defined(OS_MACOSX) && !defined(OS_IOS) &&
defined(ENABLE_HIDPI)
On 2012/09/14 01:07:18, rohitrao wrote:
> On 2012/09/14 00:52:19, Nico wrote:
> > How does HiDPI code work on iOS? Seems like you'd always wanted to add 200P
on
> > iOS?
> 
> This bit is TBD; I still need to work out exactly which scale factors I want. 
> Was planning to do this in a future CL.

SGTM. Can you add a "TODO: Figure out ios" comment?

> > (The scale factor on iOS will never change at runtime, so what I really want
is
> only 100P on some devices, and only 200P on others.  Will anything break if I
> don't add 100P as a supported scale factor?)

I don't know. But it can probably made work if it doesn't.

[rohitrao (ping after 24h), 2012-09-14 01:30:29]

-reed he's off the hook for the skia code.

Thanks for the review!  I'm going to double-check the exclusion rules in a mac
checkout and then I'll CQ.

http://codereview.chromium.org/10928093/diff/19002/skia/skia.gyp
File skia/skia.gyp (right):

http://codereview.chromium.org/10928093/diff/19002/skia/skia.gyp#newcode453
skia/skia.gyp:453: 'ext/skia_utils_ios.h',
> Hm, looks like it doesn't set chromium_code at the top, so it doesn't pull in
> common.gypi. Not sure why.

Aha, I didn't know the file pattern rules were gated on chromium_code=1.  This
all makes sense now.  I've added an ios exclusion rule next to the other
platforms.

http://codereview.chromium.org/10928093/diff/19002/ui/base/layout.cc
File ui/base/layout.cc (right):

http://codereview.chromium.org/10928093/diff/19002/ui/base/layout.cc#newcode73
ui/base/layout.cc:73: #if defined(OS_MACOSX) && !defined(OS_IOS) &&
defined(ENABLE_HIDPI)
> SGTM. Can you add a "TODO: Figure out ios" comment?

Added a TODO.  I'll be looking at this either tomorrow or next week, so I'll let
you know if I run into trouble.

[commit-bot: I haz the power, 2012-09-14 01:50:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rohitrao@chromium.org/10928093/9011

[commit-bot: I haz the power, 2012-09-14 05:31:56]

Retried try job too often for step(s) sync_integration_tests

[stuartmorgan, 2012-09-14 11:01:37]

https://chromiumcodereview.appspot.com/10928093/diff/8001/skia/ext/skia_utils...
File skia/ext/skia_utils_ios.mm (right):

https://chromiumcodereview.appspot.com/10928093/diff/8001/skia/ext/skia_utils...
skia/ext/skia_utils_ios.mm:54: UIGraphicsPushContext(context);
On 2012/09/13 17:57:57, rohitrao wrote:
> On 2012/09/13 16:14:24, rsesek wrote:
> > I'm surprised you guys don't have a scoper for this.
> 
> I didn't see one in any of the obvious places.

Did you check in ui/gfx/scoped_ui_graphics_push_context_ios.h? ;)

[commit-bot: I haz the power, 2012-09-14 11:06:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rohitrao@chromium.org/10928093/9011

[commit-bot: I haz the power, 2012-09-14 12:55:27]

Change committed as 156787