Some cleanup of WallpaperManager.

chrome/browser/chromeos/login/wallpaper_manager.cc

 // Copyright (c) 2013 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 "chrome/browser/chromeos/login/wallpaper_manager.h"
 
 #include <numeric>
 #include <vector>
 
 #include "ash/shell.h"
@@ skipping 387 matching lines @@
 }
 
 base::FilePath WallpaperManager::GetCustomWallpaperPath(
     const char* sub_dir,
     const std::string& user_id_hash,
     const std::string& file) const {
   base::FilePath custom_wallpaper_path = GetCustomWallpaperDir(sub_dir);
   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));
 
   if (UserManager::Get()->IsLoggedInAsStub()) {
     info->file = current_user_wallpaper_info_.file = "";
     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;
   }
 
   return GetUserWallpaperInfo(UserManager::Get()->GetLoggedInUser()->email(),
                               info);
 }
 
 void WallpaperManager::InitializeWallpaper() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   UserManager* user_manager = UserManager::Get();
@@ skipping 235 matching lines @@
   // If decoded wallpaper is empty, we have probably failed to decode the file.
   // Use default wallpaper in this case.
   if (wallpaper.image().isNull()) {
     SetDefaultWallpaperDelayed(user_id);
     return;
   }
 
   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,
       type,
       // 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
   // appropriate wallpaper resolution in GetCustomWallpaperInternal.
   WallpaperInfo info = {
       relative_path,
       layout,
       type,
       base::Time::Now().LocalMidnight()
   };
@@ skipping 58 matching lines @@
 
   base::DictionaryValue* wallpaper_info_dict = new base::DictionaryValue();
   wallpaper_info_dict->SetString(kNewWallpaperDateNodeName,
       base::Int64ToString(info.date.ToInternalValue()));
   wallpaper_info_dict->SetString(kNewWallpaperFileNodeName, info.file);
   wallpaper_info_dict->SetInteger(kNewWallpaperLayoutNodeName, info.layout);
   wallpaper_info_dict->SetInteger(kNewWallpaperTypeNodeName, info.type);
   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);
 }
 
 void WallpaperManager::SetUserWallpaperNow(const std::string& user_id) {
   ScheduleSetUserWallpaper(user_id, false);
 }
 
 void WallpaperManager::ScheduleSetUserWallpaper(const std::string& user_id,
                                                 bool delayed) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // Some unit tests come here without a UserManager or without a pref system.
   if (!UserManager::IsInitialized() || !g_browser_process->local_state())
     return;
   // There is no visible background in kiosk mode.
   if (UserManager::Get()->IsLoggedInAsKioskApp())
     return;
   // Guest user, regular user in ephemeral mode, or kiosk app.
   const User* user = UserManager::Get()->FindUser(user_id);
   if (UserManager::Get()->IsUserNonCryptohomeDataEphemeral(user_id) ||
       (user != NULL && user->GetType() == User::USER_TYPE_KIOSK_APP)) {
     InitInitialUserWallpaper(user_id, false);
     GetPendingWallpaper(user_id, delayed)->ResetSetDefaultWallpaper();
     return;
   }
 
   if (!UserManager::Get()->IsKnownUser(user_id))
     return;
 
-  SetLastSelectedUser(user_id);
+  last_selected_user_ = user_id;
 
   WallpaperInfo info;
 
   if (!GetUserWallpaperInfo(user_id, &info)) {
     InitInitialUserWallpaper(user_id, true);
     GetUserWallpaperInfo(user_id, &info);
   }
 
   gfx::ImageSkia user_wallpaper;
   current_user_wallpaper_info_ = info;
@@ skipping 152 matching lines @@
 
 void WallpaperManager::ClearObsoleteWallpaperPrefs() {
   PrefService* prefs = g_browser_process->local_state();
   DictionaryPrefUpdate wallpaper_properties_pref(prefs,
       kUserWallpapersProperties);
   wallpaper_properties_pref->Clear();
   DictionaryPrefUpdate wallpapers_pref(prefs, kUserWallpapers);
   wallpapers_pref->Clear();
 }
 
+// static
 void WallpaperManager::DeleteAllExcept(const base::FilePath& path) {
   base::FilePath dir = path.DirName();
   if (base::DirectoryExists(dir)) {
     base::FileEnumerator files(dir, false, base::FileEnumerator::FILES);
     for (base::FilePath current = files.Next(); !current.empty();
          current = files.Next()) {
       if (current != path)
         base::DeleteFile(current, false);
     }
   }
 }
 
+// static
 void WallpaperManager::DeleteWallpaperInList(
     const std::vector<base::FilePath>& file_list) {
   for (std::vector<base::FilePath>::const_iterator it = file_list.begin();
        it != file_list.end(); ++it) {
     base::FilePath path = *it;
     // Some users may still have legacy wallpapers with png extension. We need
     // to delete these wallpapers too.
     if (!base::DeleteFile(path, true) &&
         !base::DeleteFile(path.AddExtension(".png"), false)) {
       LOG(ERROR) << "Failed to remove user wallpaper at " << path.value();
@@ skipping 26 matching lines @@
 
   // Remove original user wallpaper.
   wallpaper_path = GetCustomWallpaperDir(kOriginalWallpaperSubDir);
   file_to_remove.push_back(wallpaper_path.Append(user_id));
   wallpaper_path = wallpaper_path.Append(path_to_file);
   file_to_remove.push_back(wallpaper_path);
 
   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;
   dir = GetCustomWallpaperDir(kSmallWallpaperSubDir);
   dir = dir.Append(user_id_hash);
   if (!base::PathExists(dir))
     base::CreateDirectory(dir);
   dir = GetCustomWallpaperDir(kLargeWallpaperSubDir);
   dir = dir.Append(user_id_hash);
   if (!base::PathExists(dir))
@@ skipping 257 matching lines @@
       UserManager::IsMultipleProfilesAllowed()) {
     wallpaper_cache_[user_id] = wallpaper.image();
   }
 
   if (update_wallpaper) {
     ash::Shell::GetInstance()->desktop_background_controller()->
         SetCustomWallpaper(wallpaper.image(), layout);
   }
 }
 
-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);
   std::string file_name = original_path.BaseName().value();
   base::FilePath small_wallpaper_path =
       GetCustomWallpaperPath(kSmallWallpaperSubDir, user_id_hash, file_name);
   base::FilePath large_wallpaper_path =
       GetCustomWallpaperPath(kLargeWallpaperSubDir, user_id_hash, file_name);
 
   // Re-encode orginal file to jpeg format and saves the result in case that
@@ skipping 91 matching lines @@
     loading_.push_back(new WallpaperManager::PendingWallpaper(
         (delayed ? GetWallpaperLoadDelay()
                  : base::TimeDelta::FromMilliseconds(0)),
         user_id));
     pending_inactive_ = loading_.back();
   }
   return pending_inactive_;
 }
 
 }  // namespace chromeos