Add support for multi resolution icons

Add support for multi resolution icons

To support HiDPI we need a way to load two copies of icons, a low resolution version and a high resolution version.

To support this, this change does the following:
  - split theme_resource.grd into three files:
      - theme_resources.grd: icons that only have one resolution
      - theme_resources_standard.grd: low resolution icons
      - theme_resources_large.grd: high resolution icons
   - theme_resource.grd and theme_resources_standard.grd and compiled into chrome.pak/chrome.rc for all platforms.
   - theme_resources_large.grd is compiled into theme_resources_large.pak for platforms that want high resolution icons (currently only Mac)
   - gfx::Image now support icons with multiple resolution

Currently not all ThemeService APIs return multi-resolution images. Once this is checked in I'll work on converting them as I go.

Note, this change will have to be coordinated with the change to reorganize theme resources. I'll work with saintlou on that.

BUG=75812
TEST=Added a TIFF to theme_resources.grd. Verified that the toolbar icon had a mutliresolution image. Verified that unit tests passed.

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

[sail, 2011-04-14 18:13:58]

Once this is checked in my next step is to move various icons over to TIFF.

Ideally I'd like to do this as a build step using tiffutil -cat a.png
a_large.png -out a.tiff

At this point though I'm a bit stuck. How would i reference these TIFF files
from theme_resources.grd? I could have the tiff files be generated in
src/chrome/app/theme but I don't think people would like that.

Any suggestions would be appreciated.

[Nico, 2011-04-14 18:28:54]

+tony, who has experience with the resource bundle stuff as far as I know.

Mention in the CL description why you're using a tiff file instead of multiple
png files.

http://codereview.chromium.org/6849030/diff/1/chrome/browser/themes/browser_t...
File chrome/browser/themes/browser_theme_pack.h (right):

http://codereview.chromium.org/6849030/diff/1/chrome/browser/themes/browser_t...
chrome/browser/themes/browser_theme_pack.h:80: bool GetBitmapsNamed(int id,
std::vector<SkBitmap*>& bitmaps) const;
Introduce a new class and have this return that. e.g. MultiresolutionImage or
some such. The new class would have methods like GetBitmapForResolution() on win
and elsewhere and a ToNSImage() on mac. That way, the mac code doesn't have to
convert to SkBitmaps

[Elliot Glaysher, 2011-04-14 18:33:41]

I'm really uncomfortable with this approach (or at least this timing).

This touches a lot of cross-platform code and adds a new concept and a new file
format that will only be implemented/used on a single platform. I would feel a
LOT better if this was done after the gfx::Image-ization so this new
multi-bitmap thing could be shoved in as a mac implementation detail since I
doubt it will be used anywhere else.

http://codereview.chromium.org/6849030/diff/1/chrome/browser/themes/browser_t...
File chrome/browser/themes/browser_theme_pack.cc (right):

http://codereview.chromium.org/6849030/diff/1/chrome/browser/themes/browser_t...
chrome/browser/themes/browser_theme_pack.cc:29: const int kThemePackVersion =
15;
Are old theme packs binary compatible with this new code? If they aren't, you
need to increment this.

[Robert Sesek, 2011-04-14 18:42:44]

I really don't like the nested vector of bitmaps for all images...

Does NSImage decode multi-resolution TIFF images automatically into a
multi-reped NSImage? If so, then simply using ResourceBundle::GetImageNamed()
should do this for you. resource_bundle_mac.mm reads into NSData directly from
the PAK and creates an NSImage from that.

If that works right, then a better solution may be to plumb a gfx::Image
interface through the theming system because then we don't have to introduce the
concept of multiresolute images for other platforms.

http://codereview.chromium.org/6849030/diff/1/chrome/browser/themes/browser_t...
File chrome/browser/themes/browser_theme_pack.h (right):

http://codereview.chromium.org/6849030/diff/1/chrome/browser/themes/browser_t...
chrome/browser/themes/browser_theme_pack.h:80: bool GetBitmapsNamed(int id,
std::vector<SkBitmap*>& bitmaps) const;
On 2011/04/14 18:28:54, Nico wrote:
> Introduce a new class and have this return that. e.g. MultiresolutionImage or
> some such. The new class would have methods like GetBitmapForResolution() on
win
> and elsewhere and a ToNSImage() on mac. That way, the mac code doesn't have to
> convert to SkBitmaps

If I'm understanding correctly, this is exactly what gfx::Image is for.

http://codereview.chromium.org/6849030/diff/1/skia/ext/skia_utils_mac.h
File skia/ext/skia_utils_mac.h (right):

http://codereview.chromium.org/6849030/diff/1/skia/ext/skia_utils_mac.h#newco...
skia/ext/skia_utils_mac.h:82: NSImage* SkBitmapsToNSImage(const
std::vector<SkBitmap*>& bitmaps);
This needs a comment

http://codereview.chromium.org/6849030/diff/1/skia/ext/skia_utils_mac.mm
File skia/ext/skia_utils_mac.mm (right):

http://codereview.chromium.org/6849030/diff/1/skia/ext/skia_utils_mac.mm#newc...
skia/ext/skia_utils_mac.mm:173: if (bitmap.allocPixels() != true)
if (!bitmap.allocPixels())

http://codereview.chromium.org/6849030/diff/1/skia/ext/skia_utils_mac.mm#newc...
skia/ext/skia_utils_mac.mm:179:
CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB));
nit: indent 4

http://codereview.chromium.org/6849030/diff/1/skia/ext/skia_utils_mac.mm#newc...
skia/ext/skia_utils_mac.mm:200: DCHECK(context != NULL);
DHCECK(context)

http://codereview.chromium.org/6849030/diff/1/skia/ext/skia_utils_mac.mm#newc...
skia/ext/skia_utils_mac.mm:207: [NSGraphicsContext
graphicsContextWithGraphicsPort:context flipped:NO];
nit: indent 4

http://codereview.chromium.org/6849030/diff/1/skia/ext/skia_utils_mac.mm#newc...
skia/ext/skia_utils_mac.mm:258: SkCreateCGImageRefWithColorspace(skiaBitmap,
colorSpace);
nit: indent 4

http://codereview.chromium.org/6849030/diff/1/skia/ext/skia_utils_mac.mm#newc...
skia/ext/skia_utils_mac.mm:261: NSBitmapImageRep* bitmap = [[[NSBitmapImageRep
alloc]
I'd use scoped_nsobject<> here so you control the lifetime, rather than letting
the pool do that.

http://codereview.chromium.org/6849030/diff/1/skia/ext/skia_utils_mac.mm#newc...
skia/ext/skia_utils_mac.mm:264: [image addRepresentation:bitmap];
nit: missing a space

http://codereview.chromium.org/6849030/diff/1/ui/gfx/codec/tiff_codec_unittes...
File ui/gfx/codec/tiff_codec_unittest_mac.mm (right):

http://codereview.chromium.org/6849030/diff/1/ui/gfx/codec/tiff_codec_unittes...
ui/gfx/codec/tiff_codec_unittest_mac.mm:20: NSAutoreleasePool* pool =
[[NSAutoreleasePool alloc] init];
base::ScopedNSAutoreleasePool

[tony, 2011-04-14 18:46:45]

Here's another idea:
Make it possible in grd files to have a png resource marked as a high quality
version of another resource.  Have grd generate a mapping of low-res ids to
high-res ids.  This mapping would be empty for win/linux.  At runtime, the code
checks the map to see if there's a high res version and changes the id
appropriately.

This would involve changes to grit to understand the new xml attribute and to
generate a .h/.cc file mapping low-res to hi-res, but we already generate .h/.cc
files like this for other reasons.

[Emmanuel Saint-loubert-Bié, 2011-04-14 18:48:52]

I have no opinion on Tiff vs other format. However I believe that it is critical
to reorganize our theme icons along the lines of various class of
size/screendensity.

As a matter of fact I proposed yesterday in a CL to do something along these
lines: http://codereview.chromium.org/6851009/
to try to address some of the issues of various sizes vs platform vs various
options which have grown organically over time and I believe are hard to manage.
(Feel free to comment on my CL BTW :-) 

-- Emmanuel

[sail, 2011-04-14 19:52:48]

Thanks for the feedback everyone.

I'm going to work on implementing Tony's suggestion. The basic idea would be:
 - multiple PNG icons with multiple IDs
 - a way to map to low res icon IDs to high res icon IDs

I'm not sure what to do about the messy std::vector<SkBitmap*> issue. Wrapping
that in a class is easy enough but it would be better to extend gfx::Image to
support it instead. I'll see if I can do that as a part of this change.

[sail, 2011-04-15 00:47:47]

* switched to PNG
* get rid of std::vector<SkBitmap*> and push it into gfx:Image
* when no theme is set NSImage is used directly, we never convert from SkBitmap

[Emmanuel Saint-loubert-Bié, 2011-04-15 14:35:16]

This CL looks good for Touch. I will therefore close and not submit my
deeper reorg in http://codereview.chromium.org/6851009/

[Elliot Glaysher, 2011-04-15 18:05:49]

1) I like how your no longer passing vector<SkBitmap*> around in cross platform
code. That addresses a major concern of mine.

2) How do you tint these larger bitmaps in browser_theme_pack? If I understand
correctly, this still is going to only tint one set of the icons.

[sail, 2011-04-15 18:11:51]

On 2011/04/15 18:05:49, Elliot Glaysher wrote:
> 1) I like how your no longer passing vector<SkBitmap*> around in cross
platform
> code. That addresses a major concern of mine.

Yep. I think gfx::Image is a good place to store multiple resolutions of the
same icon.

> 2) How do you tint these larger bitmaps in browser_theme_pack? If I understand
> correctly, this still is going to only tint one set of the icons.

Right now I don't handle tinting. This means that if the user has a theme
installed they'll get the low resolution version of the image.

Once this change is checked in I'll change the browser theme code to use stop
using the depreciated BundleResource::GetBitmapNamed() function. Then supporting
theming will be easy.

[Robert Sesek, 2011-04-15 20:27:20]

http://codereview.chromium.org/6849030/diff/9001/chrome/browser/themes/theme_...
File chrome/browser/themes/theme_service_mac.mm (right):

http://codereview.chromium.org/6849030/diff/9001/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service_mac.mm:57: nsimage =
static_cast<NSImage*>(rb_.GetNativeImageNamed(id));
You can probably get away without the static cast by just de-referencing the
gfx::Image.

http://codereview.chromium.org/6849030/diff/9001/skia/ext/skia_utils_mac.h
File skia/ext/skia_utils_mac.h (right):

http://codereview.chromium.org/6849030/diff/9001/skia/ext/skia_utils_mac.h#ne...
skia/ext/skia_utils_mac.h:71: SkBitmap NSImageRepToSkBitmap(NSImageRep* image,
NSSize size, bool is_opaque);
This needs a unit test

http://codereview.chromium.org/6849030/diff/9001/skia/ext/skia_utils_mac.h#ne...
skia/ext/skia_utils_mac.h:84: NSImage* SkBitmapsToNSImage(const
std::vector<const SkBitmap*>& bitmaps);
This needs a unit test

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

http://codereview.chromium.org/6849030/diff/9001/ui/gfx/image.h#newcode75
ui/gfx/image.h:75: size_t GetNumberOfSkBitmaps();
I'm not in love with this interface. It's not really intuitive as to what
happens with the extra versions during conversion between formats.

I think I'd prefer something along the lines of:
  gfx::Image* GetVersionBestMatchingSize(gfx::Size);

But that itself can make things tricky...

http://codereview.chromium.org/6849030/diff/9001/ui/gfx/image_mac.mm
File ui/gfx/image_mac.mm (right):

http://codereview.chromium.org/6849030/diff/9001/ui/gfx/image_mac.mm#newcode15
ui/gfx/image_mac.mm:15: SkBitmap bitmap(gfx::NSImageRepToSkBitmap(imageRep,
Use a scoped_ptr<> here and .release() if you end up push_back()ing?

[sail, 2011-04-16 01:24:59]

http://codereview.chromium.org/6849030/diff/9001/chrome/browser/themes/theme_...
File chrome/browser/themes/theme_service_mac.mm (right):

http://codereview.chromium.org/6849030/diff/9001/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service_mac.mm:57: nsimage =
static_cast<NSImage*>(rb_.GetNativeImageNamed(id));
On 2011/04/15 20:27:20, rsesek wrote:
> You can probably get away without the static cast by just de-referencing the
> gfx::Image.

Done.

http://codereview.chromium.org/6849030/diff/9001/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service_mac.mm:57: nsimage =
static_cast<NSImage*>(rb_.GetNativeImageNamed(id));
On 2011/04/15 20:27:20, rsesek wrote:
> You can probably get away without the static cast by just de-referencing the
> gfx::Image.

Done.

http://codereview.chromium.org/6849030/diff/9001/skia/ext/skia_utils_mac.h
File skia/ext/skia_utils_mac.h (right):

http://codereview.chromium.org/6849030/diff/9001/skia/ext/skia_utils_mac.h#ne...
skia/ext/skia_utils_mac.h:71: SkBitmap NSImageRepToSkBitmap(NSImageRep* image,
NSSize size, bool is_opaque);
On 2011/04/15 20:27:20, rsesek wrote:
> This needs a unit test

Done.

http://codereview.chromium.org/6849030/diff/9001/skia/ext/skia_utils_mac.h#ne...
skia/ext/skia_utils_mac.h:84: NSImage* SkBitmapsToNSImage(const
std::vector<const SkBitmap*>& bitmaps);
On 2011/04/15 20:27:20, rsesek wrote:
> This needs a unit test

Done.
Ouch, thanks for asking me to do a unit test. This caught a bug where the image
size actually the maximum of all the bitmaps instead of the minimum!

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

http://codereview.chromium.org/6849030/diff/9001/ui/gfx/image.h#newcode75
ui/gfx/image.h:75: size_t GetNumberOfSkBitmaps();
On 2011/04/15 20:27:20, rsesek wrote:
> I'm not in love with this interface. It's not really intuitive as to what
> happens with the extra versions during conversion between formats.
> 
> I think I'd prefer something along the lines of:
>   gfx::Image* GetVersionBestMatchingSize(gfx::Size);
> 
> But that itself can make things tricky...

I agree the name sucks. It'll be supper easy to change in the future though. My
goal is that no one outside chrome/browser/theme and ui/base/resource will have
to use this API.

http://codereview.chromium.org/6849030/diff/9001/ui/gfx/image_mac.mm
File ui/gfx/image_mac.mm (right):

http://codereview.chromium.org/6849030/diff/9001/ui/gfx/image_mac.mm#newcode15
ui/gfx/image_mac.mm:15: SkBitmap bitmap(gfx::NSImageRepToSkBitmap(imageRep,
On 2011/04/15 20:27:20, rsesek wrote:
> Use a scoped_ptr<> here and .release() if you end up push_back()ing?

Done.

[sail, 2011-04-18 21:43:40]

rsesek: Are you ok with this change. Should I go ahead and check it in?

[Emmanuel Saint-loubert-Bié, 2011-04-18 21:48:35]

Given the nature of your change I would recommend you launch a "linux_touch"
try-bot. Also I think splitting the GRD you did makes sense and is clearer.

On Mon, Apr 18, 2011 at 2:43 PM, <sail@chromium.org> wrote:

> rsesek: Are you ok with this change. Should I go ahead and check it in?
>
>
> http://codereview.chromium.org/6849030/
>

[Robert Sesek, 2011-04-18 22:00:39]

No, sorry, I'm still not quite happy (yes, I'm a curmudgeon) -- I'll send
feedback in a few minutes. But this should also wait to land until after the
branch point.

[Robert Sesek, 2011-04-18 22:48:13]

http://codereview.chromium.org/6849030/diff/16134/skia/ext/skia_utils_mac.mm
File skia/ext/skia_utils_mac.mm (right):

http://codereview.chromium.org/6849030/diff/16134/skia/ext/skia_utils_mac.mm#...
skia/ext/skia_utils_mac.mm:201: base::mac::ScopedCFTypeRef<CGColorSpaceRef>
colorSpace(
nit: use under_scores in C++, so this should be color_space

http://codereview.chromium.org/6849030/diff/16134/skia/ext/skia_utils_mac.mm#...
skia/ext/skia_utils_mac.mm:204: NSSize minSize = NSZeroSize;
nit: min_size

http://codereview.chromium.org/6849030/diff/16134/ui/gfx/image.cc
File ui/gfx/image.cc (right):

http://codereview.chromium.org/6849030/diff/16134/ui/gfx/image.cc#newcode264
ui/gfx/image.cc:264: if (internal::NSImageToSkBitmaps(nsimage_rep->image(),
bitmaps))
I'd maybe change this to a CHECK() so it's more clear as to what happened if
this fails.

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

http://codereview.chromium.org/6849030/diff/16134/ui/gfx/image.h#newcode54
ui/gfx/image.h:54: // TODO  Add support for multiple resolutions when necessary.
TODO(sail): ...

http://codereview.chromium.org/6849030/diff/16134/ui/gfx/image.h#newcode75
ui/gfx/image.h:75: size_t GetNumberOfSkBitmaps();
Please add some comments and unit tests for these methods.

Additionally, I'd like to see tests for converting a multi-resolution
SkBitmap-backed gfx::Image to an NSImage, and vice versa. These behaviors should
be well-defined (and documented if they don't do what's expected).

[Nico, 2011-04-18 22:51:35]

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

http://codereview.chromium.org/6849030/diff/16134/ui/gfx/image.h#newcode54
ui/gfx/image.h:54: // TODO  Add support for multiple resolutions when necessary.
On 2011/04/18 22:48:13, rsesek wrote:
> TODO(sail): ...

I'd just remove this TODO. Speculative TODOs don't help.

[sail, 2011-04-19 00:21:53]

http://codereview.chromium.org/6849030/diff/16134/skia/ext/skia_utils_mac.mm
File skia/ext/skia_utils_mac.mm (right):

http://codereview.chromium.org/6849030/diff/16134/skia/ext/skia_utils_mac.mm#...
skia/ext/skia_utils_mac.mm:201: base::mac::ScopedCFTypeRef<CGColorSpaceRef>
colorSpace(
On 2011/04/18 22:48:13, rsesek wrote:
> nit: use under_scores in C++, so this should be color_space

Done.

http://codereview.chromium.org/6849030/diff/16134/skia/ext/skia_utils_mac.mm#...
skia/ext/skia_utils_mac.mm:204: NSSize minSize = NSZeroSize;
On 2011/04/18 22:48:13, rsesek wrote:
> nit: min_size

Done.

http://codereview.chromium.org/6849030/diff/16134/ui/gfx/image.cc
File ui/gfx/image.cc (right):

http://codereview.chromium.org/6849030/diff/16134/ui/gfx/image.cc#newcode264
ui/gfx/image.cc:264: if (internal::NSImageToSkBitmaps(nsimage_rep->image(),
bitmaps))
On 2011/04/18 22:48:13, rsesek wrote:
> I'd maybe change this to a CHECK() so it's more clear as to what happened if
> this fails.

Done.

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

http://codereview.chromium.org/6849030/diff/16134/ui/gfx/image.h#newcode54
ui/gfx/image.h:54: // TODO  Add support for multiple resolutions when necessary.
On 2011/04/18 22:51:35, Nico wrote:
> On 2011/04/18 22:48:13, rsesek wrote:
> > TODO(sail): ...
> 
> I'd just remove this TODO. Speculative TODOs don't help.

Done. Removed.

[Robert Sesek, 2011-04-19 02:41:41]

I'm mostly placated -- thanks :). Just a few nits.

http://codereview.chromium.org/6849030/diff/13002/ui/gfx/image_mac_unittest.mm
File ui/gfx/image_mac_unittest.mm (right):

http://codereview.chromium.org/6849030/diff/13002/ui/gfx/image_mac_unittest.m...
ui/gfx/image_mac_unittest.mm:30: CHECK([[[image representations] lastObject]
CHECK-ing is usually bad in unit tests because it crashes the suite. I know you
must return void to use the EXPECT_ macros. Can you either use the FAIL macro or
use an out-pointer and return void with EXPECT?

http://codereview.chromium.org/6849030/diff/13002/ui/gfx/image_mac_unittest.m...
ui/gfx/image_mac_unittest.mm:45: scoped_nsobject<NSImage> nsImage([[NSImage
alloc]
nit: ns_image

http://codereview.chromium.org/6849030/diff/13002/ui/gfx/image_unittest.h
File ui/gfx/image_unittest.h (right):

http://codereview.chromium.org/6849030/diff/13002/ui/gfx/image_unittest.h#new...
ui/gfx/image_unittest.h:33: inline SkBitmap* CreateBitmap(int width, int height)
{
just curious: why inline?

[sail, 2011-04-19 04:56:59]

http://codereview.chromium.org/6849030/diff/13002/ui/gfx/image_mac_unittest.mm
File ui/gfx/image_mac_unittest.mm (right):

http://codereview.chromium.org/6849030/diff/13002/ui/gfx/image_mac_unittest.m...
ui/gfx/image_mac_unittest.mm:30: CHECK([[[image representations] lastObject]
On 2011/04/19 02:41:41, rsesek wrote:
> CHECK-ing is usually bad in unit tests because it crashes the suite. I know
you
> must return void to use the EXPECT_ macros. Can you either use the FAIL macro
or
> use an out-pointer and return void with EXPECT?

Ahh, I didn't realize that a void return type was required for EXPECT_
functions. Fixed.

http://codereview.chromium.org/6849030/diff/13002/ui/gfx/image_mac_unittest.m...
ui/gfx/image_mac_unittest.mm:45: scoped_nsobject<NSImage> nsImage([[NSImage
alloc]
On 2011/04/19 02:41:41, rsesek wrote:
> nit: ns_image

Fixed

http://codereview.chromium.org/6849030/diff/13002/ui/gfx/image_unittest.h
File ui/gfx/image_unittest.h (right):

http://codereview.chromium.org/6849030/diff/13002/ui/gfx/image_unittest.h#new...
ui/gfx/image_unittest.h:33: inline SkBitmap* CreateBitmap(int width, int height)
{
On 2011/04/19 02:41:41, rsesek wrote:
> just curious: why inline?

This function is being included by image_unittest.cc as well. This caused the
linker to complain that the symbol was defined multiple times.

I wanted to use static but that threw another error about not all the functions
being used.

[Elliot Glaysher, 2011-04-19 16:22:56]

Please out of line the functions into a CC file then.
On Apr 18, 2011 9:57 PM, <sail@chromium.org> wrote:
>
>
http://codereview.chromium.org/6849030/diff/13002/ui/gfx/image_mac_unittest.mm
> File ui/gfx/image_mac_unittest.mm (right):
>
>
http://codereview.chromium.org/6849030/diff/13002/ui/gfx/image_mac_unittest.m...
> ui/gfx/image_mac_unittest.mm:30: CHECK([[[image representations]
> lastObject]
> On 2011/04/19 02:41:41, rsesek wrote:
>> CHECK-ing is usually bad in unit tests because it crashes the suite. I
> know you
>> must return void to use the EXPECT_ macros. Can you either use the
> FAIL macro or
>> use an out-pointer and return void with EXPECT?
>
> Ahh, I didn't realize that a void return type was required for EXPECT_
> functions. Fixed.
>
>
http://codereview.chromium.org/6849030/diff/13002/ui/gfx/image_mac_unittest.m...
> ui/gfx/image_mac_unittest.mm:45: scoped_nsobject<NSImage>
> nsImage([[NSImage alloc]
> On 2011/04/19 02:41:41, rsesek wrote:
>> nit: ns_image
>
> Fixed
>
> http://codereview.chromium.org/6849030/diff/13002/ui/gfx/image_unittest.h
> File ui/gfx/image_unittest.h (right):
>
>
http://codereview.chromium.org/6849030/diff/13002/ui/gfx/image_unittest.h#new...
> ui/gfx/image_unittest.h:33: inline SkBitmap* CreateBitmap(int width, int
> height) {
> On 2011/04/19 02:41:41, rsesek wrote:
>> just curious: why inline?
>
> This function is being included by image_unittest.cc as well. This
> caused the linker to complain that the symbol was defined multiple
> times.
>
> I wanted to use static but that threw another error about not all the
> functions being used.
>
> http://codereview.chromium.org/6849030/

[Robert Sesek, 2011-04-19 17:40:09]

http://codereview.chromium.org/6849030/diff/21089/ui/gfx/image_mac_unittest.mm
File ui/gfx/image_mac_unittest.mm (right):

http://codereview.chromium.org/6849030/diff/21089/ui/gfx/image_mac_unittest.m...
ui/gfx/image_mac_unittest.mm:23: void CreateBitmapImageRep(int width, int
height, NSImageRep** image_rep) {
nit: make this a member of ImageMacTest?

http://codereview.chromium.org/6849030/diff/21089/ui/gfx/image_mac_unittest.m...
ui/gfx/image_mac_unittest.mm:66: } else {
Why should this be false? Shouldn't the ordering be deterministic?

http://codereview.chromium.org/6849030/diff/21089/ui/gfx/image_mac_unittest.m...
ui/gfx/image_mac_unittest.mm:103: } else {
Same question.

[sail, 2011-04-19 20:15:13]

On 2011/04/19 16:22:56, Elliot Glaysher wrote:
> Please out of line the functions into a CC file then.
Done

[sail, 2011-04-19 20:15:18]

http://codereview.chromium.org/6849030/diff/21089/ui/gfx/image_mac_unittest.mm
File ui/gfx/image_mac_unittest.mm (right):

http://codereview.chromium.org/6849030/diff/21089/ui/gfx/image_mac_unittest.m...
ui/gfx/image_mac_unittest.mm:23: void CreateBitmapImageRep(int width, int
height, NSImageRep** image_rep) {
On 2011/04/19 17:40:09, rsesek wrote:
> nit: make this a member of ImageMacTest?

Done.

http://codereview.chromium.org/6849030/diff/21089/ui/gfx/image_mac_unittest.m...
ui/gfx/image_mac_unittest.mm:66: } else {
On 2011/04/19 17:40:09, rsesek wrote:
> Why should this be false? Shouldn't the ordering be deterministic?

It should be but I don't think that a specific order should be enforced.

[Robert Sesek, 2011-04-19 20:53:18]

http://codereview.chromium.org/6849030/diff/21089/ui/gfx/image_mac_unittest.mm
File ui/gfx/image_mac_unittest.mm (right):

http://codereview.chromium.org/6849030/diff/21089/ui/gfx/image_mac_unittest.m...
ui/gfx/image_mac_unittest.mm:66: } else {
On 2011/04/19 20:15:19, sail wrote:
> On 2011/04/19 17:40:09, rsesek wrote:
> > Why should this be false? Shouldn't the ordering be deterministic?
> 
> It should be but I don't think that a specific order should be enforced.

Really? I think that makes a difference. Otherwise callers will have to manually
check sizes each time if they want to use the GetSkBitmapAtIndex() API.

http://codereview.chromium.org/6849030/diff/20077/skia/ext/skia_utils_mac_uni...
File skia/ext/skia_utils_mac_unittest.mm (right):

http://codereview.chromium.org/6849030/diff/20077/skia/ext/skia_utils_mac_uni...
skia/ext/skia_utils_mac_unittest.mm:130: int redWidth = 10;
nit: under_scores

http://codereview.chromium.org/6849030/diff/22087/ui/gfx/image_unittest_util.cc
File ui/gfx/image_unittest_util.cc (right):

http://codereview.chromium.org/6849030/diff/22087/ui/gfx/image_unittest_util....
ui/gfx/image_unittest_util.cc:9: #include "ui/gfx/image_unittest_util.h"
I think you missed this header in your CL

[sail, 2011-04-19 21:07:07]

http://codereview.chromium.org/6849030/diff/21089/ui/gfx/image_mac_unittest.mm
File ui/gfx/image_mac_unittest.mm (right):

http://codereview.chromium.org/6849030/diff/21089/ui/gfx/image_mac_unittest.m...
ui/gfx/image_mac_unittest.mm:66: } else {
On 2011/04/19 20:53:18, rsesek wrote:
> On 2011/04/19 20:15:19, sail wrote:
> > On 2011/04/19 17:40:09, rsesek wrote:
> > > Why should this be false? Shouldn't the ordering be deterministic?
> > 
> > It should be but I don't think that a specific order should be enforced.
> 
> Really? I think that makes a difference. Otherwise callers will have to
manually
> check sizes each time if they want to use the GetSkBitmapAtIndex() API.

I don't think knowing the order helps that much. It makes the internals of the
class a lot more flexible.
It also matches NSImage, I don't think there's any guarantee about how -[NSImage
representations] are ordered.

http://codereview.chromium.org/6849030/diff/22087/ui/gfx/image_unittest_util.cc
File ui/gfx/image_unittest_util.cc (right):

http://codereview.chromium.org/6849030/diff/22087/ui/gfx/image_unittest_util....
ui/gfx/image_unittest_util.cc:9: #include "ui/gfx/image_unittest_util.h"
On 2011/04/19 20:53:18, rsesek wrote:
> I think you missed this header in your CL

Oh damn. Added, thanks.

[Robert Sesek, 2011-04-19 21:12:56]

LGTM! Thanks for sticking with it :).

http://codereview.chromium.org/6849030/diff/21089/ui/gfx/image_mac_unittest.mm
File ui/gfx/image_mac_unittest.mm (right):

http://codereview.chromium.org/6849030/diff/21089/ui/gfx/image_mac_unittest.m...
ui/gfx/image_mac_unittest.mm:66: } else {
On 2011/04/19 21:07:08, sail wrote:
> On 2011/04/19 20:53:18, rsesek wrote:
> > On 2011/04/19 20:15:19, sail wrote:
> > > On 2011/04/19 17:40:09, rsesek wrote:
> > > > Why should this be false? Shouldn't the ordering be deterministic?
> > > 
> > > It should be but I don't think that a specific order should be enforced.
> > 
> > Really? I think that makes a difference. Otherwise callers will have to
> manually
> > check sizes each time if they want to use the GetSkBitmapAtIndex() API.
> 
> I don't think knowing the order helps that much. It makes the internals of the
> class a lot more flexible.
> It also matches NSImage, I don't think there's any guarantee about how
-[NSImage
> representations] are ordered.

Ok. Compromise: Add a comment like what I suggest in the .h.

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

http://codereview.chromium.org/6849030/diff/20078/ui/gfx/image.h#newcode75
ui/gfx/image.h:75: // Gets the number of bitmaps in this image. This may cause a
conversion
Add a comment that states these methods are primarily meant to be consumed by
the theme provider, and that the internal ordering of the bitmaps is not
guaranteed.

[sail, 2011-04-19 21:27:26]

http://codereview.chromium.org/6849030/diff/21089/ui/gfx/image_mac_unittest.mm
File ui/gfx/image_mac_unittest.mm (right):

http://codereview.chromium.org/6849030/diff/21089/ui/gfx/image_mac_unittest.m...
ui/gfx/image_mac_unittest.mm:66: } else {
On 2011/04/19 21:12:56, rsesek wrote:
> On 2011/04/19 21:07:08, sail wrote:
> > On 2011/04/19 20:53:18, rsesek wrote:
> > > On 2011/04/19 20:15:19, sail wrote:
> > > > On 2011/04/19 17:40:09, rsesek wrote:
> > > > > Why should this be false? Shouldn't the ordering be deterministic?
> > > > 
> > > > It should be but I don't think that a specific order should be enforced.
> > > 
> > > Really? I think that makes a difference. Otherwise callers will have to
> > manually
> > > check sizes each time if they want to use the GetSkBitmapAtIndex() API.
> > 
> > I don't think knowing the order helps that much. It makes the internals of
the
> > class a lot more flexible.
> > It also matches NSImage, I don't think there's any guarantee about how
> -[NSImage
> > representations] are ordered.
> 
> Ok. Compromise: Add a comment like what I suggest in the .h.

Done.

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

http://codereview.chromium.org/6849030/diff/20078/ui/gfx/image.h#newcode75
ui/gfx/image.h:75: // Gets the number of bitmaps in this image. This may cause a
conversion
On 2011/04/19 21:12:56, rsesek wrote:
> Add a comment that states these methods are primarily meant to be consumed by
> the theme provider, and that the internal ordering of the bitmaps is not
> guaranteed.

Done.

[Robert Sesek, 2011-04-19 21:30:05]

LGTM