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 d59ae0a73bc81ef7fe553538e3b1f7f486cbaf51..81ea2a2f0c9297ae65698091c1749a89ee1a23ec 100644
--- a/chrome/browser/chromeos/login/wallpaper_manager.cc
+++ b/chrome/browser/chromeos/login/wallpaper_manager.cc
@@ -31,6 +31,7 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/login/startup_utils.h"
 #include "chrome/browser/chromeos/login/user.h"
+#include "chrome/browser/chromeos/login/user_image.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/chromeos/login/wizard_controller.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
@@ -75,9 +76,6 @@ const char kNewWallpaperLayoutNodeName[] = "layout";
 const char kNewWallpaperFileNodeName[] = "file";
 const char kNewWallpaperTypeNodeName[] = "type";
 
-// File path suffix of the original custom wallpaper.
-const char kOriginalCustomWallpaperSuffix[] = "_wallpaper";
-
 // Maximum number of wallpapers cached by CacheUsersWallpapers().
 const int kMaxWallpapersToCache = 3;
 
@@ -417,14 +415,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));
 
@@ -433,6 +423,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;
   }
 
@@ -528,7 +520,7 @@ void WallpaperManager::RemoveUserWallpaperInfo(const std::string& user_id) {
 }
 
 bool WallpaperManager::ResizeWallpaper(
-    const UserImage& wallpaper,
+    const gfx::ImageSkia& wallpaper,
     ash::WallpaperLayout layout,
     int preferred_width,
     int preferred_height,
@@ -536,8 +528,8 @@ bool WallpaperManager::ResizeWallpaper(
     gfx::ImageSkia* output_skia) const {
   DCHECK(BrowserThread::GetBlockingPool()->
       IsRunningSequenceOnCurrentThread(sequence_token_));
-  int width = wallpaper.image().width();
-  int height = wallpaper.image().height();
+  int width = wallpaper.width();
+  int height = wallpaper.height();
   int resized_width;
   int resized_height;
   *output = new base::RefCountedBytes();
@@ -567,7 +559,7 @@ bool WallpaperManager::ResizeWallpaper(
   }
 
   gfx::ImageSkia resized_image = gfx::ImageSkiaOperations::CreateResizedImage(
-      wallpaper.image(),
+      wallpaper,
       skia::ImageOperations::RESIZE_LANCZOS3,
       gfx::Size(resized_width, resized_height));
 
@@ -590,7 +582,7 @@ bool WallpaperManager::ResizeWallpaper(
 }
 
 bool WallpaperManager::ResizeAndSaveWallpaper(
-    const UserImage& wallpaper,
+    const gfx::ImageSkia& wallpaper,
     const base::FilePath& path,
     ash::WallpaperLayout layout,
     int preferred_width,
@@ -678,7 +670,7 @@ void WallpaperManager::SetPolicyControlledWallpaper(
                      "policy-controlled.jpeg",
                      ash::WALLPAPER_LAYOUT_CENTER_CROPPED,
                      User::POLICY,
-                     wallpaper,
+                     wallpaper.image(),
                      true /* update wallpaper */);
 }
 
@@ -687,7 +679,7 @@ void WallpaperManager::SetCustomWallpaper(const std::string& user_id,
                                           const std::string& file,
                                           ash::WallpaperLayout layout,
                                           User::WallpaperType type,
-                                          const UserImage& wallpaper,
+                                          const gfx::ImageSkia& wallpaper,
                                           bool update_wallpaper) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(UserManager::Get()->IsUserLoggedIn());
@@ -705,7 +697,7 @@ void WallpaperManager::SetCustomWallpaper(const std::string& user_id,
 
   // If decoded wallpaper is empty, we have probably failed to decode the file.
   // Use default wallpaper in this case.
-  if (wallpaper.image().isNull()) {
+  if (wallpaper.isNull()) {
     SetDefaultWallpaperDelayed(user_id);
     return;
   }
@@ -713,9 +705,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,
@@ -723,21 +712,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.EnsureRepsForSupportedScales();
+    scoped_ptr<gfx::ImageSkia> deep_copy(wallpaper.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,
+                   base::Passed(&deep_copy)));

[oshima, 2014/04/22 00:13:47]

I think deep_copy.Pass() is better now.

[Thiemo Nagel, 2014/04/23 17:14:06]

I have tried that before.  Using deep_copy.Pass() results in a screenful of
template errors, it seems that for some reason a const qualifier is added that
shouldn't be there.  I didn't get to the bottom of it, and followed the example
of the surrounding code by using base::Passed().

[oshima, 2014/04/23 17:34:23]

Didn't you pass &deep_copy.Pass() instead of deep_copy.Pass()?

[Thiemo Nagel, 2014/04/23 17:44:38]

Neither works.  In the case of deep_copy.Pass(), this is what I get:

In file included from ../../base/bind.h:13:0,
                 from ../../base/timer/timer.h:54,
                 from ../../ui/display/chromeos/display_configurator.h:17,
                 from ../../ash/display/display_manager.h:20,
                 from ../../ash/display/display_controller.h:12,
                 from
../../ash/desktop_background/desktop_background_controller.h:9,
                 from
../../chrome/browser/chromeos/login/wallpaper_manager.h:12,
                 from
../../chrome/browser/chromeos/login/wallpaper_manager.cc:5:
../../base/bind_internal.h: In constructor 'base::internal::BindState<Runnable,
RunType, void(P1, P2, P3, P4, P5)>::BindState(const Runnable&, const P1&, const
P2&, const P3&, const P4&, const P5&) [with Runnable =
base::internal::RunnableAdapter<void (chromeos::WallpaperManager::*)(const
std::basic_string<char>&, const base::FilePath&, ash::WallpaperLayout,
scoped_ptr<gfx::ImageSkia>)const>, RunType = void(const
chromeos::WallpaperManager*, const std::basic_string<char>&, const
base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>), P1 =
base::internal::UnretainedWrapper<chromeos::WallpaperManager>, P2 =
std::basic_string<char>, P3 = base::FilePath, P4 = ash::WallpaperLayout, P5 =
scoped_ptr<gfx::ImageSkia>]':
../../base/bind.h:354:73:   instantiated from 'base::Callback<typename
base::internal::BindState<typename
base::internal::FunctorTraits<Functor>::RunnableType, typename
base::internal::FunctorTraits<Functor>::RunType, void(typename
base::internal::CallbackParamTraits<A1>::StorageType, typename
base::internal::CallbackParamTraits<A2>::StorageType, typename
base::internal::CallbackParamTraits<A3>::StorageType, typename
base::internal::CallbackParamTraits<A4>::StorageType, typename
base::internal::CallbackParamTraits<A5>::StorageType)>::UnboundRunType>
base::Bind(Functor, const P1&, const P2&, const P3&, const P4&, const P5&) [with
Functor = void (chromeos::WallpaperManager::*)(const std::basic_string<char>&,
const base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>)const,
P1 = base::internal::UnretainedWrapper<chromeos::WallpaperManager>, P2 =
std::basic_string<char>, P3 = base::FilePath, P4 = ash::WallpaperLayout, P5 =
scoped_ptr<gfx::ImageSkia>, typename base::internal::BindState<typename
base::internal::FunctorTraits<Functor>::RunnableType, typename
base::internal::FunctorTraits<Functor>::RunType, void(typename
base::internal::CallbackParamTraits<A1>::StorageType, typename
base::internal::CallbackParamTraits<A2>::StorageType, typename
base::internal::CallbackParamTraits<A3>::StorageType, typename
base::internal::CallbackParamTraits<A4>::StorageType, typename
base::internal::CallbackParamTraits<A5>::StorageType)>::UnboundRunType =
void()]'
../../chrome/browser/chromeos/login/wallpaper_manager.cc:796:36:   instantiated
from here
../../base/bind_internal.h:2686:15: error: passing 'const
scoped_ptr<gfx::ImageSkia>' as 'this' argument of 'scoped_ptr<T, D>::operator
scoped_ptr<T, D>::RValue() [with T = gfx::ImageSkia, D =
base::DefaultDeleter<gfx::ImageSkia>]' discards qualifiers [-fpermissive]
../../base/bind_internal.h: In static member function 'static void
base::internal::InvokeHelper<false, void, Runnable, void(A1, A2, A3, A4,
A5)>::MakeItSo(Runnable, A1, A2, A3, A4, A5) [with Runnable =
base::internal::RunnableAdapter<void (chromeos::WallpaperManager::*)(const
std::basic_string<char>&, const base::FilePath&, ash::WallpaperLayout,
scoped_ptr<gfx::ImageSkia>)const>, A1 = chromeos::WallpaperManager*, A2 = const
std::basic_string<char>&, A3 = const base::FilePath&, A4 = const
ash::WallpaperLayout&, A5 = const scoped_ptr<gfx::ImageSkia>&]':
../../base/bind_internal.h:1811:60:   instantiated from 'static R
base::internal::Invoker<5, StorageType, R(X1, X2, X3, X4,
X5)>::Run(base::internal::BindStateBase*) [with StorageType =
base::internal::BindState<base::internal::RunnableAdapter<void
(chromeos::WallpaperManager::*)(const std::basic_string<char>&, const
base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>)const>,
void(const chromeos::WallpaperManager*, const std::basic_string<char>&, const
base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>),
void(base::internal::UnretainedWrapper<chromeos::WallpaperManager>,
std::basic_string<char>, base::FilePath, ash::WallpaperLayout,
scoped_ptr<gfx::ImageSkia>)>, R = void, X1 = const chromeos::WallpaperManager*,
X2 = const std::basic_string<char>&, X3 = const base::FilePath&, X4 =
ash::WallpaperLayout, X5 = scoped_ptr<gfx::ImageSkia>]'
../../base/callback.h:389:28:   instantiated from
'base::Callback<R()>::Callback(base::internal::BindState<Runnable, BindRunType,
BoundArgsType>*) [with Runnable = base::internal::RunnableAdapter<void
(chromeos::WallpaperManager::*)(const std::basic_string<char>&, const
base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>)const>,
BindRunType = void(const chromeos::WallpaperManager*, const
std::basic_string<char>&, const base::FilePath&, ash::WallpaperLayout,
scoped_ptr<gfx::ImageSkia>), BoundArgsType =
void(base::internal::UnretainedWrapper<chromeos::WallpaperManager>,
std::basic_string<char>, base::FilePath, ash::WallpaperLayout,
scoped_ptr<gfx::ImageSkia>), R = void]'
../../base/bind.h:354:73:   instantiated from 'base::Callback<typename
base::internal::BindState<typename
base::internal::FunctorTraits<Functor>::RunnableType, typename
base::internal::FunctorTraits<Functor>::RunType, void(typename
base::internal::CallbackParamTraits<A1>::StorageType, typename
base::internal::CallbackParamTraits<A2>::StorageType, typename
base::internal::CallbackParamTraits<A3>::StorageType, typename
base::internal::CallbackParamTraits<A4>::StorageType, typename
base::internal::CallbackParamTraits<A5>::StorageType)>::UnboundRunType>
base::Bind(Functor, const P1&, const P2&, const P3&, const P4&, const P5&) [with
Functor = void (chromeos::WallpaperManager::*)(const std::basic_string<char>&,
const base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>)const,
P1 = base::internal::UnretainedWrapper<chromeos::WallpaperManager>, P2 =
std::basic_string<char>, P3 = base::FilePath, P4 = ash::WallpaperLayout, P5 =
scoped_ptr<gfx::ImageSkia>, typename base::internal::BindState<typename
base::internal::FunctorTraits<Functor>::RunnableType, typename
base::internal::FunctorTraits<Functor>::RunType, void(typename
base::internal::CallbackParamTraits<A1>::StorageType, typename
base::internal::CallbackParamTraits<A2>::StorageType, typename
base::internal::CallbackParamTraits<A3>::StorageType, typename
base::internal::CallbackParamTraits<A4>::StorageType, typename
base::internal::CallbackParamTraits<A5>::StorageType)>::UnboundRunType =
void()]'
../../chrome/browser/chromeos/login/wallpaper_manager.cc:796:36:   instantiated
from here
../../base/bind_internal.h:991:5: error: passing 'base::enable_if<true, const
scoped_ptr<gfx::ImageSkia> >::type {aka const scoped_ptr<gfx::ImageSkia>}' as
'this' argument of 'scoped_ptr<T, D>::operator scoped_ptr<T, D>::RValue() [with
T = gfx::ImageSkia, D = base::DefaultDeleter<gfx::ImageSkia>]' discards
qualifiers [-fpermissive]
../../base/bind_internal.h:398:5: error:   initializing argument 5 of 'R
base::internal::RunnableAdapter<R (T::*)(A1, A2, A3, A4)const>::Run(const T*,
typename base::internal::CallbackParamTraits<A2>::ForwardType, typename
base::internal::CallbackParamTraits<A3>::ForwardType, typename
base::internal::CallbackParamTraits<A4>::ForwardType, typename
base::internal::CallbackParamTraits<A5>::ForwardType) [with R = void, T =
chromeos::WallpaperManager, A1 = const std::basic_string<char>&, A2 = const
base::FilePath&, A3 = ash::WallpaperLayout, A4 = scoped_ptr<gfx::ImageSkia>,
typename base::internal::CallbackParamTraits<A2>::ForwardType = const
std::basic_string<char>&, typename
base::internal::CallbackParamTraits<A3>::ForwardType = const base::FilePath&,
typename base::internal::CallbackParamTraits<A4>::ForwardType = const
ash::WallpaperLayout&, typename
base::internal::CallbackParamTraits<A5>::ForwardType =
scoped_ptr<gfx::ImageSkia>]'

+  }
 
   std::string relative_path = base::FilePath(user_id_hash).Append(file).value();
   // User's custom wallpaper path is determined by relative path and the
@@ -751,11 +744,11 @@ void WallpaperManager::SetCustomWallpaper(const std::string& user_id,
   SetUserWallpaperInfo(user_id, info, is_persistent);
   if (update_wallpaper) {
     GetPendingWallpaper(user_id, false)
-        ->ResetSetWallpaperImage(wallpaper.image(), info);
+        ->ResetSetWallpaperImage(wallpaper, info);
   }
 
   if (UserManager::IsMultipleProfilesAllowed())
-    wallpaper_cache_[user_id] = wallpaper.image();
+    wallpaper_cache_[user_id] = wallpaper;
 }
 
 void WallpaperManager::SetDefaultWallpaperNow(const std::string& user_id) {
@@ -850,11 +843,6 @@ void WallpaperManager::SetUserWallpaperInfo(const std::string& user_id,
   wallpaper_update->SetWithoutPathExpansion(user_id, wallpaper_info_dict);
 }
 
-void WallpaperManager::SetLastSelectedUser(

[Nikita (slow), 2014/04/18 09:28:19]

I guess this was previously used for tests, maybe not.
Not anymore.

-    const std::string& last_selected_user) {
-  last_selected_user_ = last_selected_user;
-}
-
 void WallpaperManager::SetUserWallpaperDelayed(const std::string& user_id) {
   ScheduleSetUserWallpaper(user_id, true);
 }
@@ -884,7 +872,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;
 
@@ -1048,6 +1036,7 @@ void WallpaperManager::ClearObsoleteWallpaperPrefs() {
   wallpapers_pref->Clear();
 }
 
+// static
 void WallpaperManager::DeleteAllExcept(const base::FilePath& path) {

[Nikita (slow), 2014/04/18 09:28:19]

nit: This function seems to be used only in this file.
How about moving it to unnamed namespace instead?

[Thiemo Nagel, 2014/04/23 17:14:06]

Done.

   base::FilePath dir = path.DirName();
   if (base::DirectoryExists(dir)) {
@@ -1060,6 +1049,7 @@ void WallpaperManager::DeleteAllExcept(const base::FilePath& path) {
   }
 }
 
+// static
 void WallpaperManager::DeleteWallpaperInList(

[Nikita (slow), 2014/04/18 09:28:19]

nit: This function seems to be used only in this file.
How about moving it to unnamed namespace instead?

[Thiemo Nagel, 2014/04/23 17:14:06]

Done.

     const std::vector<base::FilePath>& file_list) {
   for (std::vector<base::FilePath>::const_iterator it = file_list.begin();
@@ -1106,11 +1096,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(

[Nikita (slow), 2014/04/18 09:28:19]

nit: This function seems to be used only in this file.
How about moving it to unnamed namespace instead?

[Thiemo Nagel, 2014/04/23 17:14:06]

Done.

     const std::string& user_id_hash) {
   base::FilePath dir;
@@ -1396,25 +1386,11 @@ void WallpaperManager::OnWallpaperDecoded(
   }
 }
 
-void WallpaperManager::ProcessCustomWallpaper(
+void WallpaperManager::SaveCustomWallpaper(
     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 base::FilePath& original_path,
+    ash::WallpaperLayout layout,
+    scoped_ptr<gfx::ImageSkia> wallpaper) const {
   DCHECK(BrowserThread::GetBlockingPool()->
       IsRunningSequenceOnCurrentThread(sequence_token_));
   EnsureCustomWallpaperDirectories(user_id_hash);
@@ -1427,22 +1403,22 @@ void WallpaperManager::SaveCustomWallpaper(const std::string& user_id_hash,
   // Re-encode orginal file to jpeg format and saves the result in case that
   // resized wallpaper is not generated (i.e. chrome shutdown before resized
   // wallpaper is saved).
-  ResizeAndSaveWallpaper(wallpaper,
+  ResizeAndSaveWallpaper(*wallpaper,
                          original_path,
                          ash::WALLPAPER_LAYOUT_STRETCH,
-                         wallpaper.image().width(),
-                         wallpaper.image().height(),
+                         wallpaper->width(),
+                         wallpaper->height(),
                          NULL);
   DeleteAllExcept(original_path);
 
-  ResizeAndSaveWallpaper(wallpaper,
+  ResizeAndSaveWallpaper(*wallpaper,
                          small_wallpaper_path,
                          layout,
                          kSmallWallpaperMaxWidth,
                          kSmallWallpaperMaxHeight,
                          NULL);
   DeleteAllExcept(small_wallpaper_path);
-  ResizeAndSaveWallpaper(wallpaper,
+  ResizeAndSaveWallpaper(*wallpaper,
                          large_wallpaper_path,
                          layout,
                          kLargeWallpaperMaxWidth,