[Extensions] Cache extension action icons

Cache extension action icons so that they don't have to be
reloaded each time a new view is created (i.e., each time a
new browser or the new overflow menu is opened).

BUG=452958

Review URL: https://codereview.chromium.org/885443004

Cr-Commit-Position: refs/heads/master@{#313724}

chrome/browser/extensions/api/system_indicator/system_indicator_manager.cc

@@ -30,7 +30,7 @@ class ExtensionIndicatorIcon : public StatusIconObserver,
                                public ExtensionActionIconFactory::Observer {
  public:
   static ExtensionIndicatorIcon* Create(const Extension* extension,
-                                        const ExtensionAction* action,
+                                        ExtensionAction* action,
                                         Profile* profile,
                                         StatusTray* status_tray);
   ~ExtensionIndicatorIcon() override;
@@ -43,7 +43,7 @@ class ExtensionIndicatorIcon : public StatusIconObserver,
 
  private:
   ExtensionIndicatorIcon(const Extension* extension,
-                         const ExtensionAction* action,
+                         ExtensionAction* action,
                          Profile* profile,
                          StatusTray* status_tray);
 
@@ -56,7 +56,7 @@ class ExtensionIndicatorIcon : public StatusIconObserver,
 
 ExtensionIndicatorIcon* ExtensionIndicatorIcon::Create(
     const Extension* extension,
-    const ExtensionAction* action,
+    ExtensionAction* action,
     Profile* profile,
     StatusTray* status_tray) {
   scoped_ptr<ExtensionIndicatorIcon> extension_icon(
@@ -96,7 +96,7 @@ void ExtensionIndicatorIcon::OnIconUpdated() {
 }
 
 ExtensionIndicatorIcon::ExtensionIndicatorIcon(const Extension* extension,
-                                               const ExtensionAction* action,
+                                               ExtensionAction* action,
                                                Profile* profile,
                                                StatusTray* status_tray)
     : extension_(extension),
@@ -176,7 +176,7 @@ bool SystemIndicatorManager::SendClickEventToExtensionForTest(
 
 void SystemIndicatorManager::CreateOrUpdateIndicator(
     const Extension* extension,
-    const ExtensionAction* extension_action) {
+    ExtensionAction* extension_action) {
   DCHECK(thread_checker_.CalledOnValidThread());
   SystemIndicatorMap::iterator it = system_indicators_.find(extension->id());
   if (it != system_indicators_.end()) {

chrome/browser/extensions/api/system_indicator/system_indicator_manager.h

@@ -61,7 +61,7 @@ class SystemIndicatorManager : public ExtensionRegistryObserver,
   // the indicator if necessary.
   void CreateOrUpdateIndicator(
       const Extension* extension,
-      const ExtensionAction* extension_action);
+      ExtensionAction* extension_action);
 
   // Causes the indicator for the given extension to be hidden.
   void RemoveIndicator(const std::string &extension_id);

chrome/browser/extensions/extension_action.cc

@@ -7,12 +7,13 @@
 #include <algorithm>
 
 #include "base/base64.h"
-#include "base/bind.h"
 #include "base/logging.h"
-#include "base/message_loop/message_loop.h"
 #include "chrome/common/badge_util.h"
 #include "chrome/common/icon_with_badge_image_source.h"
+#include "extensions/browser/extension_icon_image.h"
 #include "extensions/common/constants.h"
+#include "extensions/common/extension_icon_set.h"
+#include "extensions/common/manifest_handlers/icons_handler.h"
 #include "grit/theme_resources.h"
 #include "grit/ui_resources.h"
 #include "ipc/ipc_message.h"
@@ -36,6 +37,29 @@
 
 namespace {
 
+// Returns the default icon image for extensions.
+gfx::Image GetDefaultIcon() {
+  return ui::ResourceBundle::GetSharedInstance().GetImageNamed(
+      IDR_EXTENSIONS_FAVICON);
+}
+
+// Given the extension action type, returns the size the extension action icon
+// should have. The icon should be square, so only one dimension is
+// returned.
+int GetIconSizeForType(extensions::ActionInfo::Type type) {
+  switch (type) {
+    case extensions::ActionInfo::TYPE_BROWSER:
+    case extensions::ActionInfo::TYPE_PAGE:
+    case extensions::ActionInfo::TYPE_SYSTEM_INDICATOR:
+      // TODO(dewittj) Report the actual icon size of the system
+      // indicator.
+      return extension_misc::EXTENSION_ICON_ACTION;
+    default:
+      NOTREACHED();
+      return 0;
+  }
+}
+
 class GetAttentionImageSource : public gfx::ImageSkiaSource {
  public:
   explicit GetAttentionImageSource(const gfx::ImageSkia& icon)
@@ -76,61 +100,20 @@ const int ExtensionAction::kDefaultTabId = -1;
 const int ExtensionAction::kPageActionIconMaxSize =
     extension_misc::EXTENSION_ICON_ACTION;
 
-ExtensionAction::ExtensionAction(const std::string& extension_id,
+ExtensionAction::ExtensionAction(const extensions::Extension& extension,
                                  extensions::ActionInfo::Type action_type,
                                  const extensions::ActionInfo& manifest_data)
-    : extension_id_(extension_id), action_type_(action_type) {
+    : extension_id_(extension.id()), action_type_(action_type) {
   // Page/script actions are hidden/disabled by default, and browser actions are
   // visible/enabled by default.
   SetIsVisible(kDefaultTabId,
                action_type == extensions::ActionInfo::TYPE_BROWSER);
-  SetTitle(kDefaultTabId, manifest_data.default_title);
-  SetPopupUrl(kDefaultTabId, manifest_data.default_popup_url);
-  if (!manifest_data.default_icon.empty()) {
-    set_default_icon(make_scoped_ptr(new ExtensionIconSet(
-        manifest_data.default_icon)));
-  }
-  set_id(manifest_data.id);
+  Populate(extension, manifest_data);
 }
 
 ExtensionAction::~ExtensionAction() {
 }
 
-scoped_ptr<ExtensionAction> ExtensionAction::CopyForTest() const {
-  scoped_ptr<ExtensionAction> copy(
-      new ExtensionAction(extension_id_, action_type_,
-                          extensions::ActionInfo()));
-  copy->popup_url_ = popup_url_;
-  copy->title_ = title_;
-  copy->icon_ = icon_;
-  copy->badge_text_ = badge_text_;
-  copy->badge_background_color_ = badge_background_color_;
-  copy->badge_text_color_ = badge_text_color_;
-  copy->is_visible_ = is_visible_;
-  copy->id_ = id_;
-
-  if (default_icon_)
-    copy->default_icon_.reset(new ExtensionIconSet(*default_icon_));
-
-  return copy.Pass();
-}
-
-// static
-int ExtensionAction::GetIconSizeForType(
-    extensions::ActionInfo::Type type) {
-  switch (type) {
-    case extensions::ActionInfo::TYPE_BROWSER:
-    case extensions::ActionInfo::TYPE_PAGE:
-    case extensions::ActionInfo::TYPE_SYSTEM_INDICATOR:
-      // TODO(dewittj) Report the actual icon size of the system
-      // indicator.
-      return extension_misc::EXTENSION_ICON_ACTION;
-    default:
-      NOTREACHED();
-      return 0;
-  }
-}
-
 void ExtensionAction::SetPopupUrl(int tab_id, const GURL& url) {
   // We store |url| even if it is empty, rather than removing a URL from the
   // map.  If an extension has a default popup, and removes it for a tab via
@@ -282,6 +265,28 @@ gfx::ImageSkia ExtensionAction::GetIconWithBadge(
      icon.size());
 }
 
+extensions::IconImage* ExtensionAction::LoadDefaultIconImage(
+    const extensions::Extension& extension,
+    content::BrowserContext* browser_context) {
+  if (default_icon_ && !default_icon_image_) {
+    default_icon_image_.reset(new extensions::IconImage(
+        browser_context,
+        &extension,
+        *default_icon(),
+        GetIconSizeForType(action_type_),
+        *GetDefaultIcon().ToImageSkia(),
+        nullptr));
+  }
+  return default_icon_image_.get();
+}
+
+gfx::ImageSkia ExtensionAction::GetDefaultIconImage() const {
+  // If we have a default icon, it should be loaded before trying to use it.
+  DCHECK(!default_icon_image_ == !default_icon_);
+  return default_icon_image_ ? default_icon_image_->image_skia() :
+      *GetDefaultIcon().ToImageSkia();
+}
+
 bool ExtensionAction::HasPopupUrl(int tab_id) const {
   return HasValue(popup_url_, tab_id);
 }
@@ -310,6 +315,52 @@ bool ExtensionAction::HasIcon(int tab_id) const {
   return HasValue(icon_, tab_id);
 }
 
+void ExtensionAction::SetDefaultIconForTest(
+    scoped_ptr<ExtensionIconSet> default_icon) {
+  default_icon_ = default_icon.Pass();
+}
+
+void ExtensionAction::Populate(const extensions::Extension& extension,
+                               const extensions::ActionInfo& manifest_data) {
+  // If the manifest doesn't specify a title, set it to |extension|'s name.
+  const std::string& title =
+      !manifest_data.default_title.empty() ? manifest_data.default_title :
+      extension.name();
+  SetTitle(kDefaultTabId, title);
+  SetPopupUrl(kDefaultTabId, manifest_data.default_popup_url);
+  set_id(manifest_data.id);
+
+  // Initialize the specified icon set.
+  if (!manifest_data.default_icon.empty())
+    default_icon_.reset(new ExtensionIconSet(manifest_data.default_icon));
+
+  const ExtensionIconSet& extension_icons =
+      extensions::IconsInfo::GetIcons(&extension);
+  // Look for any other icons.
+  std::string largest_icon = extension_icons.Get(
+      extension_misc::EXTENSION_ICON_GIGANTOR,
+      ExtensionIconSet::MATCH_SMALLER);
+
+  if (!largest_icon.empty()) {
+    // We found an icon to use, so create an icon set if one doesn't exist.
+    if (!default_icon_)
+      default_icon_.reset(new ExtensionIconSet());
+    int largest_icon_size = extension_icons.GetIconSizeFromPath(largest_icon);
+    // Replace any missing extension action icons with the largest icon
+    // retrieved from |extension|'s manifest so long as the largest icon is
+    // larger than the current key.
+    for (int i = extension_misc::kNumExtensionActionIconSizes - 1;
+         i >= 0; --i) {
+      int size = extension_misc::kExtensionActionIconSizes[i].size;
+      if (default_icon_->Get(size, ExtensionIconSet::MATCH_BIGGER).empty()
+          && largest_icon_size > size) {
+        default_icon_->Add(size, largest_icon);
+        break;
+      }
+    }
+  }
+}
+
 // Determines which icon would be returned by |GetIcon|, and returns its width.
 int ExtensionAction::GetIconWidth(int tab_id) const {
   // If icon has been set, return its width.
@@ -324,5 +375,5 @@ int ExtensionAction::GetIconWidth(int tab_id) const {
   // If no icon has been set and there is no default icon, we need favicon
   // width.
   return ui::ResourceBundle::GetSharedInstance().GetImageNamed(
-          IDR_EXTENSIONS_FAVICON).ToImageSkia()->width();
+          IDR_EXTENSIONS_FAVICON).Width();
 }

chrome/browser/extensions/extension_action.h

@@ -9,21 +9,21 @@
 #include <string>
 #include <vector>
 
-#include "base/basictypes.h"
-#include "base/memory/linked_ptr.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/scoped_vector.h"
-#include "base/memory/weak_ptr.h"
-#include "base/observer_list.h"
+#include "base/stl_util.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
-#include "extensions/common/extension_icon_set.h"
 #include "third_party/skia/include/core/SkColor.h"
-// TODO(robertphillips): change this to "class SkBaseDevice;"
-#include "third_party/skia/include/core/SkDevice.h"
-#include "ui/gfx/animation/linear_animation.h"
 
 class GURL;
-class SkBitmap;
+
+namespace content {
+class BrowserContext;
+}
+
+namespace extensions {
+class Extension;
+class IconImage;
+}
 
 namespace gfx {
 class Canvas;
@@ -53,20 +53,11 @@ class ExtensionAction {
   // Max size (both dimensions) for page actions.
   static const int kPageActionIconMaxSize;
 
-  ExtensionAction(const std::string& extension_id,
+  ExtensionAction(const extensions::Extension& extension,
                   extensions::ActionInfo::Type action_type,
                   const extensions::ActionInfo& manifest_data);
   ~ExtensionAction();
 
-  // Gets a copy of this, ownership passed to caller.
-  // It doesn't make sense to copy of an ExtensionAction except in tests.
-  scoped_ptr<ExtensionAction> CopyForTest() const;
-
-  // Given the extension action type, returns the size the extension action icon
-  // should have. The icon should be square, so only one dimension is
-  // returned.
-  static int GetIconSizeForType(extensions::ActionInfo::Type type);
-
   // extension id
   const std::string& extension_id() const { return extension_id_; }
 
@@ -121,12 +112,6 @@ class ExtensionAction {
   void DeclarativeSetIcon(int tab_id, int priority, const gfx::Image& icon);
   void UndoDeclarativeSetIcon(int tab_id, int priority, const gfx::Image& icon);
 
-  // Non-tab-specific icon path. This is used to support the default_icon key of
-  // page and browser actions.
-  void set_default_icon(scoped_ptr<ExtensionIconSet> icon_set) {
-     default_icon_ = icon_set.Pass();
-  }
-
   const ExtensionIconSet* default_icon() const {
     return default_icon_.get();
   }
@@ -202,6 +187,16 @@ class ExtensionAction {
                                   int tab_id,
                                   const gfx::Size& spacing) const;
 
+  // Lazily loads and returns the default icon image, if one exists for the
+  // action.
+  extensions::IconImage* LoadDefaultIconImage(
+      const extensions::Extension& extension,
+      content::BrowserContext* browser_context);
+
+  // Returns the image to use as the default icon for the action. Can only be
+  // called after LoadDefaultIconImage().
+  gfx::ImageSkia GetDefaultIconImage() const;
+
   // Determine whether or not the ExtensionAction has a value set for the given
   // |tab_id| for each property.
   bool HasPopupUrl(int tab_id) const;
@@ -212,7 +207,14 @@ class ExtensionAction {
   bool HasIsVisible(int tab_id) const;
   bool HasIcon(int tab_id) const;
 
+  void SetDefaultIconForTest(scoped_ptr<ExtensionIconSet> default_icon);
+
  private:
+  // Populates the action from the |extension| and |manifest_data|, filling in
+  // any missing values (like title or icons) as possible.
+  void Populate(const extensions::Extension& extension,
+                const extensions::ActionInfo& manifest_data);
+
   // Returns width of the current icon for tab_id.
   // TODO(tbarzic): The icon selection is done in ExtensionActionIconFactory.
   // We should probably move this there too.
@@ -284,7 +286,11 @@ class ExtensionAction {
 
   // ExtensionIconSet containing paths to bitmaps from which default icon's
   // image representations will be selected.
-  scoped_ptr<const ExtensionIconSet> default_icon_;
+  scoped_ptr<ExtensionIconSet> default_icon_;
+
+  // The default icon image, if |default_icon_| exists.
+  // Lazily initialized via LoadDefaultIconImage().
+  scoped_ptr<extensions::IconImage> default_icon_image_;
 
   // The id for the ExtensionAction, for example: "RssPageAction". This is
   // needed for compat with an older version of the page actions API.

chrome/browser/extensions/extension_action_icon_factory.cc

@@ -6,41 +6,25 @@
 
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/profiles/profile.h"
-#include "extensions/common/extension.h"
-#include "extensions/common/extension_icon_set.h"
-#include "grit/theme_resources.h"
-#include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_skia.h"
 
 using extensions::Extension;
 using extensions::IconImage;
 
-namespace {
-
-gfx::ImageSkia GetDefaultIcon() {
-  return *ui::ResourceBundle::GetSharedInstance().GetImageNamed(
-      IDR_EXTENSIONS_FAVICON).ToImageSkia();
-}
-
-}  // namespace
-
 ExtensionActionIconFactory::ExtensionActionIconFactory(
     Profile* profile,
     const Extension* extension,
-    const ExtensionAction* action,
+    ExtensionAction* action,
     Observer* observer)
     : extension_(extension),
       action_(action),
-      observer_(observer) {
-  if (action_->default_icon()) {
-    default_icon_.reset(new IconImage(
-        profile,
-        extension_,
-        *action_->default_icon(),
-        ExtensionAction::GetIconSizeForType(action_->action_type()),
-        GetDefaultIcon(),
-        this));
-  }
+      observer_(observer),
+      icon_image_observer_(this) {
+  extensions::IconImage* default_icon_image =
+      action->LoadDefaultIconImage(*extension, profile);
+  if (default_icon_image)
+    icon_image_observer_.Add(default_icon_image);
 }
 
 ExtensionActionIconFactory::~ExtensionActionIconFactory() {}
@@ -51,9 +35,13 @@ void ExtensionActionIconFactory::OnExtensionIconImageChanged(IconImage* image) {
     observer_->OnIconUpdated();
 }
 
+void ExtensionActionIconFactory::OnExtensionIconImageDestroyed(
+    IconImage* image) {
+  icon_image_observer_.RemoveAll();
+}
+
 gfx::Image ExtensionActionIconFactory::GetIcon(int tab_id) {
-  gfx::ImageSkia base_icon = GetBaseIconFromAction(tab_id);
-  return gfx::Image(base_icon);
+  return gfx::Image(GetBaseIconFromAction(tab_id));
 }
 
 gfx::ImageSkia ExtensionActionIconFactory::GetBaseIconFromAction(int tab_id) {
@@ -65,8 +53,5 @@ gfx::ImageSkia ExtensionActionIconFactory::GetBaseIconFromAction(int tab_id) {
   if (!icon.isNull())
     return icon;
 
-  if (default_icon_)
-    return default_icon_->image_skia();
-
-  return GetDefaultIcon();
+  return action_->GetDefaultIconImage();
 }

chrome/browser/extensions/extension_action_icon_factory.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_EXTENSIONS_EXTENSION_ACTION_ICON_FACTORY_H_
 
 #include "base/memory/scoped_ptr.h"
+#include "base/scoped_observer.h"
 #include "extensions/browser/extension_icon_image.h"
 
 class ExtensionAction;
@@ -33,12 +34,13 @@ class ExtensionActionIconFactory : public extensions::IconImage::Observer {
   // Observer should outlive this.
   ExtensionActionIconFactory(Profile* profile,
                              const extensions::Extension* extension,
-                             const ExtensionAction* action,
+                             ExtensionAction* action,
                              Observer* observer);
   ~ExtensionActionIconFactory() override;
 
   // extensions::IconImage override.
   void OnExtensionIconImageChanged(extensions::IconImage* image) override;
+  void OnExtensionIconImageDestroyed(extensions::IconImage* image) override;
 
   // Gets the extension action icon for the tab.
   // If there is an icon set using |SetIcon|, that icon is returned.
@@ -58,8 +60,9 @@ class ExtensionActionIconFactory : public extensions::IconImage::Observer {
   const extensions::Extension* extension_;
   const ExtensionAction* action_;
   Observer* observer_;
-  // Underlying icon image for the default icon.
-  scoped_ptr<extensions::IconImage> default_icon_;
+
+  ScopedObserver<extensions::IconImage, extensions::IconImage::Observer>
+      icon_image_observer_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionActionIconFactory);
 };

chrome/browser/extensions/extension_action_icon_factory_unittest.cc

@@ -263,7 +263,7 @@ TEST_F(ExtensionActionIconFactoryTest, DefaultIcon) {
   scoped_ptr<ExtensionIconSet> default_icon_set(new ExtensionIconSet());
   default_icon_set->Add(19, "icon.png");
 
-  browser_action->set_default_icon(default_icon_set.Pass());
+  browser_action->SetDefaultIconForTest(default_icon_set.Pass());
   ASSERT_TRUE(browser_action->default_icon());
 
   ExtensionActionIconFactory icon_factory(

chrome/browser/extensions/extension_action_manager.cc

@@ -12,7 +12,6 @@
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/extensions_browser_client.h"
 #include "extensions/common/constants.h"
-#include "extensions/common/manifest_handlers/icons_handler.h"
 
 namespace extensions {
 
@@ -85,41 +84,6 @@ void ExtensionActionManager::OnExtensionUnloaded(
 
 namespace {
 
-// Loads resources missing from |action| (i.e. title, icons) from the "icons"
-// key of |extension|'s manifest.
-void PopulateMissingValues(const Extension& extension,
-                           ExtensionAction* action) {
-  // If the title is missing from |action|, set it to |extension|'s name.
-  if (action->GetTitle(ExtensionAction::kDefaultTabId).empty())
-    action->SetTitle(ExtensionAction::kDefaultTabId, extension.name());
-
-  scoped_ptr<ExtensionIconSet> default_icon(new ExtensionIconSet());
-  if (action->default_icon())
-    *default_icon = *action->default_icon();
-
-  const ExtensionIconSet& extension_icons = IconsInfo::GetIcons(&extension);
-  std::string largest_icon = extension_icons.Get(
-      extension_misc::EXTENSION_ICON_GIGANTOR,
-      ExtensionIconSet::MATCH_SMALLER);
-
-  if (!largest_icon.empty()) {
-    int largest_icon_size = extension_icons.GetIconSizeFromPath(largest_icon);
-    // Replace any missing extension action icons with the largest icon
-    // retrieved from |extension|'s manifest so long as the largest icon is
-    // larger than the current key.
-    for (int i = extension_misc::kNumExtensionActionIconSizes - 1;
-         i >= 0; --i) {
-      int size = extension_misc::kExtensionActionIconSizes[i].size;
-      if (default_icon->Get(size, ExtensionIconSet::MATCH_BIGGER).empty()
-          && largest_icon_size > size) {
-        default_icon->Add(size, largest_icon);
-        break;
-      }
-    }
-    action->set_default_icon(default_icon.Pass());
-  }
-}
-
 // Returns map[extension_id] if that entry exists. Otherwise, if
 // action_info!=NULL, creates an ExtensionAction from it, fills in the map, and
 // returns that.  Otherwise (action_info==NULL), returns NULL.
@@ -145,9 +109,8 @@ ExtensionAction* GetOrCreateOrNull(
   }
 
   linked_ptr<ExtensionAction> action(new ExtensionAction(
-      extension.id(), action_type, *action_info));
+      extension, action_type, *action_info));
   (*map)[extension.id()] = action;
-  PopulateMissingValues(extension, action.get());
   return action.get();
 }
 
@@ -181,8 +144,7 @@ scoped_ptr<ExtensionAction> ExtensionActionManager::GetBestFitAction(
   // with a blank ActionInfo.
   // Populate any missing values from |extension|'s manifest.
   scoped_ptr<ExtensionAction> new_action(new ExtensionAction(
-      extension.id(), type, info ? *info : ActionInfo()));
-  PopulateMissingValues(extension, new_action.get());
+      extension, type, info ? *info : ActionInfo()));
   return new_action.Pass();
 }
 

chrome/browser/extensions/extension_action_unittest.cc

@@ -5,114 +5,128 @@
 #include "base/message_loop/message_loop.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
+#include "extensions/common/test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
+namespace extensions {
+
 namespace {
 
-using extensions::ActionInfo;
+scoped_ptr<ExtensionAction> CreateAction(ActionInfo::Type type,
+                                         const ActionInfo& action_info) {
+  scoped_refptr<const Extension> extension = test_util::CreateEmptyExtension();
+  return make_scoped_ptr(new ExtensionAction(*extension, type, action_info));
+}
+
+}  // namespace
 
 TEST(ExtensionActionTest, Title) {
   ActionInfo action_info;
   action_info.default_title = "Initial Title";
-  ExtensionAction action(std::string(), ActionInfo::TYPE_PAGE, action_info);
-
-  ASSERT_EQ("Initial Title", action.GetTitle(1));
-  action.SetTitle(ExtensionAction::kDefaultTabId, "foo");
-  ASSERT_EQ("foo", action.GetTitle(1));
-  ASSERT_EQ("foo", action.GetTitle(100));
-  action.SetTitle(100, "bar");
-  ASSERT_EQ("foo", action.GetTitle(1));
-  ASSERT_EQ("bar", action.GetTitle(100));
-  action.SetTitle(ExtensionAction::kDefaultTabId, "baz");
-  ASSERT_EQ("baz", action.GetTitle(1));
-  action.ClearAllValuesForTab(100);
-  ASSERT_EQ("baz", action.GetTitle(100));
+  scoped_ptr<ExtensionAction> action =
+      CreateAction(ActionInfo::TYPE_PAGE, action_info);
+
+  ASSERT_EQ("Initial Title", action->GetTitle(1));
+  action->SetTitle(ExtensionAction::kDefaultTabId, "foo");
+  ASSERT_EQ("foo", action->GetTitle(1));
+  ASSERT_EQ("foo", action->GetTitle(100));
+  action->SetTitle(100, "bar");
+  ASSERT_EQ("foo", action->GetTitle(1));
+  ASSERT_EQ("bar", action->GetTitle(100));
+  action->SetTitle(ExtensionAction::kDefaultTabId, "baz");
+  ASSERT_EQ("baz", action->GetTitle(1));
+  action->ClearAllValuesForTab(100);
+  ASSERT_EQ("baz", action->GetTitle(100));
 }
 
 TEST(ExtensionActionTest, Visibility) {
-  ExtensionAction action(std::string(), ActionInfo::TYPE_PAGE, ActionInfo());
-
-  ASSERT_FALSE(action.GetIsVisible(1));
-  action.SetIsVisible(ExtensionAction::kDefaultTabId, true);
-  ASSERT_TRUE(action.GetIsVisible(1));
-  ASSERT_TRUE(action.GetIsVisible(100));
-
-  action.SetIsVisible(ExtensionAction::kDefaultTabId, false);
-  ASSERT_FALSE(action.GetIsVisible(1));
-  ASSERT_FALSE(action.GetIsVisible(100));
-  action.SetIsVisible(100, true);
-  ASSERT_FALSE(action.GetIsVisible(1));
-  ASSERT_TRUE(action.GetIsVisible(100));
-
-  action.ClearAllValuesForTab(100);
-  ASSERT_FALSE(action.GetIsVisible(1));
-  ASSERT_FALSE(action.GetIsVisible(100));
-
-  ExtensionAction browser_action(
-      std::string(), ActionInfo::TYPE_BROWSER, ActionInfo());
-  ASSERT_TRUE(browser_action.GetIsVisible(1));
+  scoped_ptr<ExtensionAction> action =
+      CreateAction(ActionInfo::TYPE_PAGE, ActionInfo());
+
+  ASSERT_FALSE(action->GetIsVisible(1));
+  action->SetIsVisible(ExtensionAction::kDefaultTabId, true);
+  ASSERT_TRUE(action->GetIsVisible(1));
+  ASSERT_TRUE(action->GetIsVisible(100));
+
+  action->SetIsVisible(ExtensionAction::kDefaultTabId, false);
+  ASSERT_FALSE(action->GetIsVisible(1));
+  ASSERT_FALSE(action->GetIsVisible(100));
+  action->SetIsVisible(100, true);
+  ASSERT_FALSE(action->GetIsVisible(1));
+  ASSERT_TRUE(action->GetIsVisible(100));
+
+  action->ClearAllValuesForTab(100);
+  ASSERT_FALSE(action->GetIsVisible(1));
+  ASSERT_FALSE(action->GetIsVisible(100));
+
+  scoped_ptr<ExtensionAction> browser_action =
+      CreateAction(ActionInfo::TYPE_BROWSER, ActionInfo());
+  ASSERT_TRUE(browser_action->GetIsVisible(1));
 }
 
 TEST(ExtensionActionTest, Icon) {
   ActionInfo action_info;
   action_info.default_icon.Add(16, "icon16.png");
-  ExtensionAction page_action(
-      std::string(), ActionInfo::TYPE_PAGE, action_info);
-  ASSERT_TRUE(page_action.default_icon());
+  scoped_ptr<ExtensionAction> page_action =
+      CreateAction(ActionInfo::TYPE_PAGE, action_info);
+  ASSERT_TRUE(page_action->default_icon());
   EXPECT_EQ("icon16.png",
-            page_action.default_icon()->Get(
+            page_action->default_icon()->Get(
                 16, ExtensionIconSet::MATCH_EXACTLY));
   EXPECT_EQ("",
-            page_action.default_icon()->Get(
+            page_action->default_icon()->Get(
                 17, ExtensionIconSet::MATCH_BIGGER));
 }
 
 TEST(ExtensionActionTest, Badge) {
-  ExtensionAction action(std::string(), ActionInfo::TYPE_PAGE, ActionInfo());
-  ASSERT_EQ("", action.GetBadgeText(1));
-  action.SetBadgeText(ExtensionAction::kDefaultTabId, "foo");
-  ASSERT_EQ("foo", action.GetBadgeText(1));
-  ASSERT_EQ("foo", action.GetBadgeText(100));
-  action.SetBadgeText(100, "bar");
-  ASSERT_EQ("foo", action.GetBadgeText(1));
-  ASSERT_EQ("bar", action.GetBadgeText(100));
-  action.SetBadgeText(ExtensionAction::kDefaultTabId, "baz");
-  ASSERT_EQ("baz", action.GetBadgeText(1));
-  action.ClearAllValuesForTab(100);
-  ASSERT_EQ("baz", action.GetBadgeText(100));
+  scoped_ptr<ExtensionAction> action =
+      CreateAction(ActionInfo::TYPE_PAGE, ActionInfo());
+  ASSERT_EQ("", action->GetBadgeText(1));
+  action->SetBadgeText(ExtensionAction::kDefaultTabId, "foo");
+  ASSERT_EQ("foo", action->GetBadgeText(1));
+  ASSERT_EQ("foo", action->GetBadgeText(100));
+  action->SetBadgeText(100, "bar");
+  ASSERT_EQ("foo", action->GetBadgeText(1));
+  ASSERT_EQ("bar", action->GetBadgeText(100));
+  action->SetBadgeText(ExtensionAction::kDefaultTabId, "baz");
+  ASSERT_EQ("baz", action->GetBadgeText(1));
+  action->ClearAllValuesForTab(100);
+  ASSERT_EQ("baz", action->GetBadgeText(100));
 }
 
 TEST(ExtensionActionTest, BadgeTextColor) {
-  ExtensionAction action(std::string(), ActionInfo::TYPE_PAGE, ActionInfo());
-  ASSERT_EQ(0x00000000u, action.GetBadgeTextColor(1));
-  action.SetBadgeTextColor(ExtensionAction::kDefaultTabId, 0xFFFF0000u);
-  ASSERT_EQ(0xFFFF0000u, action.GetBadgeTextColor(1));
-  ASSERT_EQ(0xFFFF0000u, action.GetBadgeTextColor(100));
-  action.SetBadgeTextColor(100, 0xFF00FF00);
-  ASSERT_EQ(0xFFFF0000u, action.GetBadgeTextColor(1));
-  ASSERT_EQ(0xFF00FF00u, action.GetBadgeTextColor(100));
-  action.SetBadgeTextColor(ExtensionAction::kDefaultTabId, 0xFF0000FFu);
-  ASSERT_EQ(0xFF0000FFu, action.GetBadgeTextColor(1));
-  action.ClearAllValuesForTab(100);
-  ASSERT_EQ(0xFF0000FFu, action.GetBadgeTextColor(100));
+  scoped_ptr<ExtensionAction> action =
+      CreateAction(ActionInfo::TYPE_PAGE, ActionInfo());
+  ASSERT_EQ(0x00000000u, action->GetBadgeTextColor(1));
+  action->SetBadgeTextColor(ExtensionAction::kDefaultTabId, 0xFFFF0000u);
+  ASSERT_EQ(0xFFFF0000u, action->GetBadgeTextColor(1));
+  ASSERT_EQ(0xFFFF0000u, action->GetBadgeTextColor(100));
+  action->SetBadgeTextColor(100, 0xFF00FF00);
+  ASSERT_EQ(0xFFFF0000u, action->GetBadgeTextColor(1));
+  ASSERT_EQ(0xFF00FF00u, action->GetBadgeTextColor(100));
+  action->SetBadgeTextColor(ExtensionAction::kDefaultTabId, 0xFF0000FFu);
+  ASSERT_EQ(0xFF0000FFu, action->GetBadgeTextColor(1));
+  action->ClearAllValuesForTab(100);
+  ASSERT_EQ(0xFF0000FFu, action->GetBadgeTextColor(100));
 }
 
 TEST(ExtensionActionTest, BadgeBackgroundColor) {
-  ExtensionAction action(std::string(), ActionInfo::TYPE_PAGE, ActionInfo());
-  ASSERT_EQ(0x00000000u, action.GetBadgeBackgroundColor(1));
-  action.SetBadgeBackgroundColor(ExtensionAction::kDefaultTabId,
+  scoped_ptr<ExtensionAction> action =
+      CreateAction(ActionInfo::TYPE_PAGE, ActionInfo());
+  ASSERT_EQ(0x00000000u, action->GetBadgeBackgroundColor(1));
+  action->SetBadgeBackgroundColor(ExtensionAction::kDefaultTabId,
                                  0xFFFF0000u);
-  ASSERT_EQ(0xFFFF0000u, action.GetBadgeBackgroundColor(1));
-  ASSERT_EQ(0xFFFF0000u, action.GetBadgeBackgroundColor(100));
-  action.SetBadgeBackgroundColor(100, 0xFF00FF00);
-  ASSERT_EQ(0xFFFF0000u, action.GetBadgeBackgroundColor(1));
-  ASSERT_EQ(0xFF00FF00u, action.GetBadgeBackgroundColor(100));
-  action.SetBadgeBackgroundColor(ExtensionAction::kDefaultTabId,
+  ASSERT_EQ(0xFFFF0000u, action->GetBadgeBackgroundColor(1));
+  ASSERT_EQ(0xFFFF0000u, action->GetBadgeBackgroundColor(100));
+  action->SetBadgeBackgroundColor(100, 0xFF00FF00);
+  ASSERT_EQ(0xFFFF0000u, action->GetBadgeBackgroundColor(1));
+  ASSERT_EQ(0xFF00FF00u, action->GetBadgeBackgroundColor(100));
+  action->SetBadgeBackgroundColor(ExtensionAction::kDefaultTabId,
                                  0xFF0000FFu);
-  ASSERT_EQ(0xFF0000FFu, action.GetBadgeBackgroundColor(1));
-  action.ClearAllValuesForTab(100);
-  ASSERT_EQ(0xFF0000FFu, action.GetBadgeBackgroundColor(100));
+  ASSERT_EQ(0xFF0000FFu, action->GetBadgeBackgroundColor(1));
+  action->ClearAllValuesForTab(100);
+  ASSERT_EQ(0xFF0000FFu, action->GetBadgeBackgroundColor(100));
 }
 
 TEST(ExtensionActionTest, PopupUrl) {
@@ -123,30 +137,31 @@ TEST(ExtensionActionTest, PopupUrl) {
 
   ActionInfo action_info;
   action_info.default_popup_url = url_foo;
-  ExtensionAction action(std::string(), ActionInfo::TYPE_PAGE, action_info);
-
-  ASSERT_EQ(url_foo, action.GetPopupUrl(1));
-  ASSERT_EQ(url_foo, action.GetPopupUrl(100));
-  ASSERT_TRUE(action.HasPopup(1));
-  ASSERT_TRUE(action.HasPopup(100));
-
-  action.SetPopupUrl(ExtensionAction::kDefaultTabId, url_unset);
-  ASSERT_EQ(url_unset, action.GetPopupUrl(1));
-  ASSERT_EQ(url_unset, action.GetPopupUrl(100));
-  ASSERT_FALSE(action.HasPopup(1));
-  ASSERT_FALSE(action.HasPopup(100));
-
-  action.SetPopupUrl(100, url_bar);
-  ASSERT_EQ(url_unset, action.GetPopupUrl(1));
-  ASSERT_EQ(url_bar, action.GetPopupUrl(100));
-
-  action.SetPopupUrl(ExtensionAction::kDefaultTabId, url_baz);
-  ASSERT_EQ(url_baz, action.GetPopupUrl(1));
-  ASSERT_EQ(url_bar, action.GetPopupUrl(100));
-
-  action.ClearAllValuesForTab(100);
-  ASSERT_EQ(url_baz, action.GetPopupUrl(1));
-  ASSERT_EQ(url_baz, action.GetPopupUrl(100));
+  scoped_ptr<ExtensionAction> action =
+      CreateAction(ActionInfo::TYPE_PAGE, action_info);
+
+  ASSERT_EQ(url_foo, action->GetPopupUrl(1));
+  ASSERT_EQ(url_foo, action->GetPopupUrl(100));
+  ASSERT_TRUE(action->HasPopup(1));
+  ASSERT_TRUE(action->HasPopup(100));
+
+  action->SetPopupUrl(ExtensionAction::kDefaultTabId, url_unset);
+  ASSERT_EQ(url_unset, action->GetPopupUrl(1));
+  ASSERT_EQ(url_unset, action->GetPopupUrl(100));
+  ASSERT_FALSE(action->HasPopup(1));
+  ASSERT_FALSE(action->HasPopup(100));
+
+  action->SetPopupUrl(100, url_bar);
+  ASSERT_EQ(url_unset, action->GetPopupUrl(1));
+  ASSERT_EQ(url_bar, action->GetPopupUrl(100));
+
+  action->SetPopupUrl(ExtensionAction::kDefaultTabId, url_baz);
+  ASSERT_EQ(url_baz, action->GetPopupUrl(1));
+  ASSERT_EQ(url_bar, action->GetPopupUrl(100));
+
+  action->ClearAllValuesForTab(100);
+  ASSERT_EQ(url_baz, action->GetPopupUrl(1));
+  ASSERT_EQ(url_baz, action->GetPopupUrl(100));
 }
 
-}  // namespace
+}  // namespace extensions

extensions/browser/extension_icon_image.cc

@@ -134,13 +134,14 @@ IconImage::IconImage(
       extension_(extension),
       icon_set_(icon_set),
       resource_size_in_dip_(resource_size_in_dip),
-      observer_(observer),
       source_(NULL),
       default_icon_(gfx::ImageSkiaOperations::CreateResizedImage(
           default_icon,
           skia::ImageOperations::RESIZE_BEST,
           gfx::Size(resource_size_in_dip, resource_size_in_dip))),
       weak_ptr_factory_(this) {
+  if (observer)
+    AddObserver(observer);
   gfx::Size resource_size(resource_size_in_dip, resource_size_in_dip);
   source_ = new Source(this, resource_size);
   image_skia_ = gfx::ImageSkia(source_, resource_size);
@@ -150,7 +151,16 @@ IconImage::IconImage(
                  content::NotificationService::AllSources());
 }
 
+void IconImage::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void IconImage::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
 IconImage::~IconImage() {
+  FOR_EACH_OBSERVER(Observer, observers_, OnExtensionIconImageDestroyed(this));
   source_->ResetHost();
 }
 
@@ -216,8 +226,7 @@ void IconImage::OnImageLoaded(float scale, const gfx::Image& image_in) {
   image_skia_.RemoveRepresentation(scale);
   image_skia_.AddRepresentation(rep);
 
-  if (observer_)
-    observer_->OnExtensionIconImageChanged(this);
+  FOR_EACH_OBSERVER(Observer, observers_, OnExtensionIconImageChanged(this));
 }
 
 void IconImage::Observe(int type,

extensions/browser/extension_icon_image.h

@@ -10,6 +10,7 @@
 
 #include "base/basictypes.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "extensions/common/extension_icon_set.h"
@@ -33,12 +34,11 @@ namespace extensions {
 
 // A class that provides an ImageSkia for UI code to use. It handles extension
 // icon resource loading, screen scale factor change etc. UI code that uses
-// extension icon should host this class and be its observer. ExtensionIconImage
-// should be outlived by the observer. In painting code, UI code paints with the
-// ImageSkia provided by this class. If required extension icon resource is not
-// already present, this class tries to load it and calls its observer interface
-// when the image get updated. Until the resource is loaded, the UI code will be
-// provided with blank, transparent image.
+// extension icon should host this class. In painting code, UI code paints with
+// the ImageSkia provided by this class. If the required extension icon resource
+// is not already present, this class tries to load it and calls its observer
+// interface when the image get updated. Until the resource is loaded, the UI
+// code will be provided with a blank, transparent image.
 // If the requested resource doesn't exist or can't be loaded and a default
 // icon was supplied in the constructor, icon image will be updated with the
 // default icon's resource.
@@ -56,6 +56,10 @@ class IconImage : public content::NotificationObserver {
     // is loaded and added to |image|.
     virtual void OnExtensionIconImageChanged(IconImage* image) = 0;
 
+    // Called when this object is deleted. Objects should observe this if there
+    // is a question about the lifetime of the icon image vs observer.
+    virtual void OnExtensionIconImageDestroyed(IconImage* image) {}
+
    protected:
     virtual ~Observer() {}
   };
@@ -73,6 +77,9 @@ class IconImage : public content::NotificationObserver {
 
   const gfx::ImageSkia& image_skia() const { return image_skia_; }
 
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
  private:
   class Source;
 
@@ -81,7 +88,7 @@ class IconImage : public content::NotificationObserver {
   // method.
   // If representation loading is asynchronous, an empty image
   // representation is returned. When the representation gets loaded the
-  // observer's |OnExtensionIconImageLoaded| will be called.
+  // observers' OnExtensionIconImageLoaded() will be called.
   gfx::ImageSkiaRep LoadImageForScaleFactor(ui::ScaleFactor scale_factor);
 
   void OnImageLoaded(float scale_factor, const gfx::Image& image);
@@ -96,7 +103,7 @@ class IconImage : public content::NotificationObserver {
   const ExtensionIconSet& icon_set_;
   const int resource_size_in_dip_;
 
-  Observer* observer_;
+  ObserverList<Observer> observers_;
 
   Source* source_;  // Owned by ImageSkia storage.
   gfx::ImageSkia image_skia_;