Fix the dynamic browser action setIcon path to work with any size icon.

chrome/browser/extensions/api/extension_action/browser_action_apitest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stdint.h>
 
 #include "base/macros.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
 #include "chrome/browser/extensions/browser_action_test_util.h"
@@ skipping 23 matching lines @@
 #include "extensions/test/result_catcher.h"
 #include "grit/theme_resources.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/image/canvas_image_source.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_skia_operations.h"
+#include "ui/gfx/image/image_unittest_util.h"
 #include "ui/gfx/skia_util.h"
 
 using content::WebContents;
 
 namespace extensions {
 namespace {
 
 // An ImageSkia source that will do nothing (i.e., have a blank skia). We need
 // this because we need a blank canvas at a certain size, and that can't be done
 // by just using a null ImageSkia.
 class BlankImageSource : public gfx::CanvasImageSource {
  public:
   explicit BlankImageSource(const gfx::Size& size)
      : gfx::CanvasImageSource(size, false) {}
   ~BlankImageSource() override {}
 
   void Draw(gfx::Canvas* canvas) override {}
 
  private:
   DISALLOW_COPY_AND_ASSIGN(BlankImageSource);
 };
 
 const char kEmptyImageDataError[] =
     "The imageData property must contain an ImageData object or dictionary "
     "of ImageData objects.";
 const char kEmptyPathError[] = "The path property must not be empty.";
 
-// Views platforms have the icon superimposed over a button's background.
-// Macs don't, but still need a 29x29-sized image (and the easiest way to do
-// that is to superimpose the icon over a blank background).
-gfx::ImageSkia AddBackground(const gfx::ImageSkia& icon) {
-#if !defined(OS_MACOSX)
-  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-  gfx::ImageSkia bg = *rb.GetImageSkiaNamed(IDR_BROWSER_ACTION);
-#else
-  const gfx::Size size(29, 29);  // Size of browser actions buttons.
-  gfx::ImageSkia bg(new BlankImageSource(size), size);
-#endif
-  return gfx::ImageSkiaOperations::CreateSuperimposedImage(bg, icon);
-}
-
-bool ImagesAreEqualAtScale(const gfx::ImageSkia& i1,
-                           const gfx::ImageSkia& i2,
-                           float scale) {
-  SkBitmap bitmap1 = i1.GetRepresentation(scale).sk_bitmap();
-  SkBitmap bitmap2 = i2.GetRepresentation(scale).sk_bitmap();
-  return gfx::BitmapsAreEqual(bitmap1, bitmap2);
-}
-
 class BrowserActionApiTest : public ExtensionApiTest {
  public:
   BrowserActionApiTest() {}
   ~BrowserActionApiTest() override {}
 
  protected:
   BrowserActionTestUtil* GetBrowserActionsBar() {
     if (!browser_action_test_util_)
       browser_action_test_util_.reset(new BrowserActionTestUtil(browser()));
     return browser_action_test_util_.get();
@@ skipping 78 matching lines @@
   ASSERT_EQ(1, GetBrowserActionsBar()->NumberOfBrowserActions());
   EXPECT_TRUE(GetBrowserActionsBar()->HasIcon(0));
 
   gfx::Image action_icon = icon_factory.GetIcon(0);
   uint32_t action_icon_last_id = action_icon.ToSkBitmap()->getGenerationID();
 
   // Let's check that |GetIcon| doesn't always return bitmap with new id.
   ASSERT_EQ(action_icon_last_id,
             icon_factory.GetIcon(0).ToSkBitmap()->getGenerationID());
 
-  uint32_t action_icon_current_id = 0;
+  gfx::Image last_bar_icon = GetBrowserActionsBar()->GetIcon(0);
+  EXPECT_TRUE(gfx::test::AreImagesEqual(last_bar_icon,
+                                        GetBrowserActionsBar()->GetIcon(0)));
 
-  ResultCatcher catcher;
+  // The reason we don't test more standard scales (like 1x, 2x, etc.) is that
+  // these may be generated from the provided scales.
+  float kSmallIconScale = 21.f / ExtensionAction::ActionIconSize();
+  float kLargeIconScale = 42.f / ExtensionAction::ActionIconSize();
+  ASSERT_FALSE(ui::IsSupportedScale(kSmallIconScale));
+  ASSERT_FALSE(ui::IsSupportedScale(kLargeIconScale));
 
   // Tell the extension to update the icon using ImageData object.
+  ResultCatcher catcher;
   GetBrowserActionsBar()->Press(0);
   ASSERT_TRUE(catcher.GetNextResult());
 
+  EXPECT_FALSE(gfx::test::AreImagesEqual(last_bar_icon,
+                                         GetBrowserActionsBar()->GetIcon(0)));
+  last_bar_icon = GetBrowserActionsBar()->GetIcon(0);
+
   action_icon = icon_factory.GetIcon(0);
-
-  action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
+  uint32_t action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
   EXPECT_GT(action_icon_current_id, action_icon_last_id);
   action_icon_last_id = action_icon_current_id;
 
-  EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(2.0f));
-
-  EXPECT_TRUE(
-      ImagesAreEqualAtScale(AddBackground(*action_icon.ToImageSkia()),
-                            *GetBrowserActionsBar()->GetIcon(0).ToImageSkia(),
-                            1.0f));
+  EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale));
+  EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(kLargeIconScale));
 
   // Tell the extension to update the icon using path.
   GetBrowserActionsBar()->Press(0);
   ASSERT_TRUE(catcher.GetNextResult());
 
+  // Make sure the browser action bar updated.
+  EXPECT_FALSE(gfx::test::AreImagesEqual(last_bar_icon,
+                                         GetBrowserActionsBar()->GetIcon(0)));
+  last_bar_icon = GetBrowserActionsBar()->GetIcon(0);
+
+

[Devlin, 2016/01/19 22:44:40]

extra newline

[Evan Stade, 2016/01/20 23:04:36]

Done.

   action_icon = icon_factory.GetIcon(0);
-
   action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
   EXPECT_GT(action_icon_current_id, action_icon_last_id);
   action_icon_last_id = action_icon_current_id;
 
-  EXPECT_FALSE(
-      action_icon.ToImageSkia()->HasRepresentation(2.0f));
-
-  EXPECT_TRUE(
-      ImagesAreEqualAtScale(AddBackground(*action_icon.ToImageSkia()),
-                            *GetBrowserActionsBar()->GetIcon(0).ToImageSkia(),
-                            1.0f));
+  EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale));

[Devlin, 2016/01/19 22:44:40]

nit: please comment above these why we expect/don't expect a certain
representation.

[Evan Stade, 2016/01/20 23:04:36]

done, also see comment above the kSmallIconScale definition

+  EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(kLargeIconScale));
 
   // Tell the extension to update the icon using dictionary of ImageData
   // objects.
   GetBrowserActionsBar()->Press(0);
   ASSERT_TRUE(catcher.GetNextResult());
 
+  EXPECT_FALSE(gfx::test::AreImagesEqual(last_bar_icon,
+                                         GetBrowserActionsBar()->GetIcon(0)));
+  last_bar_icon = GetBrowserActionsBar()->GetIcon(0);
+
   action_icon = icon_factory.GetIcon(0);
-
   action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
   EXPECT_GT(action_icon_current_id, action_icon_last_id);
   action_icon_last_id = action_icon_current_id;
 
-  EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(2.0f));
-
-  EXPECT_TRUE(
-      ImagesAreEqualAtScale(AddBackground(*action_icon.ToImageSkia()),
-                            *GetBrowserActionsBar()->GetIcon(0).ToImageSkia(),
-                            1.0f));
+  EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale));
+  EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kLargeIconScale));
 
   // Tell the extension to update the icon using dictionary of paths.
   GetBrowserActionsBar()->Press(0);
   ASSERT_TRUE(catcher.GetNextResult());
 
+  EXPECT_FALSE(gfx::test::AreImagesEqual(last_bar_icon,
+                                         GetBrowserActionsBar()->GetIcon(0)));
+  last_bar_icon = GetBrowserActionsBar()->GetIcon(0);
+
   action_icon = icon_factory.GetIcon(0);
-
   action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
   EXPECT_GT(action_icon_current_id, action_icon_last_id);
   action_icon_last_id = action_icon_current_id;
 
-  EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(2.0f));
-
-  EXPECT_TRUE(
-      ImagesAreEqualAtScale(AddBackground(*action_icon.ToImageSkia()),
-                            *GetBrowserActionsBar()->GetIcon(0).ToImageSkia(),
-                            1.0f));
+  EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale));
+  EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kLargeIconScale));
 
   // Tell the extension to update the icon using dictionary of ImageData
   // objects, but setting only size 19.
   GetBrowserActionsBar()->Press(0);
   ASSERT_TRUE(catcher.GetNextResult());
 
+  EXPECT_FALSE(gfx::test::AreImagesEqual(last_bar_icon,
+                                         GetBrowserActionsBar()->GetIcon(0)));
+  last_bar_icon = GetBrowserActionsBar()->GetIcon(0);
+
   action_icon = icon_factory.GetIcon(0);
-
   action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
   EXPECT_GT(action_icon_current_id, action_icon_last_id);
   action_icon_last_id = action_icon_current_id;
 
-  EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(2.0f));
-
-  EXPECT_TRUE(
-      ImagesAreEqualAtScale(AddBackground(*action_icon.ToImageSkia()),
-                            *GetBrowserActionsBar()->GetIcon(0).ToImageSkia(),
-                            1.0f));
+  EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale));
+  EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(kLargeIconScale));
 
   // Tell the extension to update the icon using dictionary of paths, but
   // setting only size 19.

[Devlin, 2016/01/19 22:44:40]

These comments (also on line 250) need to be updated, right?

[Evan Stade, 2016/01/20 23:04:36]

Done.

   GetBrowserActionsBar()->Press(0);
   ASSERT_TRUE(catcher.GetNextResult());
 
+  EXPECT_FALSE(gfx::test::AreImagesEqual(last_bar_icon,
+                                         GetBrowserActionsBar()->GetIcon(0)));
+  last_bar_icon = GetBrowserActionsBar()->GetIcon(0);
+
   action_icon = icon_factory.GetIcon(0);
-
   action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
   EXPECT_GT(action_icon_current_id, action_icon_last_id);
   action_icon_last_id = action_icon_current_id;
 
-  EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(2.0f));
-
-  EXPECT_TRUE(
-      ImagesAreEqualAtScale(AddBackground(*action_icon.ToImageSkia()),
-                            *GetBrowserActionsBar()->GetIcon(0).ToImageSkia(),
-                            1.0f));
+  EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale));
+  EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(kLargeIconScale));
 
   // Tell the extension to update the icon using dictionary of ImageData
-  // objects, but setting only size 38.
+  // objects, but setting only size 42.
   GetBrowserActionsBar()->Press(0);
   ASSERT_TRUE(catcher.GetNextResult());
 
+  EXPECT_FALSE(gfx::test::AreImagesEqual(last_bar_icon,
+                                         GetBrowserActionsBar()->GetIcon(0)));
+  last_bar_icon = GetBrowserActionsBar()->GetIcon(0);
+
   action_icon = icon_factory.GetIcon(0);
-
-  const gfx::ImageSkia* action_icon_skia = action_icon.ToImageSkia();
-
-  EXPECT_FALSE(action_icon_skia->HasRepresentation(1.0f));
-  EXPECT_TRUE(action_icon_skia->HasRepresentation(2.0f));
-
   action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
   EXPECT_GT(action_icon_current_id, action_icon_last_id);
   action_icon_last_id = action_icon_current_id;
 
-  EXPECT_TRUE(gfx::BitmapsAreEqual(
-      *action_icon.ToSkBitmap(),
-      action_icon_skia->GetRepresentation(2.0f).sk_bitmap()));
-
-  EXPECT_TRUE(
-      ImagesAreEqualAtScale(AddBackground(*action_icon_skia),
-                            *GetBrowserActionsBar()->GetIcon(0).ToImageSkia(),
-                            2.0f));
+  EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale));
+  EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kLargeIconScale));
 
   // Try setting icon with empty dictionary of ImageData objects.
   GetBrowserActionsBar()->Press(0);
   ASSERT_FALSE(catcher.GetNextResult());
   EXPECT_EQ(kEmptyImageDataError, catcher.message());
 
   // Try setting icon with empty dictionary of path objects.
   GetBrowserActionsBar()->Press(0);
   ASSERT_FALSE(catcher.GetNextResult());
   EXPECT_EQ(kEmptyPathError, catcher.message());
@@ skipping 395 matching lines @@
   std::string result;
   EXPECT_TRUE(
       content::ExecuteScriptAndExtractString(frame_host, script, &result));
   EXPECT_EQ("DONE", result);
 
   EXPECT_TRUE(actions_bar->HidePopup());
 }
 
 }  // namespace
 }  // namespace extensions