Add support for multiple icon sizes for Mac platform apps

Add support for multiple icon sizes for Mac platform apps

Most of this CL is updating ImageLoadingTracker::Observer subclasses to use gfx::Image instead of SkBitmap.

With gfx::Image we can have multiple sizes for the same image. This is used to publish mutiple icons for a single Mac platform app.

BUG=112651
TEST=

[sail, 2012-02-22 08:03:06]

Most of this CL is just refactoring. The important bits are:
  image_loading_tracker
  app_shortcut_manager
  web_app_mac

Still working on getting gtk/views/unit_tests to compile. Sending it out early
to get feedback.

[Mihai Parparita -not on Chrome, 2012-02-23 00:19:41]

http://codereview.chromium.org/9428025/diff/1/chrome/browser/extensions/app_s...
File chrome/browser/extensions/app_shortcut_manager.cc (right):

http://codereview.chromium.org/9428025/diff/1/chrome/browser/extensions/app_s...
chrome/browser/extensions/app_shortcut_manager.cc:49: kDesiredSizes[0],
kDesiredSizes[0]);
This ends up using the 16x16 size on the Mac, defaulting to the largest possible
icon seems better.

http://codereview.chromium.org/9428025/diff/1/chrome/browser/extensions/image...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/9428025/diff/1/chrome/browser/extensions/image...
chrome/browser/extensions/image_loading_tracker.cc:228: bitmaps.push_back(new
SkBitmap(*it));
Why make a new SkBitmap when you could just use a pointer to the instance you
already have via the iterator?

http://codereview.chromium.org/9428025/diff/1/chrome/browser/extensions/image...
chrome/browser/extensions/image_loading_tracker.cc:247: // Remove reference to
this extension from all pending load entries. This
Why not remove the map entry? The check at line 204 will fail, and any loads
that complete will be ignored.

http://codereview.chromium.org/9428025/diff/1/chrome/browser/extensions/image...
File chrome/browser/extensions/image_loading_tracker.h (right):

http://codereview.chromium.org/9428025/diff/1/chrome/browser/extensions/image...
chrome/browser/extensions/image_loading_tracker.h:82: // Same as LoadImage()
above except it loads multiple images. This is used
"loads multiple images from the same extension" seems more accurate.

http://codereview.chromium.org/9428025/diff/1/chrome/browser/ui/cocoa/infobar...
File chrome/browser/ui/cocoa/infobars/extension_infobar_controller.mm (right):

http://codereview.chromium.org/9428025/diff/1/chrome/browser/ui/cocoa/infobar...
chrome/browser/ui/cocoa/infobars/extension_infobar_controller.mm:75: if
(!icon_resource.relative_path().empty()) {
Seems like you could remove this check, since ImageLoadingTracker::LoadImage
does it already.

http://codereview.chromium.org/9428025/diff/1/chrome/browser/ui/gtk/browser_a...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/9428025/diff/1/chrome/browser/ui/gtk/browser_a...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:168: default_skbitmap_ =
image->ToSkBitmap();
Seems like this should be *image->ToSkBitmap()

[sail, 2012-02-23 00:22:59]

+Finnur

[sail, 2012-02-23 02:54:27]

http://codereview.chromium.org/9428025/diff/1/chrome/browser/extensions/app_s...
File chrome/browser/extensions/app_shortcut_manager.cc (right):

http://codereview.chromium.org/9428025/diff/1/chrome/browser/extensions/app_s...
chrome/browser/extensions/app_shortcut_manager.cc:49: kDesiredSizes[0],
kDesiredSizes[0]);
On 2012/02/23 00:19:41, Mihai Parparita wrote:
> This ends up using the 16x16 size on the Mac, defaulting to the largest
possible
> icon seems better.

Done.

http://codereview.chromium.org/9428025/diff/1/chrome/browser/extensions/image...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/9428025/diff/1/chrome/browser/extensions/image...
chrome/browser/extensions/image_loading_tracker.cc:228: bitmaps.push_back(new
SkBitmap(*it));
On 2012/02/23 00:19:41, Mihai Parparita wrote:
> Why make a new SkBitmap when you could just use a pointer to the instance you
> already have via the iterator?

The gfx::Image constructor takes ownership of the bitmaps. Added comment.

http://codereview.chromium.org/9428025/diff/1/chrome/browser/extensions/image...
chrome/browser/extensions/image_loading_tracker.cc:247: // Remove reference to
this extension from all pending load entries. This
On 2012/02/23 00:19:41, Mihai Parparita wrote:
> Why not remove the map entry? The check at line 204 will fail, and any loads
> that complete will be ignored.

This doesn't match the old behavior. In the old code any loads that complete
still fire the OnImageLoaded() callback.

http://codereview.chromium.org/9428025/diff/1/chrome/browser/extensions/image...
File chrome/browser/extensions/image_loading_tracker.h (right):

http://codereview.chromium.org/9428025/diff/1/chrome/browser/extensions/image...
chrome/browser/extensions/image_loading_tracker.h:82: // Same as LoadImage()
above except it loads multiple images. This is used
On 2012/02/23 00:19:41, Mihai Parparita wrote:
> "loads multiple images from the same extension" seems more accurate.

Done.

http://codereview.chromium.org/9428025/diff/1/chrome/browser/ui/cocoa/infobar...
File chrome/browser/ui/cocoa/infobars/extension_infobar_controller.mm (right):

http://codereview.chromium.org/9428025/diff/1/chrome/browser/ui/cocoa/infobar...
chrome/browser/ui/cocoa/infobars/extension_infobar_controller.mm:75: if
(!icon_resource.relative_path().empty()) {
On 2012/02/23 00:19:41, Mihai Parparita wrote:
> Seems like you could remove this check, since ImageLoadingTracker::LoadImage
> does it already.

Done.

http://codereview.chromium.org/9428025/diff/1/chrome/browser/ui/gtk/browser_a...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/9428025/diff/1/chrome/browser/ui/gtk/browser_a...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:168: default_skbitmap_ =
image->ToSkBitmap();
On 2012/02/23 00:19:41, Mihai Parparita wrote:
> Seems like this should be *image->ToSkBitmap()

Done.

[Finnur, 2012-02-23 12:05:41]

I reviewed the changes to the ImageLoadingTracker and reviewed loosely how they
affect the consumer of the ImageLoadingTracker.

What was the impetus for removing ExtensionResource in the OnImageLoaded
callback? I realize we have so far only used the extension_id part of that
object, but aa requested it specifically when I wrote this class -- not sure if
he had something special in mind.
http://codereview.chromium.org/1075006/diff/64001/chrome/browser/extensions/i...
(see last comment).

Also, TEST= needs fleshing out and the ImageLoadingTracker unit test should be
updated to test this new functionality.

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_shortcut_manager.cc (right):

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/ap...
chrome/browser/extensions/app_shortcut_manager.cc:44: if (!image) {
image can no longer be a null image? Why not use isEmpty?

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
chrome/browser/extensions/image_loading_tracker.cc:205: return;
Wait? We don't report anything back to the observer in this case?

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
chrome/browser/extensions/image_loading_tracker.cc:216: if (info->cache == CACHE
&& info->extension &&
Why null check |extension|? You dereference it when accepting the request (to
find the id()) and you set cache == DONT_CACHE at the bottom of this file. I
would think |extension| is mandatory and can be DCHECK'ed when receiving the
request, no?

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
chrome/browser/extensions/image_loading_tracker.cc:222: // If all pending images
are done report back.
nit: missing comma after 'done'?

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
File chrome/browser/extensions/image_loading_tracker.h (right):

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
chrome/browser/extensions/image_loading_tracker.h:65: ImageInfo(const
ExtensionResource resource, gfx::Size max_size);
nit: Personally, I would define the dtor in the .cc file, even though it is
empty (like you do with the other struct).

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
chrome/browser/extensions/image_loading_tracker.h:95: struct PendingLoadInfo {
nit: Document this struct (and other struct above) and their members.

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
chrome/browser/extensions/image_loading_tracker.h:100: std::string extension_id;
Why do you need this if you have the pointer to the extension?

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
chrome/browser/extensions/image_loading_tracker.h:106: typedef std::map<int,
PendingLoadInfo> LoadMap;
Not your fault, but can you document this mapping? I just realized <int,
PendingLoadInfo> isn't very readable. :)

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/ui/cocoa/info...
File chrome/browser/ui/cocoa/infobars/extension_infobar_controller.mm (left):

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/ui/cocoa/info...
chrome/browser/ui/cocoa/infobars/extension_infobar_controller.mm:74: if
(!icon_resource.relative_path().empty()) {
Hmmm... are you sure about this being redundant?

[Mihai Parparita -not on Chrome, 2012-02-23 18:37:58]

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/ui/cocoa/info...
File chrome/browser/ui/cocoa/infobars/extension_infobar_controller.mm (left):

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/ui/cocoa/info...
chrome/browser/ui/cocoa/infobars/extension_infobar_controller.mm:74: if
(!icon_resource.relative_path().empty()) {
On 2012/02/23 12:05:42, Finnur wrote:
> Hmmm... are you sure about this being redundant?

ImageLoadingTracker::LoadImage has the same resource.relative_path().empty()
check (that immediately invokes OnImageLoaded) on lines 174-179.

[sail, 2012-02-23 18:38:03]

On 2012/02/23 12:05:41, Finnur wrote:
> I reviewed the changes to the ImageLoadingTracker and reviewed loosely how
they
> affect the consumer of the ImageLoadingTracker.

Thanks for taking a look!

> What was the impetus for removing ExtensionResource in the OnImageLoaded
> callback? I realize we have so far only used the extension_id part of that
> object, but aa requested it specifically when I wrote this class -- not sure
if
> he had something special in mind.

With the new API the ExtensionResource parameter would have to become
std::vector<ExtensionResource>. There were only two call sites that actually
used it and both of them just wanted the extension ID. We can always add it back
if it becomes useful.

> Also, TEST= needs fleshing out and the ImageLoadingTracker unit test should be
> updated to test this new functionality.

Will do.

>
http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/ap...
> File chrome/browser/extensions/app_shortcut_manager.cc (right):
> 
>
http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/ap...
> chrome/browser/extensions/app_shortcut_manager.cc:44: if (!image) {
> image can no longer be a null image? Why not use isEmpty?
> 
>
http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
> File chrome/browser/extensions/image_loading_tracker.cc (right):
> 
>
http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
> chrome/browser/extensions/image_loading_tracker.cc:205: return;
> Wait? We don't report anything back to the observer in this case?
> 
>
http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
> chrome/browser/extensions/image_loading_tracker.cc:216: if (info->cache ==
CACHE
> && info->extension &&
> Why null check |extension|? You dereference it when accepting the request (to
> find the id()) and you set cache == DONT_CACHE at the bottom of this file. I
> would think |extension| is mandatory and can be DCHECK'ed when receiving the
> request, no?
> 
>
http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
> chrome/browser/extensions/image_loading_tracker.cc:222: // If all pending
images
> are done report back.
> nit: missing comma after 'done'?
> 
>
http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
> File chrome/browser/extensions/image_loading_tracker.h (right):
> 
>
http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
> chrome/browser/extensions/image_loading_tracker.h:65: ImageInfo(const
> ExtensionResource resource, gfx::Size max_size);
> nit: Personally, I would define the dtor in the .cc file, even though it is
> empty (like you do with the other struct).
> 
>
http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
> chrome/browser/extensions/image_loading_tracker.h:95: struct PendingLoadInfo {
> nit: Document this struct (and other struct above) and their members.
> 
>
http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
> chrome/browser/extensions/image_loading_tracker.h:100: std::string
extension_id;
> Why do you need this if you have the pointer to the extension?
> 
>
http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
> chrome/browser/extensions/image_loading_tracker.h:106: typedef std::map<int,
> PendingLoadInfo> LoadMap;
> Not your fault, but can you document this mapping? I just realized <int,
> PendingLoadInfo> isn't very readable. :)
> 
>
http://codereview.chromium.org/9428025/diff/7002/chrome/browser/ui/cocoa/info...
> File chrome/browser/ui/cocoa/infobars/extension_infobar_controller.mm (left):
> 
>
http://codereview.chromium.org/9428025/diff/7002/chrome/browser/ui/cocoa/info...
> chrome/browser/ui/cocoa/infobars/extension_infobar_controller.mm:74: if
> (!icon_resource.relative_path().empty()) {
> Hmmm... are you sure about this being redundant?

[sail, 2012-02-27 23:58:37]

Addressed review comments and added unit tests. Please take another look.

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_shortcut_manager.cc (right):

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/ap...
chrome/browser/extensions/app_shortcut_manager.cc:44: if (!image) {
On 2012/02/23 12:05:42, Finnur wrote:
> image can no longer be a null image? Why not use isEmpty?

Done.

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
chrome/browser/extensions/image_loading_tracker.cc:205: return;
On 2012/02/23 12:05:42, Finnur wrote:
> Wait? We don't report anything back to the observer in this case?

Done.

Changed this to a DCHECK. With the new code we always keep the info in the map
so this should never happen.

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
chrome/browser/extensions/image_loading_tracker.cc:216: if (info->cache == CACHE
&& info->extension &&
On 2012/02/23 12:05:42, Finnur wrote:
> Why null check |extension|? You dereference it when accepting the request (to
> find the id()) and you set cache == DONT_CACHE at the bottom of this file. I
> would think |extension| is mandatory and can be DCHECK'ed when receiving the
> request, no?

Done.
Changed to a DCHECK

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
chrome/browser/extensions/image_loading_tracker.cc:222: // If all pending images
are done report back.
On 2012/02/23 12:05:42, Finnur wrote:
> nit: missing comma after 'done'?

Done.

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
File chrome/browser/extensions/image_loading_tracker.h (right):

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
chrome/browser/extensions/image_loading_tracker.h:65: ImageInfo(const
ExtensionResource resource, gfx::Size max_size);
On 2012/02/23 12:05:42, Finnur wrote:
> nit: Personally, I would define the dtor in the .cc file, even though it is
> empty (like you do with the other struct).

Done.

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
chrome/browser/extensions/image_loading_tracker.h:95: struct PendingLoadInfo {
On 2012/02/23 12:05:42, Finnur wrote:
> nit: Document this struct (and other struct above) and their members.

Done.

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
chrome/browser/extensions/image_loading_tracker.h:100: std::string extension_id;
On 2012/02/23 12:05:42, Finnur wrote:
> Why do you need this if you have the pointer to the extension?

This is needed incase the extension is unload. Added a comment.

http://codereview.chromium.org/9428025/diff/7002/chrome/browser/extensions/im...
chrome/browser/extensions/image_loading_tracker.h:106: typedef std::map<int,
PendingLoadInfo> LoadMap;
On 2012/02/23 12:05:42, Finnur wrote:
> Not your fault, but can you document this mapping? I just realized <int,
> PendingLoadInfo> isn't very readable. :)

Done.

[sail, 2012-02-27 23:59:37]

To make this CL a little smaller I removed all the gtk/views code. I'll update
gtk and views in a separate CL and checkin both CLs together.

[Finnur, 2012-02-28 10:26:51]

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/a...
File chrome/browser/extensions/app_shortcut_manager.cc (right):

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/a...
chrome/browser/extensions/app_shortcut_manager.cc:120: // icon_resources may
still be empty at this point, in which case LoadImage
nit: icon_resources -> |icon_resources|

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/a...
chrome/browser/extensions/app_shortcut_manager.cc:121: // which call the
OnImageLoaded callback with an empty image and exit
s/which/will/ ?

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_uninstall_dialog.h (right):

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_uninstall_dialog.h:59: // Sets the icon that
will be used in the dialog. If |icon| is contains an
nit: Redundant word: 'is'

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:40: //
ImageLoadingTracker::ImageLoader
nit: Maybe add a similar heading for ImageInfo above?

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.h (right):

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.h:67: gfx::Size max_size;
I think it is worth calling out that the image will be resized if larger than
max_size, like we do in the comments for LoadImage below.

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.h:82: // Same as LoadImage()
above except it loads multiple images from the same
nit: It bothers me a bit that we can't agree on whether to call a gfx::Image
object with multiple dimensions an *image* (as the object name implies) or
*images* (plural)... Here, we call it Images but when the return call comes we
call it Image. Any thoughts on how we can consolidate this?

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.h:101: // This is cached
separate from |extension| incase the extension in
nit: incase -> in case

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.h:139: // identifies the id
assigned to the request.
nit: The reference to 'The integer' is probably a bit lost on people here. Not
sure how to improve on it, though. Feel free to ignore. :)

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker_unittest.cc (right):

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker_unittest.cc:188: // Tests
loading multiple images.
same here: Tests loading multiple dimension of the same image.

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker_unittest.cc:203:
ImageLoadingTracker loader(static_cast<ImageLoadingTracker::Observer*>(this));
I forget, why is this cast needed again?

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app_mac.mm (right):

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_mac.mm:43: switch ((int)size.width) {
Style guide frowns upon this casting style: (int) foo

[sail, 2012-02-28 23:25:36]

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/exte...
File chrome/browser/extensions/app_shortcut_manager.cc (right):

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/exte...
chrome/browser/extensions/app_shortcut_manager.cc:120: // icon_resources may
still be empty at this point, in which case LoadImage
On 2012/02/28 10:26:51, Finnur wrote:
> nit: icon_resources -> |icon_resources|

Done.

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/exte...
chrome/browser/extensions/app_shortcut_manager.cc:121: // which call the
OnImageLoaded callback with an empty image and exit
On 2012/02/28 10:26:51, Finnur wrote:
> s/which/will/ ?

Done.

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/exte...
File chrome/browser/extensions/extension_uninstall_dialog.h (right):

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/exte...
chrome/browser/extensions/extension_uninstall_dialog.h:59: // Sets the icon that
will be used in the dialog. If |icon| is contains an
On 2012/02/28 10:26:51, Finnur wrote:
> nit: Redundant word: 'is'

Done.

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/exte...
File chrome/browser/extensions/image_loading_tracker.cc (right):

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.cc:40: //
ImageLoadingTracker::ImageLoader
On 2012/02/28 10:26:51, Finnur wrote:
> nit: Maybe add a similar heading for ImageInfo above?

Done.

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/exte...
File chrome/browser/extensions/image_loading_tracker.h (right):

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.h:67: gfx::Size max_size;
On 2012/02/28 10:26:51, Finnur wrote:
> I think it is worth calling out that the image will be resized if larger than
> max_size, like we do in the comments for LoadImage below.

Done.

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.h:67: gfx::Size max_size;
On 2012/02/28 10:26:51, Finnur wrote:
> I think it is worth calling out that the image will be resized if larger than
> max_size, like we do in the comments for LoadImage below.

Done.

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.h:82: // Same as LoadImage()
above except it loads multiple images from the same
On 2012/02/28 10:26:51, Finnur wrote:
> nit: It bothers me a bit that we can't agree on whether to call a gfx::Image
> object with multiple dimensions an *image* (as the object name implies) or
> *images* (plural)... Here, we call it Images but when the return call comes we
> call it Image. Any thoughts on how we can consolidate this?

How about renaming this function LoadImage?

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.h:101: // This is cached
separate from |extension| incase the extension in
On 2012/02/28 10:26:51, Finnur wrote:
> nit: incase -> in case

Done.

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/exte...
File chrome/browser/extensions/image_loading_tracker_unittest.cc (right):

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker_unittest.cc:188: // Tests
loading multiple images.
On 2012/02/28 10:26:51, Finnur wrote:
> same here: Tests loading multiple dimension of the same image.

Done.

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker_unittest.cc:203:
ImageLoadingTracker loader(static_cast<ImageLoadingTracker::Observer*>(this));
On 2012/02/28 10:26:51, Finnur wrote:
> I forget, why is this cast needed again?

Done.
Not sure, removed.

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9428025/diff/12001/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:43: switch ((int)size.width) {
On 2012/02/28 10:26:51, Finnur wrote:
> Style guide frowns upon this casting style: (int) foo

Done.

[sail, 2012-02-28 23:28:23]

jhawkins: web_ui/*
sky: shell_integration.h, web_applicaitons/*
rsesek: cocoa/* web_app_mac*

Note, I'm working on a follow up CL to update gtk and views.

[Finnur, 2012-02-29 00:17:26]

ImageLoadingTracker changes LGTM.

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.h (right):

http://codereview.chromium.org/9428025/diff/12001/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.h:82: // Same as LoadImage()
above except it loads multiple images from the same
The style guide doesn't like functions with same name but different params
(except in cases where the params are roughly equivalent (like std::string and
std::wstring)).

On 2012/02/28 23:25:36, sail wrote:
> On 2012/02/28 10:26:51, Finnur wrote:
> > nit: It bothers me a bit that we can't agree on whether to call a gfx::Image
> > object with multiple dimensions an *image* (as the object name implies) or
> > *images* (plural)... Here, we call it Images but when the return call comes
we
> > call it Image. Any thoughts on how we can consolidate this?
> 
> How about renaming this function LoadImage?

[sky, 2012-02-29 00:39:19]

shell_integration LGTM. Get one of the mac guys to review web_applications

[jeremy, 2012-02-29 07:13:46]

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:172: bool image_added = false;
Doesn't look like you check the value of this?

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:179: if
(!AddBitmapImageRepToIconFamily(icon_family, image_rep))
Could you add a comment on why it's ok to continue without failing here?

Is it possible that the favicon doesn't contain square images or images of the
wrong size?

[James Hawkins, 2012-02-29 10:36:16]

webui lgtm

[Robert Sesek, 2012-02-29 15:13:37]

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/back...
File chrome/browser/background/background_application_list_model.cc (right):

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/back...
chrome/browser/background/background_application_list_model.cc:138:
icon_.reset(new SkBitmap(*image.ToSkBitmap()));
CopySkBitmap()

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/exte...
File chrome/browser/extensions/app_shortcut_manager.cc (right):

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/exte...
chrome/browser/extensions/app_shortcut_manager.cc:51: shortcut_info_.favicon =
gfx::Image(new SkBitmap(bmp));
Just |bmp| is now okay.

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/exte...
File chrome/browser/extensions/image_loading_tracker.h (right):

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.h:22: class Image;
nit: shouldn't be indented

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/ui/w...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/ui/w...
chrome/browser/ui/web_applications/web_app_ui.cc:313: new
SkBitmap(tab_contents_wrapper->favicon_tab_helper()->GetFavicon()));
The |new SkBitmap| is no longer needed.

[sail, 2012-02-29 19:45:15]

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/back...
File chrome/browser/background/background_application_list_model.cc (right):

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/back...
chrome/browser/background/background_application_list_model.cc:138:
icon_.reset(new SkBitmap(*image.ToSkBitmap()));
On 2012/02/29 15:13:37, rsesek wrote:
> CopySkBitmap()

Done.
I had to change CopySkBitmap() to return SkBitmap* instead of a const SkBitmap*.

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/exte...
File chrome/browser/extensions/app_shortcut_manager.cc (right):

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/exte...
chrome/browser/extensions/app_shortcut_manager.cc:51: shortcut_info_.favicon =
gfx::Image(new SkBitmap(bmp));
On 2012/02/29 15:13:37, rsesek wrote:
> Just |bmp| is now okay.

Done.

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/exte...
File chrome/browser/extensions/image_loading_tracker.h (right):

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.h:22: class Image;
On 2012/02/29 15:13:37, rsesek wrote:
> nit: shouldn't be indented

Done.

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/ui/w...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/ui/w...
chrome/browser/ui/web_applications/web_app_ui.cc:313: new
SkBitmap(tab_contents_wrapper->favicon_tab_helper()->GetFavicon()));
On 2012/02/29 15:13:37, rsesek wrote:
> The |new SkBitmap| is no longer needed.

Done.

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:172: bool image_added = false;
On 2012/02/29 07:13:47, jeremy wrote:
> Doesn't look like you check the value of this?

Ouch good catch. I dropped this when rebasing. Re-added.

https://chromiumcodereview.appspot.com/9428025/diff/27001/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:179: if
(!AddBitmapImageRepToIconFamily(icon_family, image_rep))
On 2012/02/29 07:13:47, jeremy wrote:
> Could you add a comment on why it's ok to continue without failing here?
> 
> Is it possible that the favicon doesn't contain square images or images of the
> wrong size?

Done.

Added a comment.

[jeremy, 2012-03-01 10:54:38]

lgtm though I'd prefer adding a test for the case of an invalid bitmap before
landing.

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:180: // doesn't work.
nit: "doesn't work" -> fails ?

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac_unittest.mm (right):

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac_unittest.mm:93:
IDR_PRODUCT_LOGO_32);
How hard would it be to add a test for a bitmap for which
AddBitmapImageRepToIconFamily() fails?

[sail, 2012-03-04 04:59:56]

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:180: // doesn't work.
On 2012/03/01 10:54:39, jeremy wrote:
> nit: "doesn't work" -> fails ?

I didn't want to fails in the sentence.

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac_unittest.mm (right):

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac_unittest.mm:93:
IDR_PRODUCT_LOGO_32);
On 2012/03/01 10:54:39, jeremy wrote:
> How hard would it be to add a test for a bitmap for which
> AddBitmapImageRepToIconFamily() fails?

There wasn't an easy way to do this. I'll look into this for a second CL.

[sail, 2012-03-04 05:00:21]

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:180: // doesn't work.
On 2012/03/04 04:59:56, sail wrote:
> On 2012/03/01 10:54:39, jeremy wrote:
> > nit: "doesn't work" -> fails ?
> 
> I didn't want to fails in the sentence.

"to fails" -> "two fails"

[Robert Sesek, 2012-03-05 15:18:32]

lgtm

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/exte...
File chrome/browser/extensions/app_shortcut_manager.cc (right):

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/exte...
chrome/browser/extensions/app_shortcut_manager.cc:26: const int kDesiredSizes[]
= {16, 32, 128, 256, 512};
Do we need 1024 for ML?

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/exte...
File chrome/browser/extensions/image_loading_tracker.cc (right):

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.cc:216: DCHECK(it !=
load_map_.end());
Can't remember if it works for stl iterators, but DCHECK_NE

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.cc:221:
DCHECK(info->pending_count > 0);
DCHECK_GT

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/exte...
File chrome/browser/extensions/image_loading_tracker.h (right):

https://chromiumcodereview.appspot.com/9428025/diff/35002/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.h:65: ~ImageInfo();
nit: blank line after