Use the extension icon for extension omnibox results instead of the generic

Use the extension icon for extension omnibox results instead of the generic
search icon.

I refactored the extension menu manager to separate the icon-specific bits.

BUG=46479
TEST=load the chrome search extension at
src/chrome/common/extensions/docs/examples/extensions/chrome_search/
and type "src foo" into the omnibox. You should see the extension icon instead of the magnifying glass. Switch back and forth between the "src" keyword result, other results, and other keywords and the icons should update properly.

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

[Matt Perry, 2010-07-12 22:19:48]

Peter: autocomplete*
Antony: the rest

[Peter Kasting, 2010-07-12 22:48:06]

http://codereview.chromium.org/2973006/diff/2001/3001
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/2973006/diff/2001/3001#newcode19
chrome/browser/autocomplete/autocomplete_edit.cc:19: #include
"chrome/browser/extensions/extensions_service.h"
Nit: Also not sure why this got added

http://codereview.chromium.org/2973006/diff/2001/3002
File chrome/browser/autocomplete/autocomplete_edit.h (right):

http://codereview.chromium.org/2973006/diff/2001/3002#newcode8
chrome/browser/autocomplete/autocomplete_edit.h:8: #include "base/scoped_ptr.h"
Nit: Why add this?  I don't see any scoped_XXX usage in this header.

http://codereview.chromium.org/2973006/diff/2001/3004
File chrome/browser/autocomplete/autocomplete_popup_model.h (right):

http://codereview.chromium.org/2973006/diff/2001/3004#newcode117
chrome/browser/autocomplete/autocomplete_popup_model.h:117: const SkBitmap*
GetIconForMatch(const AutocompleteMatch& match) const;
Nit: I wonder if there's a better name for this... "GetIconForMatch()" sounds
pretty simple and clear -- it gets the icon for the match.  Except this function
doesn't always.  Maybe "ExtensionIconForMatch()" or "GetSpecialIconForMatch()"
or something?  I dunno.

http://codereview.chromium.org/2973006/diff/2001/3008
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/2973006/diff/2001/3008#newcode373
chrome/browser/autocomplete/autocomplete_popup_view_mac.mm:373: const int
resource_id = match.starred ? IDR_OMNIBOX_STAR
Nit: Wrap after '?' instead of before ':'

http://codereview.chromium.org/2973006/diff/2001/3010
File chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/2973006/diff/2001/3010#newcode217
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:217: // Get
the magnifying glass to put at the front of the string.
Nit: Comment is out of date.

http://codereview.chromium.org/2973006/diff/2001/3012
File chrome/browser/cocoa/location_bar/location_bar_view_mac.mm (right):

http://codereview.chromium.org/2973006/diff/2001/3012#newcode489
chrome/browser/cocoa/location_bar/location_bar_view_mac.mm:489: return
keyword_image_;
Nit: Is this caching important?  It looks like the code below is synchronous, so
it seems like we could just do without it?

http://codereview.chromium.org/2973006/diff/2001/3013
File chrome/browser/cocoa/location_bar/location_bar_view_mac_unittest.mm
(right):

http://codereview.chromium.org/2973006/diff/2001/3013#newcode82
chrome/browser/cocoa/location_bar/location_bar_view_mac_unittest.mm:82: NSImage*
keyword_image = image;  // image choice is irrelevant
Nit: Comments should be complete sentences

http://codereview.chromium.org/2973006/diff/2001/3020
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/2973006/diff/2001/3020#newcode880
chrome/browser/gtk/location_bar_view_gtk.cc:880: last_keyword_ = keyword;
Nit: Is this caching important?  It looks like the code below is synchronous, so
it seems like we could just do without it?

http://codereview.chromium.org/2973006/diff/2001/3021
File chrome/browser/gtk/location_bar_view_gtk.h (right):

http://codereview.chromium.org/2973006/diff/2001/3021#newcode365
chrome/browser/gtk/location_bar_view_gtk.h:365: GtkWidget* tab_to_search_lens_;
Nit: I think elsewhere we might call this "magnifier"?  "image" would be OK too.

http://codereview.chromium.org/2973006/diff/2001/3026
File chrome/browser/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/2973006/diff/2001/3026#newcode469
chrome/browser/views/location_bar/location_bar_view.cc:469:
selected_keyword_view_->SetImage(bitmap);
Doesn't this have the problem that if we try to use a "normal" keyword after
using an extension keyword, we'll still show the icon from the extension?

[asargent_no_longer_on_chrome, 2010-07-12 23:45:56]

Extension parts mostly LG with one small issue.

(If you had any horrible problems with your handling of NSImage or GdkPixbuf I
wouldn't have spotted them being unfamiliar with those platform APIs, so I'm
assuming Peter is familiar with their proper usage.)

http://codereview.chromium.org/2973006/diff/2001/3004
File chrome/browser/autocomplete/autocomplete_popup_model.h (right):

http://codereview.chromium.org/2973006/diff/2001/3004#newcode14
chrome/browser/autocomplete/autocomplete_popup_model.h:14: class
ExtensionIconManager;
Nit: looks like you don't actually use this declaration in this .h file.

http://codereview.chromium.org/2973006/diff/2001/3015
File chrome/browser/extensions/extension_icon_manager.h (right):

http://codereview.chromium.org/2973006/diff/2001/3015#newcode37
chrome/browser/extensions/extension_icon_manager.h:37: void set_monochrome(bool
value) { monochrome_ = value; }
Why have a method for this instead of either an argument to the
ExtensionIconManager constructor, or the LoadIcon/GetIcon methods?

http://codereview.chromium.org/2973006/diff/2001/3018
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/2973006/diff/2001/3018#newcode191
chrome/browser/extensions/extensions_service.cc:191:
omnibox_icon_manager_.set_monochrome(true);
It seems like this should just be a parameter to the constructor.

[Matt Perry, 2010-07-13 00:24:16]

http://codereview.chromium.org/2973006/diff/2001/3001
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/2973006/diff/2001/3001#newcode19
chrome/browser/autocomplete/autocomplete_edit.cc:19: #include
"chrome/browser/extensions/extensions_service.h"
On 2010/07/12 22:48:06, Peter Kasting wrote:
> Nit: Also not sure why this got added

Done.

http://codereview.chromium.org/2973006/diff/2001/3002
File chrome/browser/autocomplete/autocomplete_edit.h (right):

http://codereview.chromium.org/2973006/diff/2001/3002#newcode8
chrome/browser/autocomplete/autocomplete_edit.h:8: #include "base/scoped_ptr.h"
On 2010/07/12 22:48:06, Peter Kasting wrote:
> Nit: Why add this?  I don't see any scoped_XXX usage in this header.

Done.

http://codereview.chromium.org/2973006/diff/2001/3004
File chrome/browser/autocomplete/autocomplete_popup_model.h (right):

http://codereview.chromium.org/2973006/diff/2001/3004#newcode14
chrome/browser/autocomplete/autocomplete_popup_model.h:14: class
ExtensionIconManager;
On 2010/07/12 23:45:57, Antony Sargent wrote:
> Nit: looks like you don't actually use this declaration in this .h file.

Done.

http://codereview.chromium.org/2973006/diff/2001/3004#newcode117
chrome/browser/autocomplete/autocomplete_popup_model.h:117: const SkBitmap*
GetIconForMatch(const AutocompleteMatch& match) const;
On 2010/07/12 22:48:06, Peter Kasting wrote:
> Nit: I wonder if there's a better name for this... "GetIconForMatch()" sounds
> pretty simple and clear -- it gets the icon for the match.  Except this
function
> doesn't always.  Maybe "ExtensionIconForMatch()" or "GetSpecialIconForMatch()"
> or something?  I dunno.

Done.

http://codereview.chromium.org/2973006/diff/2001/3008
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/2973006/diff/2001/3008#newcode373
chrome/browser/autocomplete/autocomplete_popup_view_mac.mm:373: const int
resource_id = match.starred ? IDR_OMNIBOX_STAR
On 2010/07/12 22:48:06, Peter Kasting wrote:
> Nit: Wrap after '?' instead of before ':'

Done.

http://codereview.chromium.org/2973006/diff/2001/3010
File chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/2973006/diff/2001/3010#newcode217
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:217: // Get
the magnifying glass to put at the front of the string.
On 2010/07/12 22:48:06, Peter Kasting wrote:
> Nit: Comment is out of date.

Done.

http://codereview.chromium.org/2973006/diff/2001/3012
File chrome/browser/cocoa/location_bar/location_bar_view_mac.mm (right):

http://codereview.chromium.org/2973006/diff/2001/3012#newcode489
chrome/browser/cocoa/location_bar/location_bar_view_mac.mm:489: return
keyword_image_;
On 2010/07/12 22:48:06, Peter Kasting wrote:
> Nit: Is this caching important?  It looks like the code below is synchronous,
so
> it seems like we could just do without it?

True, but my assumption was that creating an NSImage for a SkBitmap was slow.
The linux version claims that is the case for GdkPixmaps. Also, it just seemed
lame to create a new image each time the user typed a character (which is when
this gets called).

http://codereview.chromium.org/2973006/diff/2001/3013
File chrome/browser/cocoa/location_bar/location_bar_view_mac_unittest.mm
(right):

http://codereview.chromium.org/2973006/diff/2001/3013#newcode82
chrome/browser/cocoa/location_bar/location_bar_view_mac_unittest.mm:82: NSImage*
keyword_image = image;  // image choice is irrelevant
On 2010/07/12 22:48:06, Peter Kasting wrote:
> Nit: Comments should be complete sentences

Ah, I guess the style guide changed since I last looked and EOL comments were
exempt from this.

http://codereview.chromium.org/2973006/diff/2001/3015
File chrome/browser/extensions/extension_icon_manager.h (right):

http://codereview.chromium.org/2973006/diff/2001/3015#newcode37
chrome/browser/extensions/extension_icon_manager.h:37: void set_monochrome(bool
value) { monochrome_ = value; }
On 2010/07/12 23:45:57, Antony Sargent wrote:
> Why have a method for this instead of either an argument to the
> ExtensionIconManager constructor, or the LoadIcon/GetIcon methods?

Making it apply to all icons seems easier and suits my needs for now. As for not
making it a constructor param, it's just a matter of style; this way is more
explicit than seeing "new ExtensionIconManager(true)"

http://codereview.chromium.org/2973006/diff/2001/3021
File chrome/browser/gtk/location_bar_view_gtk.h (right):

http://codereview.chromium.org/2973006/diff/2001/3021#newcode365
chrome/browser/gtk/location_bar_view_gtk.h:365: GtkWidget* tab_to_search_lens_;
On 2010/07/12 22:48:06, Peter Kasting wrote:
> Nit: I think elsewhere we might call this "magnifier"?  "image" would be OK
too.

Done.

http://codereview.chromium.org/2973006/diff/2001/3026
File chrome/browser/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/2973006/diff/2001/3026#newcode469
chrome/browser/views/location_bar/location_bar_view.cc:469:
selected_keyword_view_->SetImage(bitmap);
On 2010/07/12 22:48:06, Peter Kasting wrote:
> Doesn't this have the problem that if we try to use a "normal" keyword after
> using an extension keyword, we'll still show the icon from the extension?

Good catch! Done.

[Peter Kasting, 2010-07-13 00:30:12]

LGTM

http://codereview.chromium.org/2973006/diff/16001/17022
File chrome/browser/views/location_bar/icon_label_bubble_view.cc (right):

http://codereview.chromium.org/2973006/diff/16001/17022#newcode23
chrome/browser/views/location_bar/icon_label_bubble_view.cc:23: int
contained_image,
Nit: Could we now get rid of contained_image and the resource_bundle.h #include?

[asargent_no_longer_on_chrome, 2010-07-13 00:34:00]

Ok, LGTM

[Scott Hess - ex-Googler, 2010-07-13 21:24:29]

LGTM, though I encourage just dropping the SkBitmap->NSImage cache, unless my
comments show that I misunderstand things.

http://codereview.chromium.org/2973006/diff/16001/17005
File chrome/browser/autocomplete/autocomplete_popup_view_mac.h (right):

http://codereview.chromium.org/2973006/diff/16001/17005#newcode27
chrome/browser/autocomplete/autocomplete_popup_view_mac.h:27: @class NSImage;
Elsewhere the style is to just alphabetize class/@class in one big block.

http://codereview.chromium.org/2973006/diff/16001/17005#newcode135
chrome/browser/autocomplete/autocomplete_popup_view_mac.h:135: ImageMap images_;
How many of these are we going to have at any given time?  I'm not sure that a
cache is really justified if we're only looking at only one or two conversions
per keystroke.  And the field already will be doing one.

Additionally - are we guaranteed that the SkBitmap* will live for the long term,
even when the extension is uninstalled or updated?  Seems to me like we might
end up with a certain number of undead objects in there, which will result in
extraneous NSImage objects hanging around.