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

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

Index: chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
diff --git a/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc b/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
index 4426fa7b827fd06aea84f80f478f5677e12452e5..feff9b3fdc48b4d145b725dd3ca1d7a56967ae25 100644
--- a/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
+++ b/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
@@ -68,28 +68,6 @@ const char kEmptyImageDataError[] =
     "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() {}
@@ -188,140 +166,140 @@ IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, DynamicBrowserAction) {
   ASSERT_EQ(action_icon_last_id,
             icon_factory.GetIcon(0).ToSkBitmap()->getGenerationID());
 
-  uint32_t action_icon_current_id = 0;
+  gfx::ImageSkia last_bar_icon =
+      GetBrowserActionsBar()->GetIcon(0).AsImageSkia();
+  EXPECT_TRUE(last_bar_icon.BackedBySameObjectAs(
+      GetBrowserActionsBar()->GetIcon(0).AsImageSkia()));
 
-  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());
 
   action_icon = icon_factory.GetIcon(0);
+  EXPECT_FALSE(last_bar_icon.BackedBySameObjectAs(
+      GetBrowserActionsBar()->GetIcon(0).AsImageSkia()));
+  last_bar_icon = GetBrowserActionsBar()->GetIcon(0).AsImageSkia();
 
-  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());
 
-  action_icon = icon_factory.GetIcon(0);
+  // Make sure the browser action bar updated.

[Evan Stade, 2016/01/13 18:35:41]

I couldn't come up with a good way to keep comparing actual pixels but I think
we can maintain the spirit of the test without doing so.

[Devlin, 2016/01/13 19:58:12]

What's stopping us from comparing pixels/bitmaps/etc still?

[Evan Stade, 2016/01/14 01:59:19]

We have to reconstruct the scaling that I recently added to
icon_with_badge_image_source.cc (in addition to the AddBackground mumbo jumbo),
but I couldn't get it to work and it's impossible to debug so after some time I
gave up. I don't think it's a good test when debugging is impossible.

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

It seems that what we should be checking is that the relationship between
icon_factory.GetIcon(0) and BrowserActionsBar()->GetIcon(0) exists, and, since
the scaling is just adding skia reps, I would have thought there'd be a good way
to somehow compare the shared Image object - but it seems it's rather buried. 
But this makes me nervous, because this is probably our only test for dynamic
browser actions.

If there's no way to suitably compare these at scaling, can you add a second
simple test that just tests the dynamic icon is used (with no scaling, so we can
do comparison)?

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

I think I found a way to salvage image comparisons. We'll see if mac likes it. I
can't even really explain why this works and other attempts failed because
again, debugging image comparisons is hard.

+  EXPECT_FALSE(last_bar_icon.BackedBySameObjectAs(
+      GetBrowserActionsBar()->GetIcon(0).AsImageSkia()));
+  last_bar_icon = GetBrowserActionsBar()->GetIcon(0).AsImageSkia();
 
+  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.
   GetBrowserActionsBar()->Press(0);
   ASSERT_TRUE(catcher.GetNextResult());
 
-  action_icon = icon_factory.GetIcon(0);
+  EXPECT_FALSE(last_bar_icon.BackedBySameObjectAs(
+      GetBrowserActionsBar()->GetIcon(0).AsImageSkia()));
+  last_bar_icon = GetBrowserActionsBar()->GetIcon(0).AsImageSkia();
 
+  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());
 
-  action_icon = icon_factory.GetIcon(0);
+  EXPECT_FALSE(last_bar_icon.BackedBySameObjectAs(
+      GetBrowserActionsBar()->GetIcon(0).AsImageSkia()));
+  last_bar_icon = GetBrowserActionsBar()->GetIcon(0).AsImageSkia();
 
+  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());
 
-  action_icon = icon_factory.GetIcon(0);
+  EXPECT_FALSE(last_bar_icon.BackedBySameObjectAs(
+      GetBrowserActionsBar()->GetIcon(0).AsImageSkia()));
+  last_bar_icon = GetBrowserActionsBar()->GetIcon(0).AsImageSkia();
 
+  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.
   GetBrowserActionsBar()->Press(0);
   ASSERT_TRUE(catcher.GetNextResult());
 
-  action_icon = icon_factory.GetIcon(0);
+  EXPECT_FALSE(last_bar_icon.BackedBySameObjectAs(
+      GetBrowserActionsBar()->GetIcon(0).AsImageSkia()));
+  last_bar_icon = GetBrowserActionsBar()->GetIcon(0).AsImageSkia();
 
+  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());
 
-  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));
+  EXPECT_FALSE(last_bar_icon.BackedBySameObjectAs(
+      GetBrowserActionsBar()->GetIcon(0).AsImageSkia()));
+  last_bar_icon = GetBrowserActionsBar()->GetIcon(0).AsImageSkia();
 
+  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(gfx::BitmapsAreEqual(
-      *action_icon.ToSkBitmap(),
-      action_icon_skia->GetRepresentation(2.0f).sk_bitmap()));
+  EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale));
+  EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kLargeIconScale));
 
-  EXPECT_TRUE(
-      ImagesAreEqualAtScale(AddBackground(*action_icon_skia),
-                            *GetBrowserActionsBar()->GetIcon(0).ToImageSkia(),
-                            2.0f));
+  EXPECT_TRUE(gfx::BitmapsAreEqual(*action_icon.ToSkBitmap(),
+                                   action_icon.ToImageSkia()
+                                       ->GetRepresentation(kLargeIconScale)
+                                       .sk_bitmap()));
 
   // Try setting icon with empty dictionary of ImageData objects.
   GetBrowserActionsBar()->Press(0);