Fix problem with extension context menu items losing top-level icon.

If an extension adds some context menu items, then removes them all, and adds
some more, instead of their actual icon the new items will get the default
extension icon in context menus. This is because we use the caching feature of
ImageLoadingTracker and get an immediate callback from LoadImage before we've
put the request in pending_icons_.

BUG=53543
TEST=Steps are outlined in bug report.

Review URL: http://codereview.chromium.org/3425007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@59726 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 172 insertions, 21 deletions

diff --git a/chrome/browser/extensions/extension_icon_manager.cc b/chrome/browser/extensions/extension_icon_manager.cc
index 23eeec5..53c8923 100644
--- a/chrome/browser/extensions/extension_icon_manager.cc
+++ b/chrome/browser/extensions/extension_icon_manager.cc
@@ -49,11 +49,13 @@ void ExtensionIconManager::LoadIcon(Extension* extension) {
   ExtensionResource icon_resource = extension->GetIconResource(
       Extension::EXTENSION_ICON_BITTY, ExtensionIconSet::MATCH_BIGGER);
   if (!icon_resource.extension_root().empty()) {
+    // Insert into pending_icons_ first because LoadImage can call us back
+    // synchronously if the image is already cached.
+    pending_icons_.insert(extension->id());
     image_tracker_.LoadImage(extension,
                              icon_resource,
                              gfx::Size(kFavIconSize, kFavIconSize),
                              ImageLoadingTracker::CACHE);
-    pending_icons_.insert(extension->id());
   }
 }
 
diff --git a/chrome/browser/extensions/extension_icon_manager_unittest.cc b/chrome/browser/extensions/extension_icon_manager_unittest.cc
new file mode 100644
index 0000000..f1d6dca
--- /dev/null
+++ b/chrome/browser/extensions/extension_icon_manager_unittest.cc
@@ -0,0 +1,135 @@
+// Copyright (c) 2010 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 "base/message_loop.h"
+#include "base/path_service.h"
+#include "base/values.h"
+#include "chrome/browser/chrome_thread.h"
+#include "chrome/browser/extensions/extension_icon_manager.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/extension_resource.h"
+#include "chrome/common/json_value_serializer.h"
+#include "gfx/skia_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+// Our test class that takes care of managing the necessary threads for loading
+// extension icons, and waiting for those loads to happen.
+class ExtensionIconManagerTest : public testing::Test {
+ public:
+  ExtensionIconManagerTest() :
+      unwaited_image_loads_(0),
+      waiting_(false),
+      ui_thread_(ChromeThread::UI, &ui_loop_),
+      file_thread_(ChromeThread::FILE),
+      io_thread_(ChromeThread::IO) {}
+
+  virtual ~ExtensionIconManagerTest() {}
+
+  void ImageLoadObserved() {
+    unwaited_image_loads_++;
+    if (waiting_) {
+      MessageLoop::current()->Quit();
+    }
+  }
+
+  void WaitForImageLoad() {
+    if (unwaited_image_loads_ == 0) {
+      waiting_ = true;
+      MessageLoop::current()->Run();
+      waiting_ = false;
+    }
+    ASSERT_GT(unwaited_image_loads_, 0);
+    unwaited_image_loads_--;
+  }
+
+ private:
+  virtual void SetUp() {
+    file_thread_.Start();
+    io_thread_.Start();
+  }
+
+  // The number of observed image loads that have not been waited for.
+  int unwaited_image_loads_;
+
+  // Whether we are currently waiting for an image load.
+  bool waiting_;
+
+  MessageLoop ui_loop_;
+  ChromeThread ui_thread_;
+  ChromeThread file_thread_;
+  ChromeThread io_thread_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionIconManagerTest);
+};
+
+// This is a specialization of ExtensionIconManager, with a special override to
+// call back to the test when an icon has completed loading.
+class TestIconManager : public ExtensionIconManager {
+ public:
+  explicit TestIconManager(ExtensionIconManagerTest* test) : test_(test) {}
+  virtual ~TestIconManager() {}
+
+  // Implements the ImageLoadingTracker::Observer interface, and calls through
+  // to the base class' implementation. Then it lets the test know that an
+  // image load was observed.
+  virtual void OnImageLoaded(SkBitmap* image, ExtensionResource resource,
+                             int index) {
+    ExtensionIconManager::OnImageLoaded(image, resource, index);
+    test_->ImageLoadObserved();
+  }
+
+ private:
+  ExtensionIconManagerTest* test_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestIconManager);
+};
+
+// Returns the default icon that ExtensionIconManager gives when an extension
+// doesn't have an icon.
+SkBitmap GetDefaultIcon() {
+  std::string dummy_id;
+  EXPECT_TRUE(Extension::GenerateId(std::string("whatever"), &dummy_id));
+  ExtensionIconManager manager;
+  return manager.GetIcon(dummy_id);
+}
+
+// Tests loading an icon for an extension, removing it, then re-loading it.
+TEST_F(ExtensionIconManagerTest, LoadRemoveLoad) {
+  SkBitmap default_icon = GetDefaultIcon();
+
+  FilePath test_dir;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_dir));
+  FilePath manifest_path = test_dir.AppendASCII(
+      "extensions/image_loading_tracker/app.json");
+
+  JSONFileValueSerializer serializer(manifest_path);
+  scoped_ptr<DictionaryValue> manifest(
+      static_cast<DictionaryValue*>(serializer.Deserialize(NULL, NULL)));
+  ASSERT_TRUE(manifest.get() != NULL);
+
+  Extension extension(manifest_path.DirName());
+  ASSERT_TRUE(extension.InitFromValue(*manifest.get(),
+                                      false /* require_key */,
+                                      NULL /* errors */));
+  TestIconManager icon_manager(this);
+
+  // Load the icon and grab the bitmap.
+  icon_manager.LoadIcon(&extension);
+  WaitForImageLoad();
+  SkBitmap first_icon = icon_manager.GetIcon(extension.id());
+  EXPECT_FALSE(gfx::BitmapsAreEqual(first_icon, default_icon));
+
+  // Remove the icon from the manager.
+  icon_manager.RemoveIcon(extension.id());
+
+  // Now re-load the icon - we should get the same result bitmap (and not the
+  // default icon).
+  icon_manager.LoadIcon(&extension);
+  WaitForImageLoad();
+  SkBitmap second_icon = icon_manager.GetIcon(extension.id());
+  EXPECT_FALSE(gfx::BitmapsAreEqual(second_icon, default_icon));
+
+  EXPECT_TRUE(gfx::BitmapsAreEqual(first_icon, second_icon));
+}
diff --git a/chrome/browser/extensions/image_loading_tracker.h b/chrome/browser/extensions/image_loading_tracker.h
index b6a19b0..bac2f04 100644
--- a/chrome/browser/extensions/image_loading_tracker.h
+++ b/chrome/browser/extensions/image_loading_tracker.h
@@ -61,7 +61,9 @@ class ImageLoadingTracker : public NotificationObserver {
   ~ImageLoadingTracker();
 
   // Specify image resource to load. If the loaded image is larger than
-  // |max_size| it will be resized to those dimensions.
+  // |max_size| it will be resized to those dimensions. IMPORTANT NOTE: this
+  // function may call back your observer synchronously (ie before it returns)
+  // if the image was found in the cache.
   void LoadImage(Extension* extension,
                  const ExtensionResource& resource,
                  const gfx::Size& max_size,
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 94e1665..9ce7667 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1053,6 +1053,7 @@
         'browser/download/save_package_unittest.cc',
         'browser/encoding_menu_controller_unittest.cc',
         'browser/extensions/convert_user_script_unittest.cc',
+        'browser/extensions/extension_icon_manager_unittest.cc',
         'browser/extensions/extension_install_ui_unittest.cc',
         'browser/extensions/extension_menu_manager_unittest.cc',
         'browser/extensions/extension_prefs_unittest.cc',
diff --git a/chrome/common/extensions/extension_action_unittest.cc b/chrome/common/extensions/extension_action_unittest.cc
index 7292ed9..1e3c8d4 100644
--- a/chrome/common/extensions/extension_action_unittest.cc
+++ b/chrome/common/extensions/extension_action_unittest.cc
@@ -7,11 +7,14 @@
 #include "base/path_service.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/extension_action.h"
+#include "gfx/skia_util.h"
 #include "googleurl/src/gurl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "webkit/glue/image_decoder.h"
 
+using gfx::BitmapsAreEqual;
+
 static SkBitmap LoadIcon(const std::string& filename) {
   FilePath path;
   PathService::Get(chrome::DIR_TEST_DATA, &path);
@@ -29,21 +32,6 @@ static SkBitmap LoadIcon(const std::string& filename) {
   return bitmap;
 }
 
-static bool BitmapsAreEqual(const SkBitmap& bitmap1, const SkBitmap& bitmap2) {
-  void* addr1 = NULL;
-  void* addr2 = NULL;
-
-  bitmap1.lockPixels();
-  addr1 = bitmap1.getAddr32(0, 0);
-  bitmap1.unlockPixels();
-
-  bitmap2.lockPixels();
-  addr2 = bitmap2.getAddr32(0, 0);
-  bitmap2.unlockPixels();
-
-  return 0 == memcmp(addr1, addr2, bitmap1.getSize());
-}
-
 TEST(ExtensionActionTest, TabSpecificState) {
   ExtensionAction action;
 
diff --git a/gfx/skia_util.cc b/gfx/skia_util.cc
index 13ef4d9..865f8fda 100644
--- a/gfx/skia_util.cc
+++ b/gfx/skia_util.cc
@@ -5,6 +5,7 @@
 #include "gfx/skia_util.h"
 
 #include "gfx/rect.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColorPriv.h"
 #include "third_party/skia/include/core/SkShader.h"
 #include "third_party/skia/include/effects/SkGradientShader.h"
@@ -38,5 +39,23 @@ SkShader* CreateGradientShader(int start_point,
       grad_points, grad_colors, NULL, 2, SkShader::kRepeat_TileMode);
 }
 
-}  // namespace gfx
+bool BitmapsAreEqual(const SkBitmap& bitmap1, const SkBitmap& bitmap2) {
+  void* addr1 = NULL;
+  void* addr2 = NULL;
+  size_t size1 = 0;
+  size_t size2 = 0;
+
+  bitmap1.lockPixels();
+  addr1 = bitmap1.getAddr32(0, 0);
+  size1 = bitmap1.getSize();
+  bitmap1.unlockPixels();
+
+  bitmap2.lockPixels();
+  addr2 = bitmap2.getAddr32(0, 0);
+  size2 = bitmap2.getSize();
+  bitmap2.unlockPixels();
 
+  return (size1 == size2) && (0 == memcmp(addr1, addr2, bitmap1.getSize()));
+}
+
+}  // namespace gfx
diff --git a/gfx/skia_util.h b/gfx/skia_util.h
index 4a55c77..26c54bf 100644
--- a/gfx/skia_util.h
+++ b/gfx/skia_util.h
@@ -2,13 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef APP_GFX_SKIA_UTIL_H_
-#define APP_GFX_SKIA_UTIL_H_
+#ifndef GFX_SKIA_UTIL_H_
+#define GFX_SKIA_UTIL_H_
 #pragma once
 
 #include "third_party/skia/include/core/SkColor.h"
 #include "third_party/skia/include/core/SkRect.h"
 
+class SkBitmap;
 class SkShader;
 
 namespace gfx {
@@ -30,6 +31,9 @@ SkShader* CreateGradientShader(int start_point,
                                SkColor start_color,
                                SkColor end_color);
 
+// Returns true if the two bitmaps contain the same pixels.
+bool BitmapsAreEqual(const SkBitmap& bitmap1, const SkBitmap& bitmap2);
+
 }  // namespace gfx;
 
-#endif  // APP_GFX_SKIA_UTIL_H_
+#endif  // GFX_SKIA_UTIL_H_