Show extension icons next to their top-level context menu items....

Show extension icons next to their top-level context menu items.

Also fix a bug in extension icon caching where we weren't keeping track of
potential resizing done by ImageLoadingTracker before setting the cached
SkBitmap.

BUG=39494
TEST=Install an extension that includes an icon and uses the experimental
context menu API. You should see the extension's icon in the context menu
next to its top-level item.


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

[asargent_no_longer_on_chrome, 2010-06-17 00:27:08]

aa for the main review
+cc sky for the fix to extension icon caching

[sky, 2010-06-17 15:54:55]

http://codereview.chromium.org/2867008/diff/1/14
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/2867008/diff/1/14#newcode1581
chrome/common/extensions/extension.cc:1581: ImageCacheKey
key(source.relative_path(), SizeToString(max_size));
Lets say you have an image of size 16x16. If you then ask for the image with a
max size of 24x24 and again with 48x48 we're going to end up caching two
separate images that should be identical, right?

One idea is if the image doesn't get scaled we store it with the size string, if
it is scaled we store it with the size string. The lookup then becomes check for
unscaled first, and if it's smaller than max, return it. Otherwise fetch, scale
and store with max size. Alternatively we could store both scaled and unscaled,
but I'm assuming we don't want to always keep around the unscaled image.

[Aaron Boodman, 2010-06-18 20:37:47]

http://codereview.chromium.org/2867008/diff/1/3
File chrome/browser/extensions/extension_menu_manager.cc (right):

http://codereview.chromium.org/2867008/diff/1/3#newcode151
chrome/browser/extensions/extension_menu_manager.cc:151: if
(!icon_resource.extension_root().empty()) {
Seems useful to add an empty() method to ExtensionResource. Would make this more
readable.

http://codereview.chromium.org/2867008/diff/1/3#newcode152
chrome/browser/extensions/extension_menu_manager.cc:152:
image_tracker_.LoadImage(extension,
I believe that the contract for LoadImage() allows it to callback re-entrantly.
That wouldn't work here because the item isn't in context_items_ yet. You should
move this code down below where context_items_ is populated.

http://codereview.chromium.org/2867008/diff/1/4
File chrome/browser/extensions/extension_menu_manager.h (right):

http://codereview.chromium.org/2867008/diff/1/4#newcode176
chrome/browser/extensions/extension_menu_manager.h:176: NotificationObserver {
You want public inheritance for NotificationObserver, right? Best to make that
explicit.

http://codereview.chromium.org/2867008/diff/1/4#newcode193
chrome/browser/extensions/extension_menu_manager.h:193: int
AddContextItem(Extension* extension, ExtensionMenuItem* item);
Maybe add a note about what |extension| is used for?

http://codereview.chromium.org/2867008/diff/1/4#newcode245
chrome/browser/extensions/extension_menu_manager.h:245: SkBitmap*
GetDefaultIcon();
SkBitmap does internal refcounting, so you can return it by value here.

http://codereview.chromium.org/2867008/diff/1/4#newcode265
chrome/browser/extensions/extension_menu_manager.h:265: std::map<std::string,
SkBitmap> extension_icons_;
This map could use a comment. What is the std::string?

http://codereview.chromium.org/2867008/diff/1/4#newcode266
chrome/browser/extensions/extension_menu_manager.h:266: scoped_ptr<SkBitmap>
default_icon_;
Same thing here. scoped_ptr is unnecessary with SkBitmap.

http://codereview.chromium.org/2867008/diff/1/15
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/2867008/diff/1/15#newcode375
chrome/common/extensions/extension.h:375: typedef std::pair<FilePath,
std::string> ImageCacheKey;
Does gfx::Size not work for some reason? It has an == operator.

http://codereview.chromium.org/2867008/diff/1/17
File chrome/common/extensions/extension_resource.h (right):

http://codereview.chromium.org/2867008/diff/1/17#newcode66
chrome/common/extensions/extension_resource.h:66: std::string extension_id_;
Please try to comment data members.

[asargent_no_longer_on_chrome, 2010-06-22 17:40:37]

http://codereview.chromium.org/2867008/diff/1/3
File chrome/browser/extensions/extension_menu_manager.cc (right):

http://codereview.chromium.org/2867008/diff/1/3#newcode151
chrome/browser/extensions/extension_menu_manager.cc:151: if
(!icon_resource.extension_root().empty()) {
On 2010/06/18 20:37:47, Aaron Boodman wrote:
> Seems useful to add an empty() method to ExtensionResource. Would make this
more
> readable.

Done.

http://codereview.chromium.org/2867008/diff/1/3#newcode152
chrome/browser/extensions/extension_menu_manager.cc:152:
image_tracker_.LoadImage(extension,
On 2010/06/18 20:37:47, Aaron Boodman wrote:
> I believe that the contract for LoadImage() allows it to callback
re-entrantly.
> That wouldn't work here because the item isn't in context_items_ yet. You
should
> move this code down below where context_items_ is populated.

Good catch. Fixed.

http://codereview.chromium.org/2867008/diff/1/4
File chrome/browser/extensions/extension_menu_manager.h (right):

http://codereview.chromium.org/2867008/diff/1/4#newcode176
chrome/browser/extensions/extension_menu_manager.h:176: NotificationObserver {
On 2010/06/18 20:37:47, Aaron Boodman wrote:
> You want public inheritance for NotificationObserver, right? Best to make that
> explicit.

Oops, I think that was a reformatting typo. Good catch.

http://codereview.chromium.org/2867008/diff/1/4#newcode193
chrome/browser/extensions/extension_menu_manager.h:193: int
AddContextItem(Extension* extension, ExtensionMenuItem* item);
On 2010/06/18 20:37:47, Aaron Boodman wrote:
> Maybe add a note about what |extension| is used for?

Done.

http://codereview.chromium.org/2867008/diff/1/4#newcode245
chrome/browser/extensions/extension_menu_manager.h:245: SkBitmap*
GetDefaultIcon();
On 2010/06/18 20:37:47, Aaron Boodman wrote:
> SkBitmap does internal refcounting, so you can return it by value here.

I decided to change this to EnsureDefaultIcon().

http://codereview.chromium.org/2867008/diff/1/4#newcode265
chrome/browser/extensions/extension_menu_manager.h:265: std::map<std::string,
SkBitmap> extension_icons_;
On 2010/06/18 20:37:47, Aaron Boodman wrote:
> This map could use a comment. What is the std::string?

Done.

http://codereview.chromium.org/2867008/diff/1/4#newcode266
chrome/browser/extensions/extension_menu_manager.h:266: scoped_ptr<SkBitmap>
default_icon_;
On 2010/06/18 20:37:47, Aaron Boodman wrote:
> Same thing here. scoped_ptr is unnecessary with SkBitmap.

Done.

http://codereview.chromium.org/2867008/diff/1/14
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/2867008/diff/1/14#newcode1581
chrome/common/extensions/extension.cc:1581: ImageCacheKey
key(source.relative_path(), SizeToString(max_size));
On 2010/06/17 15:54:55, sky wrote:
> Lets say you have an image of size 16x16. If you then ask for the image with a
> max size of 24x24 and again with 48x48 we're going to end up caching two
> separate images that should be identical, right?
> 
> One idea is if the image doesn't get scaled we store it with the size string,
if
> it is scaled we store it with the size string. The lookup then becomes check
for
> unscaled first, and if it's smaller than max, return it. Otherwise fetch,
scale
> and store with max size. Alternatively we could store both scaled and
unscaled,
> but I'm assuming we don't want to always keep around the unscaled image.

I ended up doing something very similar to what you suggested. When setting an
item in the cache, I make the caller pass in the original size of the image, and
use either the scaled size or the empty string in the key depending on whether
the image is scaled down or at its original size. Then for lookup I search for
an image of max_size first, and if that isn't found then I look for the empty
string (meaning original size) and return that if it isn't too big.

http://codereview.chromium.org/2867008/diff/1/15
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/2867008/diff/1/15#newcode375
chrome/common/extensions/extension.h:375: typedef std::pair<FilePath,
std::string> ImageCacheKey;
On 2010/06/18 20:37:47, Aaron Boodman wrote:
> Does gfx::Size not work for some reason? It has an == operator.

For std::map, you also need operator< and operator> because the keys are kept in
sorted order. And for a hash_map, you need to provide a hash function. I can
look into doing that if you think it would be cleaner.

http://codereview.chromium.org/2867008/diff/1/17
File chrome/common/extensions/extension_resource.h (right):

http://codereview.chromium.org/2867008/diff/1/17#newcode66
chrome/common/extensions/extension_resource.h:66: std::string extension_id_;
On 2010/06/18 20:37:47, Aaron Boodman wrote:
> Please try to comment data members.

Done.

[sky, 2010-06-22 20:26:24]

The caching changes LGTM. Could you add a unit test for coverage though?

http://codereview.chromium.org/2867008/diff/16002/31014
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/2867008/diff/16002/31014#newcode1584
chrome/common/extensions/extension.cc:1584: image_cache_[ImageCacheKey(path,
"")] = image;
"" -> std::string()

http://codereview.chromium.org/2867008/diff/16002/31014#newcode1611
chrome/common/extensions/extension.cc:1611: ImageCacheKey(path,
SizeToString(max_size)));
indent 4.

http://codereview.chromium.org/2867008/diff/16002/31014#newcode1617
chrome/common/extensions/extension.cc:1617: i =
image_cache_.find(ImageCacheKey(path, ""));
"" -> std::string()

[asargent_no_longer_on_chrome, 2010-06-22 23:07:45]

Uploaded a new version fixing nits and including the requested unit test for
image caching.

http://codereview.chromium.org/2867008/diff/16002/31014
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/2867008/diff/16002/31014#newcode1584
chrome/common/extensions/extension.cc:1584: image_cache_[ImageCacheKey(path,
"")] = image;
On 2010/06/22 20:26:24, sky wrote:
> "" -> std::string()

Done.

http://codereview.chromium.org/2867008/diff/16002/31014#newcode1611
chrome/common/extensions/extension.cc:1611: ImageCacheKey(path,
SizeToString(max_size)));
On 2010/06/22 20:26:24, sky wrote:
> indent 4.

Done.

http://codereview.chromium.org/2867008/diff/16002/31014#newcode1617
chrome/common/extensions/extension.cc:1617: i =
image_cache_.find(ImageCacheKey(path, ""));
On 2010/06/22 20:26:24, sky wrote:
> "" -> std::string()

Done.

[Aaron Boodman, 2010-06-24 18:02:39]

lgtm