Some cleanup of WallpaperManager.

chrome/browser/chromeos/login/wallpaper_manager.cc

Index: chrome/browser/chromeos/login/wallpaper_manager.cc
diff --git a/chrome/browser/chromeos/login/wallpaper_manager.cc b/chrome/browser/chromeos/login/wallpaper_manager.cc
index a036ca9064fdbf43a1a63d07eba4e94046c9fc0f..7b48019d44e8e18d77f5723bba5db28602f98c55 100644
--- a/chrome/browser/chromeos/login/wallpaper_manager.cc
+++ b/chrome/browser/chromeos/login/wallpaper_manager.cc
@@ -405,14 +405,6 @@ base::FilePath WallpaperManager::GetCustomWallpaperPath(
   return custom_wallpaper_path.Append(user_id_hash).Append(file);
 }
 
-base::FilePath WallpaperManager::GetOriginalWallpaperPathForUser(
-    const std::string& user_id) {
-  std::string filename = user_id + kOriginalCustomWallpaperSuffix;
-  base::FilePath user_data_dir;
-  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
-  return user_data_dir.AppendASCII(filename);
-}
-
 bool WallpaperManager::GetLoggedInUserWallpaperInfo(WallpaperInfo* info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -421,6 +413,8 @@ bool WallpaperManager::GetLoggedInUserWallpaperInfo(WallpaperInfo* info) {
     info->layout = current_user_wallpaper_info_.layout =
         ash::WALLPAPER_LAYOUT_CENTER_CROPPED;
     info->type = current_user_wallpaper_info_.type = User::DEFAULT;
+    info->date = current_user_wallpaper_info_.date =
+        base::Time::Now().LocalMidnight();
     return true;
   }
 
@@ -676,9 +670,6 @@ void WallpaperManager::SetCustomWallpaper(const std::string& user_id,
   bool is_persistent =
       !UserManager::Get()->IsUserNonCryptohomeDataEphemeral(user_id);
 
-  wallpaper.image().EnsureRepsForSupportedScales();
-  scoped_ptr<gfx::ImageSkia> deep_copy(wallpaper.image().DeepCopy());
-
   WallpaperInfo wallpaper_info = {
       wallpaper_path.value(),
       layout,
@@ -686,21 +677,25 @@ void WallpaperManager::SetCustomWallpaper(const std::string& user_id,
       // Date field is not used.
       base::Time::Now().LocalMidnight()
   };
-  // Block shutdown on this task. Otherwise, we may lose the custom wallpaper
-  // that the user selected.
-  scoped_refptr<base::SequencedTaskRunner> blocking_task_runner =
-      BrowserThread::GetBlockingPool()->
-          GetSequencedTaskRunnerWithShutdownBehavior(sequence_token_,
-              base::SequencedWorkerPool::BLOCK_SHUTDOWN);
-  // TODO(bshe): This may break if RawImage becomes RefCountedMemory.
-  blocking_task_runner->PostTask(FROM_HERE,
-      base::Bind(&WallpaperManager::ProcessCustomWallpaper,
-                 base::Unretained(this),
-                 user_id_hash,
-                 is_persistent,
-                 wallpaper_info,
-                 base::Passed(&deep_copy),
-                 wallpaper.raw_image()));
+  if (is_persistent) {
+    wallpaper.image().EnsureRepsForSupportedScales();
+    scoped_ptr<gfx::ImageSkia> deep_copy(wallpaper.image().DeepCopy());
+    // Block shutdown on this task. Otherwise, we may lose the custom wallpaper
+    // that the user selected.
+    scoped_refptr<base::SequencedTaskRunner> blocking_task_runner =
+        BrowserThread::GetBlockingPool()->
+            GetSequencedTaskRunnerWithShutdownBehavior(
+                sequence_token_, base::SequencedWorkerPool::BLOCK_SHUTDOWN);
+    // TODO(bshe): This may break if RawImage becomes RefCountedMemory.
+    blocking_task_runner->PostTask(
+        FROM_HERE,
+        base::Bind(&WallpaperManager::SaveCustomWallpaper,
+                   base::Unretained(this),
+                   user_id_hash,
+                   base::FilePath(wallpaper_info.file),
+                   wallpaper_info.layout,
+                   UserImage(*deep_copy, wallpaper.raw_image())));

[bshe, 2014/04/02 15:11:10]

the ownership of deep_copy is not passed to the |SaveCustomWallpaper|. It may
run into problems as ImageSkia is not thread safe.

[Thiemo Nagel, 2014/04/02 15:39:56]

I don't understand what you're concerned about.  Could you please elaborate?  In
which way is my handling of the deep_copy different from the existing code?

To my understanding, ImageSkia is an "empty hull" and only contains pointers to
the data, which is possibly shared with other ImageSkia instances.  By calling
DeepCopy(), an new ImageSkia is generated that has pointers to a fresh set of
image data.  This is handed over to the worker thread.  How can that cause
troubles with thread safety?

And wallpaper.raw_image() is copied inside the UserImage() constructor, thus I
don't see a problem there, either.

But maybe I'm overlooking something?  Please explain!

[bshe, 2014/04/02 16:32:19]

+oshima

I remember it could cause problem when try to modify the ref count of the image
data on different threads. Base::Pass will pass the ownership of the deep copy
object to worker thread. So when deep_copy is out of scope, we wont try to
decrease ref count. But with the current approach, we will try to decrease the
ref count. And we essentially try to Add/Remove ref counts on two different
threads on a non thread safe object.

https://chromiumcodereview.appspot.com/11366143 should have more details.

On 2014/04/02 15:39:56, Thiemo Nagel wrote:

[Thiemo Nagel, 2014/04/09 16:00:24]

Thanks a lot for the explanation!  I've fixed this by following the example of
wallpaper_api.cc.

+  }
 
   std::string relative_path = base::FilePath(user_id_hash).Append(file).value();
   // User's custom wallpaper path is determined by relative path and the
@@ -779,11 +774,6 @@ void WallpaperManager::SetUserWallpaperInfo(const std::string& user_id,
   wallpaper_update->SetWithoutPathExpansion(user_id, wallpaper_info_dict);
 }
 
-void WallpaperManager::SetLastSelectedUser(
-    const std::string& last_selected_user) {
-  last_selected_user_ = last_selected_user;
-}
-
 void WallpaperManager::SetUserWallpaperDelayed(const std::string& user_id) {
   ScheduleSetUserWallpaper(user_id, true);
 }
@@ -813,7 +803,7 @@ void WallpaperManager::ScheduleSetUserWallpaper(const std::string& user_id,
   if (!UserManager::Get()->IsKnownUser(user_id))
     return;
 
-  SetLastSelectedUser(user_id);
+  last_selected_user_ = user_id;
 
   WallpaperInfo info;
 
@@ -986,6 +976,7 @@ void WallpaperManager::ClearObsoleteWallpaperPrefs() {
   wallpapers_pref->Clear();
 }
 
+// static
 void WallpaperManager::DeleteAllExcept(const base::FilePath& path) {
   base::FilePath dir = path.DirName();
   if (base::DirectoryExists(dir)) {
@@ -998,6 +989,7 @@ void WallpaperManager::DeleteAllExcept(const base::FilePath& path) {
   }
 }
 
+// static
 void WallpaperManager::DeleteWallpaperInList(
     const std::vector<base::FilePath>& file_list) {
   for (std::vector<base::FilePath>::const_iterator it = file_list.begin();
@@ -1044,11 +1036,11 @@ void WallpaperManager::DeleteUserWallpapers(const std::string& user_id,
   base::WorkerPool::PostTask(
       FROM_HERE,
       base::Bind(&WallpaperManager::DeleteWallpaperInList,
-                 base::Unretained(this),
                  file_to_remove),
       false);
 }
 
+// static
 void WallpaperManager::EnsureCustomWallpaperDirectories(
     const std::string& user_id_hash) {
   base::FilePath dir;
@@ -1326,25 +1318,10 @@ void WallpaperManager::OnWallpaperDecoded(
   }
 }
 
-void WallpaperManager::ProcessCustomWallpaper(
-    const std::string& user_id_hash,
-    bool persistent,
-    const WallpaperInfo& info,
-    scoped_ptr<gfx::ImageSkia> image,
-    const UserImage::RawImage& raw_image) {
-  DCHECK(BrowserThread::GetBlockingPool()->
-      IsRunningSequenceOnCurrentThread(sequence_token_));
-  UserImage wallpaper(*image.get(), raw_image);
-  if (persistent) {
-    SaveCustomWallpaper(user_id_hash, base::FilePath(info.file), info.layout,
-                        wallpaper);
-  }
-}
-
 void WallpaperManager::SaveCustomWallpaper(const std::string& user_id_hash,
                                            const base::FilePath& original_path,
                                            ash::WallpaperLayout layout,
-                                           const UserImage& wallpaper) {
+                                           const UserImage& wallpaper) const {
   DCHECK(BrowserThread::GetBlockingPool()->
       IsRunningSequenceOnCurrentThread(sequence_token_));
   EnsureCustomWallpaperDirectories(user_id_hash);