BrowserThemePack: Move encoding the processed images to the writing thread.

BUG=NONE
TEST=NONE

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

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

5 files changed, 81 insertions, 38 deletions

diff --git a/app/theme_provider.h b/app/theme_provider.h
index 410e699..0bd7ae1 100644
--- a/app/theme_provider.h
+++ b/app/theme_provider.h
@@ -61,8 +61,9 @@ class ThemeProvider {
   // doesn't provide a certain image, but custom themes might (badges, etc).
   virtual bool HasCustomImage(int id) const = 0;
 
-  // Reads the image data from the theme file into the specified
-  // vector. Returns NULL on error.
+  // Reads the image data from the theme file into the specified vector. Only
+  // valid for un-themed resources and the themed IDR_THEME_NTP_* in most
+  // implementations of ThemeProvider. Returns NULL on error.
   virtual RefCountedMemory* GetRawData(int id) const = 0;
 
 #if defined(OS_LINUX) && !defined(TOOLKIT_VIEWS)
diff --git a/chrome/browser/browser_theme_pack.cc b/chrome/browser/browser_theme_pack.cc
index 78be2f9..638cca2 100644
--- a/chrome/browser/browser_theme_pack.cc
+++ b/chrome/browser/browser_theme_pack.cc
@@ -203,11 +203,13 @@ BrowserThemePack::~BrowserThemePack() {
     delete [] display_properties_;
   }
 
-  STLDeleteValues(&image_cache_);
+  STLDeleteValues(&prepared_images_);
+  STLDeleteValues(&loaded_images_);
 }
 
 // static
 BrowserThemePack* BrowserThemePack::BuildFromExtension(Extension* extension) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(extension);
   DCHECK(extension->IsTheme());
 
@@ -222,23 +224,19 @@ BrowserThemePack* BrowserThemePack::BuildFromExtension(Extension* extension) {
   pack->ParseImageNamesFromJSON(extension->GetThemeImages(),
                                 extension->path(),
                                 &file_paths);
-  pack->LoadRawBitmapsTo(file_paths, &pack->image_cache_);
+  pack->LoadRawBitmapsTo(file_paths, &pack->prepared_images_);
 
-  pack->GenerateFrameImages(&pack->image_cache_);
+  pack->GenerateFrameImages(&pack->prepared_images_);
 
 #if !defined(OS_MACOSX)
   // OSX uses its own special buttons that are PDFs that do odd sorts of vector
   // graphics tricks. Other platforms use bitmaps and we must pre-tint them.
   pack->GenerateTintedButtons(
       pack->GetTintInternal(BrowserThemeProvider::TINT_BUTTONS),
-      &pack->image_cache_);
+      &pack->prepared_images_);
 #endif
 
-  pack->GenerateTabBackgroundImages(&pack->image_cache_);
-
-  // Repack all the images from |image_cache_| into |image_memory_| for
-  // writing to the data pack.
-  pack->RepackImageCacheToImageMemory();
+  pack->GenerateTabBackgroundImages(&pack->prepared_images_);
 
   // The BrowserThemePack is now in a consistent state.
   return pack;
@@ -247,6 +245,7 @@ BrowserThemePack* BrowserThemePack::BuildFromExtension(Extension* extension) {
 // static
 scoped_refptr<BrowserThemePack> BrowserThemePack::BuildFromDataPack(
     FilePath path, const std::string& expected_id) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   scoped_refptr<BrowserThemePack> pack = new BrowserThemePack;
   pack->data_pack_.reset(new base::DataPack);
 
@@ -291,8 +290,9 @@ scoped_refptr<BrowserThemePack> BrowserThemePack::BuildFromDataPack(
 }
 
 bool BrowserThemePack::WriteToDisk(FilePath path) const {
-  std::map<uint32, base::StringPiece> resources;
-
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+  // Add resources for each of the property arrays.
+  RawDataForWriting resources;
   resources[kHeaderID] = base::StringPiece(
       reinterpret_cast<const char*>(header_), sizeof(BrowserThemePackHeader));
   resources[kTintsID] = base::StringPiece(
@@ -304,11 +304,11 @@ bool BrowserThemePack::WriteToDisk(FilePath path) const {
       reinterpret_cast<const char*>(display_properties_),
       sizeof(DisplayPropertyPair[kDisplayPropertySize]));
 
-  for (RawImages::const_iterator it = image_memory_.begin();
-       it != image_memory_.end(); ++it) {
-    resources[it->first] = base::StringPiece(
-        reinterpret_cast<const char*>(it->second->front()), it->second->size());
-  }
+  AddRawImagesTo(image_memory_, &resources);
+
+  RawImages reencoded_images;
+  RepackImages(prepared_images_, &reencoded_images);
+  AddRawImagesTo(reencoded_images, &resources);
 
   return base::DataPack::WritePack(path, resources);
 }
@@ -356,8 +356,13 @@ bool BrowserThemePack::GetDisplayProperty(int id, int* result) const {
 
 SkBitmap* BrowserThemePack::GetBitmapNamed(int id) const {
   // Check our cache of prepared images, first.
-  ImageCache::const_iterator image_iter = image_cache_.find(id);
-  if (image_iter != image_cache_.end())
+  ImageCache::const_iterator image_iter = prepared_images_.find(id);
+  if (image_iter != prepared_images_.end())
+    return image_iter->second;
+
+  // Check if we've already loaded this image.
+  image_iter = loaded_images_.find(id);
+  if (image_iter != loaded_images_.end())
     return image_iter->second;
 
   scoped_refptr<RefCountedMemory> memory;
@@ -381,7 +386,7 @@ SkBitmap* BrowserThemePack::GetBitmapNamed(int id) const {
     }
 
     SkBitmap* ret = new SkBitmap(bitmap);
-    image_cache_[id] = ret;
+    loaded_images_[id] = ret;
 
     return ret;
   }
@@ -409,7 +414,8 @@ bool BrowserThemePack::HasCustomImage(int id) const {
     base::StringPiece ignored;
     return data_pack_->GetStringPiece(id, &ignored);
   } else {
-    return image_memory_.count(id) > 0;
+    return prepared_images_.count(id) > 0 ||
+        image_memory_.count(id) > 0;
   }
 }
 
@@ -810,17 +816,17 @@ void BrowserThemePack::GenerateTabBackgroundImages(ImageCache* bitmaps) const {
   MergeImageCaches(temp_output, bitmaps);
 }
 
-void BrowserThemePack::RepackImageCacheToImageMemory() {
-  // TODO(erg): This shouldn't be done on the main thread. This should be done
-  // during the WriteToDisk() method, but will require some tricky locking.
-  for (ImageCache::const_iterator it = image_cache_.begin();
-       it != image_cache_.end(); ++it) {
+void BrowserThemePack::RepackImages(const ImageCache& images,
+                                    RawImages* reencoded_images) const {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+  for (ImageCache::const_iterator it = images.begin();
+       it != images.end(); ++it) {
     std::vector<unsigned char> image_data;
     if (!gfx::PNGCodec::EncodeBGRASkBitmap(*(it->second), false, &image_data)) {
       NOTREACHED() << "Image file for resource " << it->first
                    << " could not be encoded.";
     } else {
-      image_memory_[it->first] = RefCountedBytes::TakeVector(&image_data);
+      (*reencoded_images)[it->first] = RefCountedBytes::TakeVector(&image_data);
     }
   }
 }
@@ -838,6 +844,16 @@ void BrowserThemePack::MergeImageCaches(
   }
 }
 
+void BrowserThemePack::AddRawImagesTo(const RawImages& images,
+                                      RawDataForWriting* out) const {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+  for (RawImages::const_iterator it = images.begin(); it != images.end();
+       ++it) {
+    (*out)[it->first] = base::StringPiece(
+        reinterpret_cast<const char*>(it->second->front()), it->second->size());
+  }
+}
+
 color_utils::HSL BrowserThemePack::GetTintInternal(int id) const {
   if (tints_) {
     for (int i = 0; i < kTintArraySize; ++i) {
diff --git a/chrome/browser/browser_theme_pack.h b/chrome/browser/browser_theme_pack.h
index 9b30872..8e4bff1 100644
--- a/chrome/browser/browser_theme_pack.h
+++ b/chrome/browser/browser_theme_pack.h
@@ -32,7 +32,9 @@ class RefCountedMemory;
 // A note on const-ness. All public, non-static methods are const.  We do this
 // because once we've constructed a BrowserThemePack through the
 // BuildFromExtension() interface, we WriteToDisk() on a thread other than the
-// UI thread that consumes a BrowserThemePack.
+// UI thread that consumes a BrowserThemePack. There is no locking; thread
+// safety between the writing thread and the UI thread is ensured by having the
+// data be immutable.
 class BrowserThemePack : public base::RefCountedThreadSafe<BrowserThemePack> {
  public:
   ~BrowserThemePack();
@@ -61,6 +63,9 @@ class BrowserThemePack : public base::RefCountedThreadSafe<BrowserThemePack> {
   bool GetColor(int id, SkColor* color) const;
   bool GetDisplayProperty(int id, int* result) const;
   SkBitmap* GetBitmapNamed(int id) const;
+
+  // Returns the raw PNG encoded data for IDR_THEME_NTP_*. This method is only
+  // supposed to work for the NTP attribution and background resources.
   RefCountedMemory* GetRawData(int id) const;
 
   // Whether this theme provides an image for |id|.
@@ -77,6 +82,9 @@ class BrowserThemePack : public base::RefCountedThreadSafe<BrowserThemePack> {
   // The raw PNG memory associated with a certain id.
   typedef std::map<int, scoped_refptr<RefCountedMemory> > RawImages;
 
+  // The type passed to base::DataPack::WritePack.
+  typedef std::map<uint32, base::StringPiece> RawDataForWriting;
+
   // Default. Everything is empty.
   BrowserThemePack();
 
@@ -121,15 +129,19 @@ class BrowserThemePack : public base::RefCountedThreadSafe<BrowserThemePack> {
   // in |bitmaps|. Must be called after GenerateFrameImages().
   void GenerateTabBackgroundImages(ImageCache* bitmaps) const;
 
-  // Takes all the SkBitmaps in |image_cache_|, encodes them as PNGs and places
-  // them in |image_memory_|.
-  void RepackImageCacheToImageMemory();
+  // Takes all the SkBitmaps in |images|, encodes them as PNGs and places
+  // them in |reencoded_images|.
+  void RepackImages(const ImageCache& images,
+                    RawImages* reencoded_images) const;
 
   // Takes all images in |source| and puts them in |destination|, freeing any
   // image already in |destination| that |source| would overwrite.
   void MergeImageCaches(const ImageCache& source,
                         ImageCache* destination) const;
 
+  // Changes the RefCountedMemory based |images| into StringPiece data in |out|.
+  void AddRawImagesTo(const RawImages& images, RawDataForWriting* out) const;
+
   // Retrieves the tint OR the default tint. Unlike the public interface, we
   // always need to return a reasonable tint here, instead of partially
   // querying if the tint exists.
@@ -180,10 +192,15 @@ class BrowserThemePack : public base::RefCountedThreadSafe<BrowserThemePack> {
   // needs to be in |image_memory_|.
   RawImages image_memory_;
 
-  // Tinted (or otherwise prepared) images for passing out. BrowserThemePack
-  // owns all these images. This cache isn't touched when we write our data to
-  // a DataPack.
-  mutable ImageCache image_cache_;
+  // An immutable cache of images generated in BuildFromExtension(). When this
+  // BrowserThemePack is generated from BuildFromDataPack(), this cache is
+  // empty. We separate the images from the images loaded from disk so that
+  // WriteToDisk()'s implementation doesn't need locks. There should be no IDs
+  // in |image_memory_| that are in |prepared_images_| or vice versa.
+  ImageCache prepared_images_;
+
+  // Loaded images. These are loaded from |image_memory_| or the |data_pack_|.
+  mutable ImageCache loaded_images_;
 
   DISALLOW_COPY_AND_ASSIGN(BrowserThemePack);
 };
diff --git a/chrome/browser/browser_theme_pack_unittest.cc b/chrome/browser/browser_theme_pack_unittest.cc
index 77da71b..7d780f2 100644
--- a/chrome/browser/browser_theme_pack_unittest.cc
+++ b/chrome/browser/browser_theme_pack_unittest.cc
@@ -20,7 +20,12 @@
 
 class BrowserThemePackTest : public ::testing::Test {
  public:
-  BrowserThemePackTest() : theme_pack_(new BrowserThemePack) { }
+  BrowserThemePackTest()
+      : message_loop(),
+        fake_ui_thread(ChromeThread::UI, &message_loop),
+        fake_file_thread(ChromeThread::FILE, &message_loop),
+        theme_pack_(new BrowserThemePack) {
+  }
 
   // Transformation for link underline colors.
   SkColor BuildThirdOpacity(SkColor color_link) {
@@ -159,6 +164,10 @@ class BrowserThemePackTest : public ::testing::Test {
     EXPECT_FALSE(pack->GetTint(BrowserThemeProvider::TINT_FRAME, &actual));
   }
 
+  MessageLoop message_loop;
+  ChromeThread fake_ui_thread;
+  ChromeThread fake_file_thread;
+
   scoped_refptr<BrowserThemePack> theme_pack_;
 };
 
diff --git a/chrome/browser/browser_theme_provider.cc b/chrome/browser/browser_theme_provider.cc
index 3887c2f..338e42e 100644
--- a/chrome/browser/browser_theme_provider.cc
+++ b/chrome/browser/browser_theme_provider.cc
@@ -566,7 +566,7 @@ void BrowserThemeProvider::BuildFromExtension(Extension* extension) {
 
   // Write the packed file to disk.
   FilePath pack_path = extension->path().Append(chrome::kThemePackFilename);
-  ChromeThread::PostTask(ChromeThread::IO, FROM_HERE,
+  ChromeThread::PostTask(ChromeThread::FILE, FROM_HERE,
                          new WritePackToDiskTask(pack, pack_path));
 
   SavePackName(pack_path);