Attempt 2 at landing this. Patch is exactly same as last time around.

Adds ability for ImageLoadingTracker to cache images.

BUG=none
TEST=none
TBR=aa@chromium.org

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

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

4 files changed, 287 insertions, 29 deletions

diff --git a/chrome/browser/extensions/extension_install_ui.cc b/chrome/browser/extensions/extension_install_ui.cc
index b95b957..3accc44 100644
--- a/chrome/browser/extensions/extension_install_ui.cc
+++ b/chrome/browser/extensions/extension_install_ui.cc
@@ -377,9 +377,10 @@ void ExtensionInstallUI::ShowConfirmation(PromptType prompt_type) {
   prompt_type_ = prompt_type;
   ExtensionResource image =
       extension_->GetIconPath(Extension::EXTENSION_ICON_LARGE);
-  tracker_.LoadImage(image,
+  tracker_.LoadImage(extension_, image,
                      gfx::Size(Extension::EXTENSION_ICON_LARGE,
-                               Extension::EXTENSION_ICON_LARGE));
+                               Extension::EXTENSION_ICON_LARGE),
+                     ImageLoadingTracker::DONT_CACHE);
 }
 
 #if defined(OS_MACOSX)
diff --git a/chrome/browser/extensions/image_loading_tracker.cc b/chrome/browser/extensions/image_loading_tracker.cc
index a1f4ad2..2f7ef4d 100644
--- a/chrome/browser/extensions/image_loading_tracker.cc
+++ b/chrome/browser/extensions/image_loading_tracker.cc
@@ -7,6 +7,8 @@
 #include "base/file_util.h"
 #include "chrome/browser/chrome_thread.h"
 #include "chrome/common/extensions/extension_resource.h"
+#include "chrome/common/notification_service.h"
+#include "chrome/common/notification_type.h"
 #include "skia/ext/image_operations.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "webkit/glue/image_decoder.h"
@@ -33,23 +35,25 @@ class ImageLoadingTracker::ImageLoader
 
   // Instructs the loader to load a task on the File thread.
   void LoadImage(const ExtensionResource& resource,
-                 const gfx::Size& max_size) {
+                 const gfx::Size& max_size,
+                 int id) {
     DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::FILE));
     ChromeThread::PostTask(
         ChromeThread::FILE, FROM_HERE,
         NewRunnableMethod(this, &ImageLoader::LoadOnFileThread, resource,
-                          max_size));
+                          max_size, id));
   }
 
   void LoadOnFileThread(ExtensionResource resource,
-                        const gfx::Size& max_size) {
+                        const gfx::Size& max_size,
+                        int id) {
     DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
 
     // Read the file from disk.
     std::string file_contents;
     FilePath path = resource.GetFilePath();
     if (path.empty() || !file_util::ReadFileToString(path, &file_contents)) {
-      ReportBack(NULL, resource);
+      ReportBack(NULL, resource, id);
       return;
     }
 
@@ -60,7 +64,7 @@ class ImageLoadingTracker::ImageLoader
     scoped_ptr<SkBitmap> decoded(new SkBitmap());
     *decoded = decoder.Decode(data, file_contents.length());
     if (decoded->empty()) {
-      ReportBack(NULL, resource);
+      ReportBack(NULL, resource, id);
       return;  // Unable to decode.
     }
 
@@ -72,26 +76,27 @@ class ImageLoadingTracker::ImageLoader
           max_size.width(), max_size.height());
     }
 
-    ReportBack(decoded.release(), resource);
+    ReportBack(decoded.release(), resource, id);
   }
 
-  void ReportBack(SkBitmap* image, const ExtensionResource& resource) {
+  void ReportBack(SkBitmap* image, const ExtensionResource& resource,
+                  int id) {
     DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
 
     ChromeThread::PostTask(
         callback_thread_id_, FROM_HERE,
         NewRunnableMethod(this, &ImageLoader::ReportOnUIThread,
-                                image, resource));
+                          image, resource, id));
   }
 
-  void ReportOnUIThread(SkBitmap* image, ExtensionResource resource) {
+  void ReportOnUIThread(SkBitmap* image, ExtensionResource resource,
+                        int id) {
     DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::FILE));
 
     if (tracker_)
-      tracker_->OnImageLoaded(image, resource);
+      tracker_->OnImageLoaded(image, resource, id);
 
-    if (image)
-      delete image;
+    delete image;
   }
 
  private:
@@ -110,7 +115,11 @@ class ImageLoadingTracker::ImageLoader
 
 ImageLoadingTracker::ImageLoadingTracker(Observer* observer)
     : observer_(observer),
-      responses_(0) {
+      next_id_(0) {
+  registrar_.Add(this, NotificationType::EXTENSION_UNLOADED,
+                 NotificationService::AllSources());
+  registrar_.Add(this, NotificationType::EXTENSION_UNLOADED_DISABLED,
+                 NotificationService::AllSources());
 }
 
 ImageLoadingTracker::~ImageLoadingTracker() {
@@ -120,23 +129,66 @@ ImageLoadingTracker::~ImageLoadingTracker() {
     loader_->StopTracking();
 }
 
-void ImageLoadingTracker::LoadImage(const ExtensionResource& resource,
-                                    gfx::Size max_size) {
+void ImageLoadingTracker::LoadImage(Extension* extension,
+                                    const ExtensionResource& resource,
+                                    const gfx::Size& max_size,
+                                    CacheParam cache) {
+  DCHECK(extension->path() == resource.extension_root());
+
   // If we don't have a path we don't need to do any further work, just respond
   // back.
+  int id = next_id_++;
   if (resource.relative_path().empty()) {
-    OnImageLoaded(NULL, resource);
+    OnImageLoaded(NULL, resource, id);
+    return;
+  }
+
+  // See if the extension has the image already.
+  if (extension->HasCachedImage(resource)) {
+    SkBitmap image = extension->GetCachedImage(resource);
+    OnImageLoaded(&image, resource, id);
     return;
   }
 
+  if (cache == CACHE) {
+    load_map_[id] = extension;
+  }
+
   // Instruct the ImageLoader to load this on the File thread. LoadImage does
   // not block.
   if (!loader_)
     loader_ = new ImageLoader(this);
-  loader_->LoadImage(resource, max_size);
+  loader_->LoadImage(resource, max_size, id);
 }
 
 void ImageLoadingTracker::OnImageLoaded(
-    SkBitmap* image, const ExtensionResource& resource) {
-  observer_->OnImageLoaded(image, resource, responses_++);
+    SkBitmap* image,
+    const ExtensionResource& resource,
+    int id) {
+  LoadMap::iterator i = load_map_.find(id);
+  if (i != load_map_.end()) {
+    i->second->SetCachedImage(resource, image ? *image : SkBitmap());
+    load_map_.erase(i);
+  }
+
+  observer_->OnImageLoaded(image, resource, id);
+}
+
+void ImageLoadingTracker::Observe(NotificationType type,
+                                  const NotificationSource& source,
+                                  const NotificationDetails& details) {
+  DCHECK(type == NotificationType::EXTENSION_UNLOADED ||
+         type == NotificationType::EXTENSION_UNLOADED_DISABLED);
+
+  Extension* extension = Details<Extension>(details).ptr();
+
+  // Remove all entries in the load_map_ referencing the extension. This ensures
+  // we don't attempt to cache the image when the load completes.
+  for (LoadMap::iterator i = load_map_.begin(); i != load_map_.end();) {
+    if (i->second == extension) {
+      load_map_.erase(i++);
+    } else {
+      ++i;
+    }
+  }
 }
diff --git a/chrome/browser/extensions/image_loading_tracker.h b/chrome/browser/extensions/image_loading_tracker.h
index adcf009..e4df4cf 100644
--- a/chrome/browser/extensions/image_loading_tracker.h
+++ b/chrome/browser/extensions/image_loading_tracker.h
@@ -5,8 +5,12 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_IMAGE_LOADING_TRACKER_H_
 #define CHROME_BROWSER_EXTENSIONS_IMAGE_LOADING_TRACKER_H_
 
+#include <map>
+
 #include "base/ref_counted.h"
 #include "chrome/common/extensions/extension.h"
+#include "chrome/common/notification_observer.h"
+#include "chrome/common/notification_registrar.h"
 
 class ExtensionResource;
 class SkBitmap;
@@ -23,12 +27,20 @@ namespace gfx {
 // To use this class, have your class derive from ImageLoadingTracker::Observer,
 // and add a member variable ImageLoadingTracker tracker_. Then override
 // Observer::OnImageLoaded and call:
-//   tracker_.LoadImage(resource, max_size);
+//   tracker_.LoadImage(extension, resource, max_size, false);
 // ... and wait for OnImageLoaded to be called back on you with a pointer to the
 // SkBitmap loaded.
+// NOTE: if the image is available already (or the resource is not valid), the
+// Observer is notified immediately from the call to LoadImage. In other words,
+// by the time LoadImage returns the observer has been notified.
 //
-class ImageLoadingTracker {
+class ImageLoadingTracker : public NotificationObserver {
  public:
+  enum CacheParam {
+    CACHE,
+    DONT_CACHE
+  };
+
   class Observer {
    public:
     // Will be called when the image with the given index has loaded.
@@ -37,7 +49,7 @@ class ImageLoadingTracker {
     // image was not found or it failed to decode. |resource| is the
     // ExtensionResource where the |image| came from and the |index| represents
     // the index of the image just loaded (starts at 0 and increments every
-    // time this function is called).
+    // time LoadImage is called).
     virtual void OnImageLoaded(SkBitmap* image, ExtensionResource resource,
                                int index) = 0;
   };
@@ -47,10 +59,14 @@ class ImageLoadingTracker {
 
   // Specify image resource to load. If the loaded image is larger than
   // |max_size| it will be resized to those dimensions.
-  void LoadImage(const ExtensionResource& resource,
-                 gfx::Size max_size);
+  void LoadImage(Extension* extension,
+                 const ExtensionResource& resource,
+                 const gfx::Size& max_size,
+                 CacheParam cache);
 
  private:
+  typedef std::map<int, Extension*> LoadMap;
+
   class ImageLoader;
 
   // When an image has finished loaded and been resized on the file thread, it
@@ -58,17 +74,31 @@ class ImageLoadingTracker {
   // calls the observer's OnImageLoaded and deletes the ImageLoadingTracker if
   // it was the last image in the list.
   // |image| may be null if the file failed to decode.
-  void OnImageLoaded(SkBitmap* image, const ExtensionResource& resource);
+  void OnImageLoaded(SkBitmap* image, const ExtensionResource& resource,
+                     int id);
+
+  // NotificationObserver method. If an extension is uninstalled while we're
+  // waiting for the image we remove the entry from load_map_.
+  virtual void Observe(NotificationType type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details);
 
   // The view that is waiting for the image to load.
   Observer* observer_;
 
-  // The number of times we've reported back.
-  int responses_;
+  // ID to use for next image requested. This is an ever increasing integer.
+  int next_id_;
 
   // The object responsible for loading the image on the File thread.
   scoped_refptr<ImageLoader> loader_;
 
+  // If LoadImage is told to cache the result an entry is added here. The
+  // integer identifies the id assigned to the request. If the extension is
+  // deleted while fetching the image the entry is removed from the map.
+  LoadMap load_map_;
+
+  NotificationRegistrar registrar_;
+
   DISALLOW_COPY_AND_ASSIGN(ImageLoadingTracker);
 };
 
diff --git a/chrome/browser/extensions/image_loading_tracker_unittest.cc b/chrome/browser/extensions/image_loading_tracker_unittest.cc
new file mode 100644
index 0000000..dde786e
--- /dev/null
+++ b/chrome/browser/extensions/image_loading_tracker_unittest.cc
@@ -0,0 +1,175 @@
+// 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 "chrome/browser/chrome_thread.h"
+#include "chrome/browser/extensions/image_loading_tracker.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/json_value_serializer.h"
+#include "chrome/common/notification_service.h"
+#include "chrome/common/notification_type.h"
+#include "gfx/size.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class ImageLoadingTrackerTest : public testing::Test,
+                                public ImageLoadingTracker::Observer {
+ public:
+  ImageLoadingTrackerTest()
+      : image_loaded_count_(0),
+        quit_in_image_loaded_(false),
+        ui_thread_(ChromeThread::UI, &ui_loop_),
+        file_thread_(ChromeThread::FILE),
+        io_thread_(ChromeThread::IO) {
+  }
+
+  virtual void OnImageLoaded(SkBitmap* image, ExtensionResource resource,
+                             int index) {
+    image_loaded_count_++;
+    if (quit_in_image_loaded_)
+      MessageLoop::current()->Quit();
+    if (image)
+      image_ = *image;
+    else
+      image_.reset();
+  }
+
+  void WaitForImageLoad() {
+    quit_in_image_loaded_ = true;
+    MessageLoop::current()->Run();
+    quit_in_image_loaded_ = false;
+  }
+
+  int image_loaded_count() {
+    int result = image_loaded_count_;
+    image_loaded_count_ = 0;
+    return result;
+  }
+
+  Extension* CreateExtension() {
+    // Create and load an extension.
+    FilePath test_file;
+    if (!PathService::Get(chrome::DIR_TEST_DATA, &test_file)) {
+      EXPECT_FALSE(true);
+      return NULL;
+    }
+    test_file = test_file.AppendASCII("extensions")
+                         .AppendASCII("image_loading_tracker");
+    std::string error;
+    JSONFileValueSerializer serializer(test_file.AppendASCII("app.json"));
+    scoped_ptr<DictionaryValue> valid_value(
+        static_cast<DictionaryValue*>(serializer.Deserialize(&error)));
+    EXPECT_EQ("", error);
+    if (error != "")
+      return NULL;
+
+    EXPECT_TRUE(valid_value.get());
+    if (!valid_value.get())
+      return NULL;
+
+    scoped_ptr<Extension> extension(new Extension(test_file));
+    if (!extension->InitFromValue(*valid_value, false, &error))
+      return NULL;
+
+    return extension.release();
+  }
+
+  SkBitmap image_;
+
+ private:
+  virtual void SetUp() {
+    file_thread_.Start();
+    io_thread_.Start();
+  }
+
+  int image_loaded_count_;
+  bool quit_in_image_loaded_;
+  MessageLoop ui_loop_;
+  ChromeThread ui_thread_;
+  ChromeThread file_thread_;
+  ChromeThread io_thread_;
+};
+
+// Tests asking ImageLoadingTracker to cache pushes the result to the Extension.
+TEST_F(ImageLoadingTrackerTest, Cache) {
+  scoped_ptr<Extension> extension(CreateExtension());
+  ASSERT_TRUE(extension.get() != NULL);
+
+  ExtensionResource image_resource =
+      extension->GetIconPath(Extension::EXTENSION_ICON_SMALLISH);
+  ImageLoadingTracker loader(static_cast<ImageLoadingTracker::Observer*>(this));
+  loader.LoadImage(extension.get(),
+                   image_resource,
+                   gfx::Size(Extension::EXTENSION_ICON_SMALLISH,
+                             Extension::EXTENSION_ICON_SMALLISH),
+                   ImageLoadingTracker::CACHE);
+
+  // The image isn't cached, so we should not have received notification.
+  EXPECT_EQ(0, image_loaded_count());
+
+  WaitForImageLoad();
+
+  // We should have gotten the image.
+  EXPECT_EQ(1, image_loaded_count());
+
+  // Check that the image was loaded.
+  EXPECT_EQ(Extension::EXTENSION_ICON_SMALLISH, image_.width());
+
+  // The image should be cached in the Extension.
+  EXPECT_TRUE(extension->HasCachedImage(image_resource));
+
+  // Make sure the image is in the extension.
+  EXPECT_EQ(Extension::EXTENSION_ICON_SMALLISH,
+            extension->GetCachedImage(image_resource).width());
+
+  // Ask the tracker for the image again, this should call us back immediately.
+  loader.LoadImage(extension.get(),
+                   image_resource,
+                   gfx::Size(Extension::EXTENSION_ICON_SMALLISH,
+                             Extension::EXTENSION_ICON_SMALLISH),
+                   ImageLoadingTracker::CACHE);
+  // We should have gotten the image.
+  EXPECT_EQ(1, image_loaded_count());
+
+  // Check that the image was loaded.
+  EXPECT_EQ(Extension::EXTENSION_ICON_SMALLISH, image_.width());
+}
+
+// Tests deleting an extension while waiting for the image to load doesn't cause
+// problems.
+TEST_F(ImageLoadingTrackerTest, DeleteExtensionWhileWaitingForCache) {
+  scoped_ptr<Extension> extension(CreateExtension());
+  ASSERT_TRUE(extension.get() != NULL);
+
+  ExtensionResource image_resource =
+      extension->GetIconPath(Extension::EXTENSION_ICON_SMALLISH);
+  ImageLoadingTracker loader(static_cast<ImageLoadingTracker::Observer*>(this));
+  loader.LoadImage(extension.get(),
+                   image_resource,
+                   gfx::Size(Extension::EXTENSION_ICON_SMALLISH,
+                             Extension::EXTENSION_ICON_SMALLISH),
+                   ImageLoadingTracker::CACHE);
+
+  // The image isn't cached, so we should not have received notification.
+  EXPECT_EQ(0, image_loaded_count());
+
+  // Send out notification the extension was uninstalled.
+  NotificationService::current()->Notify(
+      NotificationType::EXTENSION_UNLOADED,
+      NotificationService::AllSources(),
+      Details<Extension>(extension.get()));
+
+  // Chuck the image, that way if anyone tries to access it we should crash or
+  // get valgrind errors.
+  extension.release();
+
+  WaitForImageLoad();
+
+  // Even though we deleted the extension, we should still get the image.
+  // We should still have gotten the image.
+  EXPECT_EQ(1, image_loaded_count());
+
+  // Check that the image was loaded.
+  EXPECT_EQ(Extension::EXTENSION_ICON_SMALLISH, image_.width());
+}