[Extensions] Make extension actions use gfx::Image over gfx::ImageSkia.

gfx::Images cache different representations (such as NSImages), whereas
gfx::ImageSkias (as a representation themselves) do not. Prefer to use
gfx::Image in most icon code, so that these representations are cached.

BUG=452971

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

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

chrome/browser/extensions/api/declarative_content/content_action_unittest.cc

@@ -23,7 +23,8 @@
 #include "ipc/ipc_message_utils.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#include "ui/gfx/image/image_skia.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "ui/gfx/image/image.h"
 #include "ui/gfx/ipc/gfx_param_traits.h"
 
 namespace extensions {
@@ -206,11 +207,11 @@ TEST(DeclarativeContentActionTest, SetIcon) {
   };
 
   // The declarative icon shouldn't exist unless the content action is applied.
-  EXPECT_TRUE(page_action->GetDeclarativeIcon(tab_id).bitmap()->empty());
+  EXPECT_TRUE(page_action->GetDeclarativeIcon(tab_id).IsEmpty());
   result->Apply(extension->id(), base::Time(), &apply_info);
-  EXPECT_FALSE(page_action->GetDeclarativeIcon(tab_id).bitmap()->empty());
+  EXPECT_FALSE(page_action->GetDeclarativeIcon(tab_id).IsEmpty());
   result->Revert(extension->id(), base::Time(), &apply_info);
-  EXPECT_TRUE(page_action->GetDeclarativeIcon(tab_id).bitmap()->empty());
+  EXPECT_TRUE(page_action->GetDeclarativeIcon(tab_id).IsEmpty());
 }
 
 TEST_F(RequestContentScriptTest, MissingScripts) {

chrome/browser/extensions/api/declarative_content/set_icon_apitest.cc

@@ -9,7 +9,7 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/extensions/features/feature_channel.h"
 #include "extensions/test/extension_test_message_listener.h"
-#include "ui/gfx/image/image_skia.h"
+#include "ui/gfx/image/image.h"
 
 namespace extensions {
 namespace {
@@ -84,13 +84,13 @@ IN_PROC_BROWSER_TEST_F(SetIconAPITest, Overview) {
   const int tab_id = ExtensionTabUtil::GetTabId(tab);
 
   // There should be no declarative icon until we navigate to a matched page.
-  EXPECT_TRUE(page_action->GetDeclarativeIcon(tab_id).bitmap()->empty());
+  EXPECT_TRUE(page_action->GetDeclarativeIcon(tab_id).IsEmpty());
   NavigateInRenderer(tab, GURL("http://test1/"));
-  EXPECT_FALSE(page_action->GetDeclarativeIcon(tab_id).bitmap()->empty());
+  EXPECT_FALSE(page_action->GetDeclarativeIcon(tab_id).IsEmpty());
 
   // Navigating to an unmatched page should reset the icon.
   NavigateInRenderer(tab, GURL("http://test2/"));
-  EXPECT_TRUE(page_action->GetDeclarativeIcon(tab_id).bitmap()->empty());
+  EXPECT_TRUE(page_action->GetDeclarativeIcon(tab_id).IsEmpty());
 }
 }  // namespace
 }  // namespace extensions

chrome/browser/extensions/extension_action.cc

@@ -132,7 +132,7 @@ GURL ExtensionAction::GetPopupUrl(int tab_id) const {
 }
 
 void ExtensionAction::SetIcon(int tab_id, const gfx::Image& image) {
-  SetValue(&icon_, tab_id, image.AsImageSkia());
+  SetValue(&icon_, tab_id, image);
 }
 
 bool ExtensionAction::ParseIconFromCanvasDictionary(
@@ -164,7 +164,7 @@ bool ExtensionAction::ParseIconFromCanvasDictionary(
   return true;
 }
 
-gfx::ImageSkia ExtensionAction::GetExplicitlySetIcon(int tab_id) const {
+gfx::Image ExtensionAction::GetExplicitlySetIcon(int tab_id) const {
   return GetValue(&icon_, tab_id);
 }
 
@@ -211,13 +211,12 @@ void ExtensionAction::UndoDeclarativeSetIcon(int tab_id,
   }
 }
 
-const gfx::ImageSkia ExtensionAction::GetDeclarativeIcon(int tab_id) const {
+const gfx::Image ExtensionAction::GetDeclarativeIcon(int tab_id) const {
   if (declarative_icon_.find(tab_id) != declarative_icon_.end() &&
       !declarative_icon_.find(tab_id)->second.rbegin()->second.empty()) {
-    return declarative_icon_.find(tab_id)->second.rbegin()
-        ->second.back().AsImageSkia();
+    return declarative_icon_.find(tab_id)->second.rbegin()->second.back();
   }
-  return gfx::ImageSkia();
+  return gfx::Image();
 }
 
 void ExtensionAction::ClearAllValuesForTab(int tab_id) {
@@ -280,11 +279,10 @@ extensions::IconImage* ExtensionAction::LoadDefaultIconImage(
   return default_icon_image_.get();
 }
 
-gfx::ImageSkia ExtensionAction::GetDefaultIconImage() const {
+gfx::Image 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();
+  return default_icon_image_ ? default_icon_image_->image() : GetDefaultIcon();
 }
 
 bool ExtensionAction::HasPopupUrl(int tab_id) const {
@@ -338,8 +336,7 @@ void ExtensionAction::Populate(const extensions::Extension& extension,
       extensions::IconsInfo::GetIcons(&extension);
   // Look for any other icons.
   std::string largest_icon = extension_icons.Get(
-      extension_misc::EXTENSION_ICON_GIGANTOR,
-      ExtensionIconSet::MATCH_SMALLER);
+      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.
@@ -349,11 +346,11 @@ void ExtensionAction::Populate(const extensions::Extension& extension,
     // 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) {
+    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) {
+      if (default_icon_->Get(size, ExtensionIconSet::MATCH_BIGGER).empty() &&
+          largest_icon_size > size) {
         default_icon_->Add(size, largest_icon);
         break;
       }
@@ -364,9 +361,9 @@ void ExtensionAction::Populate(const extensions::Extension& extension,
 // 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.
-  gfx::ImageSkia icon = GetValue(&icon_, tab_id);
-  if (!icon.isNull())
-    return icon.width();
+  gfx::Image icon = GetValue(&icon_, tab_id);
+  if (!icon.IsEmpty())
+    return icon.Width();
   // If there is a default icon, the icon width will be set depending on our
   // action type.
   if (default_icon_)

chrome/browser/extensions/extension_action.h

@@ -104,7 +104,7 @@ class ExtensionAction {
                                             gfx::ImageSkia* icon);
 
   // Gets the icon that has been set using |SetIcon| for the tab.
-  gfx::ImageSkia GetExplicitlySetIcon(int tab_id) const;
+  gfx::Image GetExplicitlySetIcon(int tab_id) const;
 
   // Sets the icon for a tab, in a way that can't be read by the extension's
   // javascript.  Multiple icons can be set at the same time; some icon with the
@@ -152,7 +152,7 @@ class ExtensionAction {
   // by an appearance set directly on the tab.
   void DeclarativeShow(int tab_id);
   void UndoDeclarativeShow(int tab_id);
-  const gfx::ImageSkia GetDeclarativeIcon(int tab_id) const;
+  const gfx::Image GetDeclarativeIcon(int tab_id) const;
 
   // Get the badge visibility for a tab, or the default badge visibility
   // if none was set.
@@ -195,7 +195,7 @@ class ExtensionAction {
 
   // Returns the image to use as the default icon for the action. Can only be
   // called after LoadDefaultIconImage().
-  gfx::ImageSkia GetDefaultIconImage() const;
+  gfx::Image GetDefaultIconImage() const;
 
   // Determine whether or not the ExtensionAction has a value set for the given
   // |tab_id| for each property.
@@ -263,7 +263,7 @@ class ExtensionAction {
   // kDefaultTabId), or tab-specific state (stored with the tab_id as the key).
   std::map<int, GURL> popup_url_;
   std::map<int, std::string> title_;
-  std::map<int, gfx::ImageSkia> icon_;
+  std::map<int, gfx::Image> icon_;
   std::map<int, std::string> badge_text_;
   std::map<int, SkColor> badge_background_color_;
   std::map<int, SkColor> badge_text_color_;

chrome/browser/extensions/extension_action_icon_factory.cc

@@ -41,16 +41,12 @@ void ExtensionActionIconFactory::OnExtensionIconImageDestroyed(
 }
 
 gfx::Image ExtensionActionIconFactory::GetIcon(int tab_id) {
-  return gfx::Image(GetBaseIconFromAction(tab_id));
-}
-
-gfx::ImageSkia ExtensionActionIconFactory::GetBaseIconFromAction(int tab_id) {
-  gfx::ImageSkia icon = action_->GetExplicitlySetIcon(tab_id);
-  if (!icon.isNull())
+  gfx::Image icon = action_->GetExplicitlySetIcon(tab_id);
+  if (!icon.IsEmpty())
     return icon;
 
   icon = action_->GetDeclarativeIcon(tab_id);
-  if (!icon.isNull())
+  if (!icon.IsEmpty())
     return icon;
 
   return action_->GetDefaultIconImage();

chrome/browser/extensions/extension_action_icon_factory.h

@@ -53,10 +53,6 @@ class ExtensionActionIconFactory : public extensions::IconImage::Observer {
   gfx::Image GetIcon(int tab_id);
 
  private:
-  // Gets the icon that should be returned by |GetIcon| (without the attention
-  // and animation transformations).
-  gfx::ImageSkia GetBaseIconFromAction(int tab_id);
-
   const extensions::Extension* extension_;
   const ExtensionAction* action_;
   Observer* observer_;

chrome/browser/extensions/extension_action_icon_factory_unittest.cc

@@ -190,7 +190,7 @@ TEST_F(ExtensionActionIconFactoryTest, NoIcons) {
   ExtensionAction* browser_action = GetBrowserAction(*extension.get());
   ASSERT_TRUE(browser_action);
   ASSERT_FALSE(browser_action->default_icon());
-  ASSERT_TRUE(browser_action->GetExplicitlySetIcon(0 /*tab id*/).isNull());
+  ASSERT_TRUE(browser_action->GetExplicitlySetIcon(0 /*tab id*/).IsEmpty());
 
   gfx::ImageSkia favicon = GetFavicon();
 
@@ -216,14 +216,14 @@ TEST_F(ExtensionActionIconFactoryTest, AfterSetIcon) {
   ExtensionAction* browser_action = GetBrowserAction(*extension.get());
   ASSERT_TRUE(browser_action);
   ASSERT_FALSE(browser_action->default_icon());
-  ASSERT_TRUE(browser_action->GetExplicitlySetIcon(0 /*tab id*/).isNull());
+  ASSERT_TRUE(browser_action->GetExplicitlySetIcon(0 /*tab id*/).IsEmpty());
 
   gfx::Image set_icon = LoadIcon("browser_action/no_icon/icon.png");
   ASSERT_FALSE(set_icon.IsEmpty());
 
   browser_action->SetIcon(0, set_icon);
 
-  ASSERT_FALSE(browser_action->GetExplicitlySetIcon(0 /*tab id*/).isNull());
+  ASSERT_FALSE(browser_action->GetExplicitlySetIcon(0 /*tab id*/).IsEmpty());
 
   ExtensionActionIconFactory icon_factory(
       profile(), extension.get(), browser_action, this);
@@ -254,7 +254,7 @@ TEST_F(ExtensionActionIconFactoryTest, DefaultIcon) {
   ExtensionAction* browser_action = GetBrowserAction(*extension.get());
   ASSERT_TRUE(browser_action);
   ASSERT_FALSE(browser_action->default_icon());
-  ASSERT_TRUE(browser_action->GetExplicitlySetIcon(0 /*tab id*/).isNull());
+  ASSERT_TRUE(browser_action->GetExplicitlySetIcon(0 /*tab id*/).IsEmpty());
 
   gfx::Image default_icon =
       EnsureImageSize(LoadIcon("browser_action/no_icon/icon.png"), 19);

chrome/browser/extensions/extension_action_storage_manager.cc

@@ -169,7 +169,8 @@ scoped_ptr<base::DictionaryValue> DefaultsToValue(ExtensionAction* action) {
   dict->SetInteger(kAppearanceStorageKey,
                    action->GetIsVisible(kDefaultTabId) ? ACTIVE : INVISIBLE);
 
-  gfx::ImageSkia icon = action->GetExplicitlySetIcon(kDefaultTabId);
+  gfx::ImageSkia icon =
+      action->GetExplicitlySetIcon(kDefaultTabId).AsImageSkia();
   if (!icon.isNull()) {
     scoped_ptr<base::DictionaryValue> icon_value(new base::DictionaryValue());
     for (size_t i = 0; i < extension_misc::kNumExtensionActionIconSizes; i++) {

extensions/browser/extension_icon_image.cc

@@ -145,6 +145,7 @@ IconImage::IconImage(
   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);
+  image_ = gfx::Image(image_skia_);
 
   registrar_.Add(this,
                  extensions::NOTIFICATION_EXTENSION_REMOVED,
@@ -226,6 +227,12 @@ void IconImage::OnImageLoaded(float scale, const gfx::Image& image_in) {
   image_skia_.RemoveRepresentation(scale);
   image_skia_.AddRepresentation(rep);
 
+  // Update the image to use the updated image skia.
+  // It's a shame we have to do this because it means that all the other
+  // representations stored on |image_| will be deleted, but unfortunately
+  // there's no way to combine the storage of two images.
+  image_ = gfx::Image(image_skia_);
+
   FOR_EACH_OBSERVER(Observer, observers_, OnExtensionIconImageChanged(this));
 }
 

extensions/browser/extension_icon_image.h

@@ -15,6 +15,7 @@
 #include "content/public/browser/notification_registrar.h"
 #include "extensions/common/extension_icon_set.h"
 #include "ui/base/layout.h"
+#include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_skia.h"
 
 namespace content {
@@ -75,6 +76,7 @@ class IconImage : public content::NotificationObserver {
             Observer* observer);
   ~IconImage() override;
 
+  gfx::Image image() const { return image_; }
   const gfx::ImageSkia& image_skia() const { return image_skia_; }
 
   void AddObserver(Observer* observer);
@@ -111,6 +113,10 @@ class IconImage : public content::NotificationObserver {
   // its own representation load fails.
   gfx::ImageSkia default_icon_;
 
+  // The image wrapper around |image_skia_|.
+  // Note: this is reset each time a new representation is loaded.
+  gfx::Image image_;
+
   content::NotificationRegistrar registrar_;
 
   base::WeakPtrFactory<IconImage> weak_ptr_factory_;

extensions/browser/extension_icon_image_unittest.cc

@@ -562,4 +562,44 @@ TEST_F(ExtensionIconImageTest, IconImageDestruction) {
   EXPECT_EQ(2.0f, representation.scale());
 }
 
+// Test that new representations added to the image of an IconImage are cached
+// for future use.
+TEST_F(ExtensionIconImageTest, ImageCachesNewRepresentations) {
+  // Load up an extension and create an icon image.
+  scoped_refptr<Extension> extension(
+      CreateExtension("extension_icon_image", Manifest::INVALID_LOCATION));
+  ASSERT_TRUE(extension.get() != NULL);
+  gfx::ImageSkia default_icon = GetDefaultIcon();
+  scoped_ptr<IconImage> icon_image(
+      new IconImage(browser_context(),
+                    extension.get(),
+                    IconsInfo::GetIcons(extension.get()),
+                    16,
+                    default_icon,
+                    this));
+
+  // Load an image representation.
+  gfx::ImageSkiaRep representation =
+      icon_image->image_skia().GetRepresentation(1.0f);
+  WaitForImageLoad();
+
+  // Cache for later use.
+  gfx::Image prior_image = icon_image->image();
+
+  // Now the fun part: access the image from the IconImage, and create a png
+  // representation of it.
+  gfx::Image image = icon_image->image();
+  scoped_refptr<base::RefCountedMemory> image_as_png = image.As1xPNGBytes();
+
+  // Access the image from the IconImage again, and get a png representation.
+  // The two png representations should be exactly equal (the same object),
+  // because image storage is shared, so when we created one from the first
+  // image, all other images should also have that representation...
+  gfx::Image image2 = icon_image->image();
+  EXPECT_EQ(image_as_png.get(), image2.As1xPNGBytes().get());
+
+  // ...even images that were copied before the representation was constructed.
+  EXPECT_EQ(image_as_png.get(), prior_image.As1xPNGBytes().get());
+}
+
 }  // namespace extensions