Reverting 27520.

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

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

2 files changed, 61 insertions, 121 deletions

diff --git a/chrome/browser/browser_theme_provider.cc b/chrome/browser/browser_theme_provider.cc
index 96f1d29..53ead1f 100644
--- a/chrome/browser/browser_theme_provider.cc
+++ b/chrome/browser/browser_theme_provider.cc
@@ -9,10 +9,8 @@
 #include "base/gfx/png_decoder.h"
 #include "base/gfx/png_encoder.h"
 #include "base/string_util.h"
-#include "base/thread.h"
 #include "base/values.h"
 #include "chrome/browser/browser_list.h"
-#include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_window.h"
 #include "chrome/browser/metrics/user_metrics.h"
 #include "chrome/browser/profile.h"
@@ -200,60 +198,35 @@ static std::map<const int, bool> themeable_images_;
 // A map of frame image IDs to the tints for those ids.
 static std::map<const int, int> frame_tints_;
 
-Lock BrowserThemeProvider::themed_image_cache_lock_;
-
-namespace {
-
-class WriteImagesToDiskTask : public Task {
- public:
-  WriteImagesToDiskTask(
-      const BrowserThemeProvider::ImagesDiskCache& images_disk_cache,
-      const BrowserThemeProvider::ImageCache& themed_image_cache) :
-    images_disk_cache_(images_disk_cache),
-    themed_image_cache_(themed_image_cache) {
-  }
-
-  virtual void Run() {
-    AutoLock lock(BrowserThemeProvider::themed_image_cache_lock_);
-    BrowserThemeProvider::ImagesDiskCache::const_iterator iter;
-    for (iter = images_disk_cache_.begin();
-         iter != images_disk_cache_.end();
-         iter++) {
-      FilePath image_path = (*iter).first;
-      BrowserThemeProvider::ImageCache::const_iterator found =
-          themed_image_cache_.find((*iter).second);
-      if (found != themed_image_cache_.end()) {
-        SkBitmap* bitmap = found->second;
-        std::vector<unsigned char> image_data;
-        if (!PNGEncoder::EncodeBGRASkBitmap(*bitmap, false, &image_data)) {
-          NOTREACHED() << "Image file could not be encoded.";
-          return;
-        }
-        const char* image_data_ptr =
-            reinterpret_cast<const char*>(&image_data[0]);
-        if (!file_util::WriteFile(image_path,
-            image_data_ptr, image_data.size())) {
-          NOTREACHED() << "Image file could not be written to disk.";
-          return;
-        }
-      } else {
-        NOTREACHED() << "Themed image missing from cache.";
-        return;
+// TODO(mirandac): move this to a different thread.
+bool BrowserThemeProvider::WriteImagesToDisk() {
+  BrowserThemeProvider::ImagesDiskCache::iterator iter;
+  for (iter = images_disk_cache_.begin();
+       iter != images_disk_cache_.end();
+       iter++) {
+    FilePath image_path = (*iter).first;
+    ImageCache::const_iterator found = image_cache_.find((*iter).second);
+    if (found != image_cache_.end()) {
+      SkBitmap* bitmap = found->second;
+      std::vector<unsigned char> image_data;
+      if (!PNGEncoder::EncodeBGRASkBitmap(*bitmap, false, &image_data)) {
+        NOTREACHED() << "Image file could not be encoded.";
+        return false;
       }
+      const char* image_data_ptr =
+          reinterpret_cast<const char*>(&image_data[0]);
+      if (!file_util::WriteFile(image_path,
+                                image_data_ptr, image_data.size())) {
+        NOTREACHED() << "Image file could not be written to disk.";
+        return false;
+      }
+    } else {
+      NOTREACHED();
+      return false;
     }
   }
-
- private:
-  // References to data held in the BrowserThemeProvider.
-  const BrowserThemeProvider::ImagesDiskCache& images_disk_cache_;
-  const BrowserThemeProvider::ImageCache& themed_image_cache_;
-};
-}
-
-void BrowserThemeProvider::WriteImagesToDisk() {
-  g_browser_process->file_thread()->message_loop()->PostTask(FROM_HERE,
-      new WriteImagesToDiskTask(images_disk_cache_, themed_image_cache_));
-  SaveCachedImageData();
+  // Save is only successful if we made it through the entire cache.
+  return true;
 }
 
 BrowserThemeProvider::BrowserThemeProvider()
@@ -315,14 +288,7 @@ void BrowserThemeProvider::Init(Profile* profile) {
 SkBitmap* BrowserThemeProvider::GetBitmapNamed(int id) {
   DCHECK(CalledOnValidThread());
 
-  // First check to see if the Skia image is in the themed cache. The themed
-  // cache is not changed in this method, so it can remain unlocked.
-  ImageCache::const_iterator t_found = themed_image_cache_.find(id);
-  if (t_found != themed_image_cache_.end())
-    return t_found->second;
-
-  // If Skia image is not in themed cache, check regular cache, and possibly
-  // generate and store.
+  // Check to see if we already have the Skia image in the cache.
   ImageCache::const_iterator found = image_cache_.find(id);
   if (found != image_cache_.end())
     return found->second;
@@ -335,7 +301,7 @@ SkBitmap* BrowserThemeProvider::GetBitmapNamed(int id) {
   // If the extension doesn't provide the requested image, but has provided
   // a custom frame, then we may be able to generate the image required.
   if (!result.get())
-    result.reset(GenerateTabBackgroundBitmap(id));
+    result.reset(GenerateBitmap(id));
 
   // If we still don't have an image, load it from resourcebundle.
   if (!result.get())
@@ -587,12 +553,12 @@ void BrowserThemeProvider::SetTheme(Extension* extension) {
 
   GenerateFrameColors();
   if (ShouldTintFrames()) {
-    AutoLock lock(themed_image_cache_lock_);
     GenerateFrameImages();
     GenerateTabImages();
+    if (WriteImagesToDisk())
+      SaveCachedImageData();
   }
 
-  WriteImagesToDisk();
   NotifyThemeChanged();
   UserMetrics::RecordAction(L"Themes_Installed", profile_);
 }
@@ -671,7 +637,7 @@ SkBitmap* BrowserThemeProvider::LoadThemeBitmap(int id) {
 void BrowserThemeProvider::SaveThemeBitmap(
     std::string resource_name, int id) {
   DCHECK(CalledOnValidThread());
-  if (!themed_image_cache_[id]) {
+  if (!image_cache_[id]) {
     NOTREACHED();
     return;
   }
@@ -695,6 +661,8 @@ void BrowserThemeProvider::SaveThemeBitmap(
   images_disk_cache_[image_path] = id;
 }
 
+
+
 const std::string BrowserThemeProvider::GetTintKey(int id) {
   switch (id) {
     case TINT_FRAME:
@@ -985,21 +953,25 @@ void BrowserThemeProvider::GenerateFrameImages() {
     if (!process_images_) {
       frame.reset(LoadThemeBitmap(id));
       if (frame.get())
-        themed_image_cache_[id] = new SkBitmap(*frame.get());
+        image_cache_[id] = new SkBitmap(*frame.get());
     } else {
-      resource_name = resource_names_[id];
       if (id == IDR_THEME_FRAME_INCOGNITO_INACTIVE) {
+        resource_name = "theme_frame_incognito_inactive";
         base_id = HasCustomImage(IDR_THEME_FRAME_INCOGNITO) ?
             IDR_THEME_FRAME_INCOGNITO : IDR_THEME_FRAME;
       } else if (id == IDR_THEME_FRAME_OVERLAY_INACTIVE) {
         base_id = IDR_THEME_FRAME_OVERLAY;
+        resource_name = "theme_frame_overlay_inactive";
       } else if (id == IDR_THEME_FRAME_INACTIVE) {
         base_id = IDR_THEME_FRAME;
+        resource_name = "theme_frame_inactive";
       } else if (id == IDR_THEME_FRAME_INCOGNITO &&
                  !HasCustomImage(IDR_THEME_FRAME_INCOGNITO)) {
         base_id = IDR_THEME_FRAME;
+        resource_name = "theme_frame_incognito";
       } else {
         base_id = id;
+        resource_name = resource_names_[id];
       }
 
       if (HasCustomImage(id)) {
@@ -1020,7 +992,7 @@ void BrowserThemeProvider::GenerateFrameImages() {
 
       if (frame.get()) {
         SkBitmap* tinted = new SkBitmap(TintBitmap(*frame, iter->second));
-        themed_image_cache_[id] = tinted;
+        image_cache_[id] = tinted;
         SaveThemeBitmap(resource_name, id);
       }
     }
@@ -1038,8 +1010,8 @@ bool BrowserThemeProvider::ShouldTintFrames() {
 }
 
 void BrowserThemeProvider::GenerateTabImages() {
-  GenerateTabBackgroundBitmap(IDR_THEME_TAB_BACKGROUND);
-  GenerateTabBackgroundBitmap(IDR_THEME_TAB_BACKGROUND_INCOGNITO);
+  GenerateBitmap(IDR_THEME_TAB_BACKGROUND);
+  GenerateBitmap(IDR_THEME_TAB_BACKGROUND_INCOGNITO);
 }
 
 void BrowserThemeProvider::ClearAllThemeData() {
@@ -1059,7 +1031,7 @@ void BrowserThemeProvider::ClearAllThemeData() {
   SaveThemeID(kDefaultThemeID);
 }
 
-SkBitmap* BrowserThemeProvider::GenerateTabBackgroundBitmap(int id) {
+SkBitmap* BrowserThemeProvider::GenerateBitmap(int id) {
   if (id == IDR_THEME_TAB_BACKGROUND ||
       id == IDR_THEME_TAB_BACKGROUND_INCOGNITO) {
     // The requested image is a background tab. Get a frame to create the
@@ -1070,7 +1042,7 @@ SkBitmap* BrowserThemeProvider::GenerateTabBackgroundBitmap(int id) {
       scoped_ptr<SkBitmap> frame;
       frame.reset(LoadThemeBitmap(id));
       if (frame.get())
-        themed_image_cache_[id] = new SkBitmap(*frame.get());
+        image_cache_[id] = new SkBitmap(*frame.get());
     } else {
       int base_id;
       std::string resource_name;
@@ -1081,8 +1053,8 @@ SkBitmap* BrowserThemeProvider::GenerateTabBackgroundBitmap(int id) {
         base_id = IDR_THEME_FRAME_INCOGNITO;
         resource_name = "theme_tab_background_incognito";
       }
-      std::map<int, SkBitmap*>::iterator it = themed_image_cache_.find(base_id);
-      if (it != themed_image_cache_.end()) {
+      std::map<int, SkBitmap*>::iterator it = image_cache_.find(base_id);
+      if (it != image_cache_.end()) {
         SkBitmap bg_tint = TintBitmap(*(it->second), TINT_BACKGROUND_TAB);
         int vertical_offset = HasCustomImage(id) ?
             kRestoredTabVerticalOffset : 0;
@@ -1099,7 +1071,7 @@ SkBitmap* BrowserThemeProvider::GenerateTabBackgroundBitmap(int id) {
           }
         }
 
-        themed_image_cache_[id] = bg_tab;
+        image_cache_[id] = bg_tab;
         SaveThemeBitmap(resource_name, id);
         return bg_tab;
       }
@@ -1256,15 +1228,10 @@ void BrowserThemeProvider::LoadThemePrefs() {
     }
 
     GenerateFrameColors();
-    // Scope for AutoLock on themed_image_cache.
-    {
-      AutoLock lock(themed_image_cache_lock_);
-      GenerateFrameImages();
-      GenerateTabImages();
-    }
-
-    if (process_images_) {
-      WriteImagesToDisk();
+    GenerateFrameImages();
+    GenerateTabImages();
+    if (process_images_ && WriteImagesToDisk()) {
+      SaveCachedImageData();
       UserMetrics::RecordAction(L"Migrated noncached to cached theme.",
                                 profile_);
     }
@@ -1275,18 +1242,8 @@ void BrowserThemeProvider::LoadThemePrefs() {
 void BrowserThemeProvider::ClearCaches() {
   FreePlatformCaches();
   for (ImageCache::iterator i = image_cache_.begin();
-    i != image_cache_.end(); i++) {
-      delete i->second;
-  }
-
-  // Scope for AutoLock on themed_image_cache.
-  {
-    AutoLock lock(themed_image_cache_lock_);
-    for (ImageCache::iterator i = themed_image_cache_.begin();
-         i != themed_image_cache_.end(); i++) {
-      delete i->second;
-    }
-    themed_image_cache_.clear();
+       i != image_cache_.end(); i++) {
+    delete i->second;
   }
   image_cache_.clear();
   images_disk_cache_.clear();
diff --git a/chrome/browser/browser_theme_provider.h b/chrome/browser/browser_theme_provider.h
index d6c20b6..e2059a8 100644
--- a/chrome/browser/browser_theme_provider.h
+++ b/chrome/browser/browser_theme_provider.h
@@ -13,14 +13,12 @@
 #include "app/resource_bundle.h"
 #include "app/theme_provider.h"
 #include "base/basictypes.h"
-#include "base/lock.h"
 #include "base/non_thread_safe.h"
 #include "base/ref_counted.h"
 
 class Extension;
 class Profile;
 class DictionaryValue;
-class PrefService;
 
 class BrowserThemeProvider : public NonThreadSafe,
                              public ThemeProvider {
@@ -205,20 +203,6 @@ class BrowserThemeProvider : public NonThreadSafe,
   // Parse tiling values from something like "no-repeat" into a Tiling value.
   static int StringToTiling(const std::string &tiling);
 
-  // Lock on write to themed_image_cache_ in UI thread; lock on all cache
-  // access in File thread. This allows the File thread and UI thread to
-  // both read themed images at the same time, while preventing simultaneous
-  // File thread read and UI thread write.
-  static Lock themed_image_cache_lock_;
-
-  // Save the images to be written to disk, mapping file path to id.
-  typedef std::map<FilePath, int> ImagesDiskCache;
-
-  // Cached images. We cache all retrieved and generated bitmaps and keep
-  // track of the pointers. We own these and will delete them when we're done
-  // using them.
-  typedef std::map<int, SkBitmap*> ImageCache;
-
  protected:
   // Sets an individual color value.
   void SetColor(const char* id, const SkColor& color);
@@ -313,7 +297,7 @@ class BrowserThemeProvider : public NonThreadSafe,
   void SetDisplayPropertyData(DictionaryValue* display_properties);
 
   // Create any images that aren't pregenerated (e.g. background tab images).
-  SkBitmap* GenerateTabBackgroundBitmap(int id);
+  SkBitmap* GenerateBitmap(int id);
 
   // Save our data - when saving images we need the original dictionary
   // from the extension because it contains the text ids that we want to save.
@@ -332,7 +316,7 @@ class BrowserThemeProvider : public NonThreadSafe,
   void ClearCaches();
 
   // Encode image at image_cache_[id] as PNG and write to disk.
-  void WriteImagesToDisk();
+  bool WriteImagesToDisk();
 
   // Do we have a custom frame image or custom tints?
   bool ShouldTintFrames();
@@ -342,14 +326,11 @@ class BrowserThemeProvider : public NonThreadSafe,
   GdkPixbuf* GetPixbufImpl(int id, bool rtl_enabled);
 #endif
 
+  // Cached images. We cache all retrieved and generated bitmaps and keep
+  // track of the pointers. We own these and will delete them when we're done
+  // using them.
+  typedef std::map<int, SkBitmap*> ImageCache;
   ImageCache image_cache_;
-
-  // Keep images generated for theme cache in their own place, so we can lock
-  // them on WRITE from UI thread and READ from file thread.  Read from UI
-  // thread will be allowed unlocked, because no other thread has write
-  // access to the cache.
-  ImageCache themed_image_cache_;
-
 #if defined(OS_LINUX)
   typedef std::map<int, GdkPixbuf*> GdkPixbufMap;
   GdkPixbufMap gdk_pixbufs_;
@@ -360,6 +341,8 @@ class BrowserThemeProvider : public NonThreadSafe,
   NSColorMap nscolor_cache_;
 #endif
 
+  // Save the images to be written to disk, mapping file path to id.
+  typedef std::map<FilePath, int> ImagesDiskCache;
   ImagesDiskCache images_disk_cache_;
 
   ResourceBundle& rb_;