Move icon fallbacks into ExtensionAction.

Move icon fallbacks into ExtensionAction.

This will help with changing the icon's appearance when it's in
request-to-act mode.

To deal with the fact that ExtensionAction can't load files, the
platform-specific code now explicitly caches icons into the
ExtensionAction.

I also noticed that several users were converting the returned icons
into another format, and caching the most recent such conversion.
Since gfx::Image does that automatically, I switched to storing that
instead of SkBitmap.

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

[Jeffrey Yasskin, 2012-07-25 19:45:26]

PTAL

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (left):

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:67: [owner_
updateState];
This updateState call is provided by the function that creates the
ExtensionImageTrackerBridge, and similarly for the Windows version.

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:181:
SetImage([[[NSImage alloc] initWithSize:default_size] autorelease]);
Fixed an unrelated leak.

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action_unittest.cc (right):

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action_unittest.cc:46: ExtensionAction
action;
Breaks the naming rule in order to keep the diff small.

[not at google - send to devlin, 2012-07-26 02:29:44]

nice cleanup

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:102: const
Extension* const extension_;
We only ever access extension_->browser_action(), doesn't seem necessary to hold
onto the whole extension, particularly since it's weak?

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:263: [self
setImage:image.ToNSImage()];
nit: no {}

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:204: }
nit: remove surrounding {}

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:225: }
same nit

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action.cc (right):

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.cc:199: const PathToIconCache& cache)
const {
strange to pass in the cache then not use it sometime. Pulling this logic into a
separate class would be cleaner, I think.

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.cc:250: }
anonymous namespaces preferred over static methods, iirc for debugging/stack
trace purposes?

that said it's only used in the one place.

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action.h (right):

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.h:162: gfx::Image GetIcon(int tab_id,
const PathToIconCache& cache) const;
A method like this makes me wonder if PathToIconCache should be a class like
ExtensionAction::IconCache, with an ExtensionAction curried in.

I also found it pretty confusing seeing PathToIconCache in other code when it's
just an alias for a map; I was expecting it to be a class.

---

Really I think all this icon loading logic should be pulled somewhere inside
chrome/extensions/browser and somehow combined with, or replacing,
ImageLoadingTracker. I say that because there's still a lot of shared
boilerplate for image loading in the UI code that could be cleaner.

However since I'm not prepared to do that, I don't expect anybody else to be.

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action_unittest.cc (right):

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action_unittest.cc:19: static bool
ImagesAreEqual(const gfx::Image& i1, const gfx::Image& i2) {
ditto anonymous namespace, and next function.

In fact, the whole test class can be put inside namespace{} I believe.

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ma...
File
chrome/common/extensions/manifest_tests/extension_manifests_scriptbadge_unittest.cc
(right):

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ma...
chrome/common/extensions/manifest_tests/extension_manifests_scriptbadge_unittest.cc:21:
static bool ImagesAreEqual(const gfx::Image& i1, const gfx::Image& i2) {
ditto

[Jeffrey Yasskin, 2012-07-26 21:11:47]

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:102: const
Extension* const extension_;
On 2012/07/26 02:29:44, kalman wrote:
> We only ever access extension_->browser_action(), doesn't seem necessary to
hold
> onto the whole extension, particularly since it's weak?

Good point. Turned this into the browser action.

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:263: [self
setImage:image.ToNSImage()];
On 2012/07/26 02:29:44, kalman wrote:
> nit: no {}

Done.

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:204: }
On 2012/07/26 02:29:44, kalman wrote:
> nit: remove surrounding {}

Done.

http://codereview.chromium.org/10806058/diff/8001/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:225: }
On 2012/07/26 02:29:44, kalman wrote:
> same nit

Ah the perils of removing code. :)

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action.cc (right):

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.cc:199: const PathToIconCache& cache)
const {
On 2012/07/26 02:29:44, kalman wrote:
> strange to pass in the cache then not use it sometime. Pulling this logic into
a
> separate class would be cleaner, I think.

Are you referring to the fact that if an icon is set as an icon instead of a
path, we don't need the cache? That's true, but the goal of this method is that
the caller shouldn't need to know which cases need a cache vs which cases don't.

I think making PathToIconCache a class is overkill since this and some map<>
methods would be its only operations, and there's no real gain from either
encapsulating a few of map<>'s methods or moving the logic away from this file.
When we write the interface to let common/ code load resources, the cache will
move into an ExtensionAction member, and this logic will just need to move back
into ExtensionAction, yielding more churn.

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.cc:250: }
On 2012/07/26 02:29:44, kalman wrote:
> anonymous namespaces preferred over static methods, iirc for debugging/stack
> trace purposes?
> 
> that said it's only used in the one place.

Sure, done. Is there any practical effect beyond spelling the function as
namespace::(anonymous)::Function instead of namespace::Function?

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action.h (right):

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.h:162: gfx::Image GetIcon(int tab_id,
const PathToIconCache& cache) const;
On 2012/07/26 02:29:44, kalman wrote:
> A method like this makes me wonder if PathToIconCache should be a class like
> ExtensionAction::IconCache, with an ExtensionAction curried in.
> 
> I also found it pretty confusing seeing PathToIconCache in other code when
it's
> just an alias for a map; I was expecting it to be a class.

Would you like me to duplicate the map<> arguments in each place I use the type?
I tend to think that kind of duplication makes code easier to read, but so many
people seem to like the typedefs that I got lazy and used one here.

> ---
> 
> Really I think all this icon loading logic should be pulled somewhere inside
> chrome/extensions/browser and somehow combined with, or replacing,
> ImageLoadingTracker. I say that because there's still a lot of shared
> boilerplate for image loading in the UI code that could be cleaner.
> 
> However since I'm not prepared to do that, I don't expect anybody else to be.

I definitely agree that there should be some sort of structure for loading
images/resources from common code. I don't feel like I have enough context yet
to design it (presumably images aren't the only thing we want to load?), and I'd
rather not put a big refactoring before the grey'ed out request-to-act badges.
This was the simplest change that lets me do the icon-greying without changes to
platform-specific code.

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action_unittest.cc (right):

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action_unittest.cc:19: static bool
ImagesAreEqual(const gfx::Image& i1, const gfx::Image& i2) {
On 2012/07/26 02:29:44, kalman wrote:
> ditto anonymous namespace, and next function.
> 
> In fact, the whole test class can be put inside namespace{} I believe.

Done.

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ma...
File
chrome/common/extensions/manifest_tests/extension_manifests_scriptbadge_unittest.cc
(right):

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ma...
chrome/common/extensions/manifest_tests/extension_manifests_scriptbadge_unittest.cc:21:
static bool ImagesAreEqual(const gfx::Image& i1, const gfx::Image& i2) {
On 2012/07/26 02:29:44, kalman wrote:
> ditto

Done.

[not at google - send to devlin, 2012-07-27 03:16:12]

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action.cc (right):

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.cc:199: const PathToIconCache& cache)
const {
On 2012/07/26 21:11:47, Jeffrey Yasskin wrote:
> On 2012/07/26 02:29:44, kalman wrote:
> > strange to pass in the cache then not use it sometime. Pulling this logic
into
> a
> > separate class would be cleaner, I think.
> 
> Are you referring to the fact that if an icon is set as an icon instead of a
> path, we don't need the cache? That's true, but the goal of this method is
that
> the caller shouldn't need to know which cases need a cache vs which cases
don't.

Of course. That's not what I meant; the caching should definitely return the
icon transparently.

I just meant that it's odd from an OO perspective to be passing an object into
another one, when all state behaviour could be encapsulated in the thing that's
being passed. I.e. make the map a little bit smarter.

> 
> I think making PathToIconCache a class is overkill since this and some map<>
> methods would be its only operations, and there's no real gain from either
> encapsulating a few of map<>'s methods or moving the logic away from this
file.
> When we write the interface to let common/ code load resources, the cache will
> move into an ExtensionAction member, and this logic will just need to move
back
> into ExtensionAction, yielding more churn.

Mm, I think it would end up being simpler, and easier to understand / less
boilerplatey for calling code. The class isn't exactly complicated, and it
encapsulates the purpose of what you're trying to do. I wrote a comment in
ExtensionAction.

To put it another way, all calling code cares about is getting an icon. It has a
tab ID. The callers shouldn't need to depend on there being a map from path ->
icon somewhere, that's an implementation detail.

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.cc:250: }
On 2012/07/26 21:11:47, Jeffrey Yasskin wrote:
> On 2012/07/26 02:29:44, kalman wrote:
> > anonymous namespaces preferred over static methods, iirc for debugging/stack
> > trace purposes?
> > 
> > that said it's only used in the one place.
> 
> Sure, done. Is there any practical effect beyond spelling the function as
> namespace::(anonymous)::Function instead of namespace::Function?

No idea. Reviewer once made me do it, said something about optimisations bleh.
Just seems to be the general common practice in chrome though, much as I prefer
the "static" keyword (Java background; plus for little utility methods, it's
cleaner).

http://codereview.chromium.org/10806058/diff/15002/chrome/common/extensions/e...
File chrome/common/extensions/extension_action.h (right):

http://codereview.chromium.org/10806058/diff/15002/chrome/common/extensions/e...
chrome/common/extensions/extension_action.h:158: 
I meant like

class ExtensionAction {
 public:
  class IconCache {
   public:
    gfx::Image GetForTab(int tab_id) const {
      ...
    }

    // either

    void SetForPathIndex(int index, gfx::Image image) {
      ...
    }

    void SetForDefaultPath(gfx::Image image) {
    }

    // or

    void SetForPath(const std::string& path, gfx::Image image) {
      ...
    }

   private:
    // does ExtensionAction need to be a friend to do this? Eh.
    IconCache(ExtensionAction* action) : action_(action) {}

    ExtensionAction* action_;

    typedef std::map<std::string, gfx::Image> PathToIconCache;
    PathToIconCache cache_;

    DISALLOW_COPY_AND_ASSIGN(IconCache);
  };

  IconCache& cache() { return cache; }
  const IconCache& cache() const { return cache; }
};

---

Then the UI code which only needs the cache can hold onto that rather than the
ExtensionAction + some random map, and know that the random map needs to be
passed into the method on ExtensionAction that it needs.

---

The other option would be to incorporate the cache into ExtensionAction, but I
can see why you didn't; it'd be some non-sequiturish methods, which are nicely
grouped into a cache-style object.

[Jeffrey Yasskin, 2012-07-31 14:40:01]

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action.cc (right):

http://codereview.chromium.org/10806058/diff/8001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.cc:199: const PathToIconCache& cache)
const {
On 2012/07/27 03:16:12, kalman wrote:
> On 2012/07/26 21:11:47, Jeffrey Yasskin wrote:
> > On 2012/07/26 02:29:44, kalman wrote:
> > > strange to pass in the cache then not use it sometime. Pulling this logic
> into
> > a
> > > separate class would be cleaner, I think.
> > 
> > Are you referring to the fact that if an icon is set as an icon instead of a
> > path, we don't need the cache? That's true, but the goal of this method is
> that
> > the caller shouldn't need to know which cases need a cache vs which cases
> don't.
> 
> Of course. That's not what I meant; the caching should definitely return the
> icon transparently.
> 
> I just meant that it's odd from an OO perspective to be passing an object into
> another one, when all state behaviour could be encapsulated in the thing
that's
> being passed. I.e. make the map a little bit smarter.
> 
> > 
> > I think making PathToIconCache a class is overkill since this and some map<>
> > methods would be its only operations, and there's no real gain from either
> > encapsulating a few of map<>'s methods or moving the logic away from this
> file.
> > When we write the interface to let common/ code load resources, the cache
will
> > move into an ExtensionAction member, and this logic will just need to move
> back
> > into ExtensionAction, yielding more churn.
> 
> Mm, I think it would end up being simpler, and easier to understand / less
> boilerplatey for calling code. The class isn't exactly complicated, and it
> encapsulates the purpose of what you're trying to do. I wrote a comment in
> ExtensionAction.
> 
> To put it another way, all calling code cares about is getting an icon. It has
a
> tab ID. The callers shouldn't need to depend on there being a map from path ->
> icon somewhere, that's an implementation detail.

Done, I think.

http://codereview.chromium.org/10806058/diff/15002/chrome/common/extensions/e...
File chrome/common/extensions/extension_action.h (right):

http://codereview.chromium.org/10806058/diff/15002/chrome/common/extensions/e...
chrome/common/extensions/extension_action.h:158: 
On 2012/07/27 03:16:12, kalman wrote:
> The other option would be to incorporate the cache into ExtensionAction, but I
> can see why you didn't; it'd be some non-sequiturish methods, which are nicely
> grouped into a cache-style object. 

I've done this one.

[not at google - send to devlin, 2012-07-31 15:39:02]

lgtm

[commit-bot: I haz the power, 2012-07-31 15:43:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/10806058/18019

[commit-bot: I haz the power, 2012-07-31 15:43:32]

Presubmit check for 10806058-18019 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/gtk
    chrome/browser/ui
    chrome/browser/ui/cocoa/location_bar
    chrome/browser/ui/cocoa

Presubmit checks took 1.5s to calculate.

[Jeffrey Yasskin, 2012-07-31 15:46:00]

Hi Scott, I need OWNERS approval for the refactoring inside ui.

[sky, 2012-07-31 16:25:18]

http://codereview.chromium.org/10806058/diff/18019/chrome/browser/ui/views/br...
File chrome/browser/ui/views/browser_action_view.cc (left):

http://codereview.chromium.org/10806058/diff/18019/chrome/browser/ui/views/br...
chrome/browser/ui/views/browser_action_view.cc:102: UpdateState();
Are you sure you don't still need the call to UpdateState here?

http://codereview.chromium.org/10806058/diff/18019/chrome/browser/ui/views/br...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10806058/diff/18019/chrome/browser/ui/views/br...
chrome/browser/ui/views/browser_action_view.cc:171: SkBitmap
icon(*browser_action()->GetIcon(tab_id).ToSkBitmap());
I believe all this code should be using ImageSkia.

[Jeffrey Yasskin, 2012-07-31 19:53:20]

http://codereview.chromium.org/10806058/diff/18019/chrome/browser/ui/views/br...
File chrome/browser/ui/views/browser_action_view.cc (left):

http://codereview.chromium.org/10806058/diff/18019/chrome/browser/ui/views/br...
chrome/browser/ui/views/browser_action_view.cc:102: UpdateState();
On 2012/07/31 16:25:19, sky wrote:
> Are you sure you don't still need the call to UpdateState here?

Pretty sure, but not 100%. I believe the only time the call to UpdateState()
matters is when this is called indirectly from
BrowserActionView::BrowserActionView(), which calls UpdateState() itself. Is
there some other time I've missed?

http://codereview.chromium.org/10806058/diff/18019/chrome/browser/ui/views/br...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10806058/diff/18019/chrome/browser/ui/views/br...
chrome/browser/ui/views/browser_action_view.cc:171: SkBitmap
icon(*browser_action()->GetIcon(tab_id).ToSkBitmap());
On 2012/07/31 16:25:19, sky wrote:
> I believe all this code should be using ImageSkia.

I believe you're right. However, I don't think this is the right CL to do that
migration because it's primarily focused on something else.

By the way, is there a porting guide for migrating SkBitmap to gfx::Image? I
tried to avoid Image->SkBitmap->Image conversions in this CL, but I was stumped
by things like finding the analog of SkDevice. Also, is there a guideline for
picking a scale factor when building an SkCanvas out of an ImageSkia?

[sky, 2012-07-31 23:36:37]

LGTM

Peter might be able to point you at a document detailing migrating from SkBitmap
to ImageSkia.

[commit-bot: I haz the power, 2012-08-01 05:38:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/10806058/18019

[commit-bot: I haz the power, 2012-08-01 07:22:34]

Change committed as 149385