Update profile desktop shortcuts

chrome/browser/profiles/profile_shortcut_manager_win.cc

Index: chrome/browser/profiles/profile_shortcut_manager_win.cc
===================================================================
--- chrome/browser/profiles/profile_shortcut_manager_win.cc	(revision 152316)
+++ chrome/browser/profiles/profile_shortcut_manager_win.cc	(working copy)
@@ -15,7 +15,7 @@
 #include "chrome/browser/app_icon_win.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/prefs/pref_service.h"
-#include "chrome/browser/profiles/profile_info_cache.h"
+#include "chrome/browser/profiles/profile_info_cache_observer.h"
 #include "chrome/browser/profiles/profile_info_util.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/chrome_constants.h"
@@ -48,9 +48,8 @@
 // Use index 0 when assigning the resulting file as the icon.
 FilePath CreateChromeDesktopShortcutIconForProfile(
     const FilePath& profile_path,
-    const gfx::Image& avatar_image) {
+    const SkBitmap& avatar_bitmap) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  const SkBitmap* avatar_bitmap = avatar_image.ToSkBitmap();
   HICON app_icon_handle = GetAppIconForSize(kShortcutIconSize);
   scoped_ptr<SkBitmap> app_icon_bitmap(
       IconUtil::CreateSkBitmapFromHICON(app_icon_handle));
@@ -62,16 +61,16 @@
   // avatar_menu_button::DrawTaskBarDecoration.
   const SkBitmap* source_bitmap = NULL;
   SkBitmap squarer_bitmap;
-  if ((avatar_bitmap->width() == profiles::kAvatarIconWidth) &&
-      (avatar_bitmap->height() == profiles::kAvatarIconHeight)) {
+  if ((avatar_bitmap.width() == profiles::kAvatarIconWidth) &&
+      (avatar_bitmap.height() == profiles::kAvatarIconHeight)) {
     // Shave a couple of columns so the bitmap is more square. So when
     // resized to a square aspect ratio it looks pretty.
     int x = 2;
-    avatar_bitmap->extractSubset(&squarer_bitmap, SkIRect::MakeXYWH(x, 0,
+    avatar_bitmap.extractSubset(&squarer_bitmap, SkIRect::MakeXYWH(x, 0,
         profiles::kAvatarIconWidth - x * 2, profiles::kAvatarIconHeight));
     source_bitmap = &squarer_bitmap;
   } else {
-    source_bitmap = avatar_bitmap;
+    source_bitmap = &avatar_bitmap;
   }
   SkBitmap sk_icon = skia::ImageOperations::Resize(
       *source_bitmap,
@@ -115,9 +114,90 @@
   bool_function.Run();
 }
 
+// Renames an existing Chrome desktop profile shortcut. Must be called on the
+// FILE thread.
+void RenameChromeDesktopShortcutForProfile(
+    const string16& old_shortcut,

[sail, 2012/08/23 18:46:44]

variable names could be more descriptive (i.e. shortcut name, shortcut file
name, shortcut directory, etc...)

[Halli, 2012/08/24 02:56:49]

Done.

+    const string16& new_shortcut) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  FilePath shortcut_path;
+  if (ShellUtil::GetDesktopPath(false,  // User's directory instead of system.
+                                &shortcut_path)) {
+    FilePath old_profile = shortcut_path.Append(old_shortcut);
+    FilePath new_profile = shortcut_path.Append(new_shortcut);
+    file_util::Move(old_profile, new_profile);

[sail, 2012/08/23 18:46:44]

if this fails can you log the error

[Halli, 2012/08/24 02:56:49]

Done.

+  }
+}
+
+// Updates the arguments to a Chrome desktop shortcut for a profile. Must be

[sail, 2012/08/23 18:46:44]

Comment doesn't match the function.

[Halli, 2012/08/24 02:56:49]

n/a function has been removed
On 2012/08/23 18:46:44, sail wrote:

+// called on the FILE thread.
+void UpdateChromeDesktopShortcutForProfile(

[gab, 2012/08/23 18:32:39]

You do not need a whole new function to do this, simply change
CreateChromeDesktopShortcutForProfile to
CreateOrUpdateChromeDesktopShortcutForProfile and have it take a boolean
"create" or something, than based on "create", pass in the
"ShellUtil::SHORTCUT_CREATE_ALWAYS" property to CreateChromeDesktopShortcut or
not (I know the name is confusing, but I'm changing all of this in upcoming
http://codereview.chromium.org/10836247/ and doing it this way will be easier to
merge with my CL).

[Halli, 2012/08/24 02:56:49]

Done.

+    const string16& shortcut,

[sail, 2012/08/23 18:46:44]

same as above, need more descriptive name

[Halli, 2012/08/24 02:56:49]

n/a function has been removed
On 2012/08/23 18:46:44, sail wrote:

+    const string16& arguments,

[sail, 2012/08/23 18:46:44]

same, maybe exe_arguments?

[Halli, 2012/08/24 02:56:49]

n/a function has been removed
On 2012/08/23 18:46:44, sail wrote:

+    const FilePath& profile_path,
+    const SkBitmap& avatar_bitmap) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  FilePath shortcut_path;
+  if (!ShellUtil::GetDesktopPath(false, &shortcut_path))
+    return;
+
+  shortcut_path = shortcut_path.Append(shortcut);
+  FilePath chrome_exe;
+  if (!PathService::Get(base::FILE_EXE, &chrome_exe))
+    return;
+  BrowserDistribution* dist = BrowserDistribution::GetDistribution();
+  string16 description;
+  if (!dist)
+    return;
+  else
+    description = WideToUTF16(dist->GetAppDescription());
+  FilePath icon_path = avatar_bitmap.isNull() ?

[sail, 2012/08/23 18:46:44]

when can the avatar bitmap be null? can this be changed to a DCHECK() instead?

[Halli, 2012/08/24 02:56:49]

It should be DCHECK(!avatar_image.empty()) before calling ToSkBitmap() and then
it shouldn't be null. Fixed for the create version of this and removed this
function.
On 2012/08/23 18:46:44, sail wrote:

+      FilePath() :
+      CreateChromeDesktopShortcutIconForProfile(profile_path, avatar_bitmap);
+
+  ShellUtil::UpdateChromeShortcut(
+      dist,
+      chrome_exe.value(),
+      shortcut_path.value(),
+      arguments,
+      description,
+      icon_path.empty() ? chrome_exe.value() : icon_path.value(),
+      icon_path.empty() ? dist->GetIconIndex() : 0,
+      ShellUtil::SHORTCUT_NO_OPTIONS);
+}
+
+void CompleteProfileShortcutCreation(

[sail, 2012/08/23 18:46:44]

function level comments.
also function name is not very descriptive 

[Halli, 2012/08/24 02:56:49]

Done.

+    const FilePath& profile_path, const string16& profile_name,

[sail, 2012/08/23 18:46:44]

one line per parameter would be easier to read

[Halli, 2012/08/24 02:56:49]

Done.

+    const SkBitmap& avatar_image) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  FilePath shortcut_icon = CreateChromeDesktopShortcutIconForProfile(
+    profile_path, avatar_image);
+
+  FilePath chrome_exe;
+  if (!PathService::Get(base::FILE_EXE, &chrome_exe))
+    return;
+  BrowserDistribution* dist = BrowserDistribution::GetDistribution();
+  string16 description;

[sail, 2012/08/23 18:46:44]

variable declaration can be combined with assignment below

[Halli, 2012/08/24 02:56:49]

Done.

+  if (!dist)
+    return;
+  description = WideToUTF16(dist->GetAppDescription());
+
+  ShellUtil::CreateChromeDesktopShortcut(
+      dist,
+      chrome_exe.value(),
+      description,
+      profile_name,
+      CreateProfileShortcutFlags(profile_path),
+      shortcut_icon.empty() ? chrome_exe.value() : shortcut_icon.value(),
+      shortcut_icon.empty() ? dist->GetIconIndex() : 0,
+      ShellUtil::CURRENT_USER,
+      ShellUtil::SHORTCUT_CREATE_ALWAYS);
+}
+
 }  // namespace
 
-class ProfileShortcutManagerWin : public ProfileShortcutManager {
+class ProfileShortcutManagerWin : public ProfileShortcutManager,
+                                  public ProfileInfoCacheObserver {
  public:
   ProfileShortcutManagerWin();
   virtual ~ProfileShortcutManagerWin();
@@ -125,34 +205,17 @@
   virtual void CreateChromeDesktopShortcut(
       const FilePath& profile_path, const string16& profile_name,
       const gfx::Image& avatar_image) OVERRIDE;
-  virtual void DeleteChromeDesktopShortcut(const FilePath& profile_path)
-      OVERRIDE;
+  virtual void DeleteChromeDesktopShortcut(const FilePath& profile_path,
+      const string16& profile_name) OVERRIDE;
 
- private:
-  struct ProfileShortcutInfo {
-    string16 flags;
-    string16 profile_name;
-    gfx::Image avatar_image;
+  virtual void OnProfileAdded(const FilePath& profile_path) OVERRIDE;
+  virtual void OnProfileWillBeRemoved(const FilePath& profile_path) OVERRIDE;
+  virtual void OnProfileWasRemoved(const FilePath& profile_path,
+                                   const string16& profile_name) OVERRIDE;
+  virtual void OnProfileNameChanged(const FilePath& profile_path,
+                                    const string16& old_profile_name) OVERRIDE;
+  virtual void OnProfileAvatarChanged(const FilePath& profile_path) OVERRIDE;
 
-    ProfileShortcutInfo()
-        : flags(string16()),
-          profile_name(string16()),
-          avatar_image(gfx::Image()) {
-    }
-
-    ProfileShortcutInfo(
-        string16 p_flags,
-        string16 p_profile_name,
-        gfx::Image p_avatar_image)
-        : flags(p_flags),
-          profile_name(p_profile_name),
-          avatar_image(p_avatar_image) {
-    }
-  };
-
-  // TODO(hallielaine): Repopulate this map on chrome session startup
-  typedef std::map<FilePath, ProfileShortcutInfo> ProfileShortcutsMap;
-  ProfileShortcutsMap profile_shortcuts_;
 };
 
 // static
@@ -165,8 +228,11 @@
 }
 
 // static
-ProfileShortcutManager* ProfileShortcutManager::Create() {
-  return new ProfileShortcutManagerWin();
+ProfileShortcutManager* ProfileShortcutManager::Create(
+    ProfileInfoCache& cache) {
+  ProfileShortcutManagerWin* manager = new ProfileShortcutManagerWin();
+  cache.AddObserver(manager);

[sail, 2012/08/23 18:46:44]

you might also need a RemoveObserver() call somewhere

[Halli, 2012/08/24 02:56:49]

Done.

+  return manager;
 }
 
 ProfileShortcutManagerWin::ProfileShortcutManagerWin() {
@@ -178,52 +244,106 @@
 void ProfileShortcutManagerWin::CreateChromeDesktopShortcut(
     const FilePath& profile_path, const string16& profile_name,
     const gfx::Image& avatar_image) {
-  FilePath shortcut_icon = CreateChromeDesktopShortcutIconForProfile(
-      profile_path, avatar_image);
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  FilePath chrome_exe;
-  if (!PathService::Get(base::FILE_EXE, &chrome_exe))
-    return;
-  BrowserDistribution* dist = BrowserDistribution::GetDistribution();
-  string16 description;
-  if (!dist)
-    return;
-  description = WideToUTF16(dist->GetAppDescription());
-
-  // Add the profile to the map if it doesn't exist already
-  if (!profile_shortcuts_.count(profile_path)) {
-    string16 flags = CreateProfileShortcutFlags(profile_path);
-    profile_shortcuts_.insert(std::make_pair(profile_path,
-                              ProfileShortcutInfo(flags, profile_name,
-                                  avatar_image)));
+  const SkBitmap* avatar_bitmap = avatar_image.ToSkBitmap();
+  SkBitmap avatar_bitmap_copy;
+  if (avatar_bitmap->deepCopyTo(&avatar_bitmap_copy,

[sail, 2012/08/23 18:46:44]

this should just be a DCHECK I think, 
otherwise this should be changed to an early return

also, need a comment explaining why you're doing a copy

[Halli, 2012/08/24 02:56:49]

Done.

+                                avatar_bitmap->getConfig())) {
+    BrowserThread::PostTask(
+        BrowserThread::FILE, FROM_HERE,
+        base::Bind(&CompleteProfileShortcutCreation,
+                   profile_path, profile_name, avatar_bitmap_copy));
   }
-
-  ShellUtil::CreateChromeDesktopShortcut(
-      dist,
-      chrome_exe.value(),
-      description,
-      profile_shortcuts_[profile_path].profile_name,
-      profile_shortcuts_[profile_path].flags,
-      shortcut_icon.empty() ? chrome_exe.value() : shortcut_icon.value(),
-      shortcut_icon.empty() ? dist->GetIconIndex() : 0,
-      ShellUtil::CURRENT_USER,
-      ShellUtil::SHORTCUT_CREATE_ALWAYS);
 }
 
 void ProfileShortcutManagerWin::DeleteChromeDesktopShortcut(
-    const FilePath& profile_path) {
+    const FilePath& profile_path, const string16& profile_name) {
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
   string16 shortcut;
   // If we can find the shortcut, delete it
   if (ShellUtil::GetChromeShortcutName(dist, false,
-      profile_shortcuts_[profile_path].profile_name, &shortcut)) {
+       profile_name, &shortcut)) {

[sail, 2012/08/23 18:46:44]

is this indentation correct?

[Halli, 2012/08/24 02:56:49]

Done.

     std::vector<string16> appended_names(1, shortcut);
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
             base::Bind(&CallShellUtilBoolFunction, base::Bind(
                 &ShellUtil::RemoveChromeDesktopShortcutsWithAppendedNames,
                 appended_names)));
-    profile_shortcuts_.erase(profile_path);
   }
 }
 
+void ProfileShortcutManagerWin::OnProfileAdded(const FilePath& profile_path) {
+}
+
+void ProfileShortcutManagerWin::OnProfileWillBeRemoved(
+    const FilePath& profile_path) {
+}
+
+void ProfileShortcutManagerWin::OnProfileWasRemoved(
+    const FilePath& profile_path,
+    const string16& profile_name) {
+}
+
+void ProfileShortcutManagerWin::OnProfileNameChanged(
+    const FilePath& profile_path,
+    const string16& old_profile_name) {
+  ProfileInfoCache& cache =
+      g_browser_process->profile_manager()->GetProfileInfoCache();

[sail, 2012/08/23 18:46:44]

It's weird that you're accessing the cache through the profile manager here and
in the constructor you're having it passed in.

I think you should pick one and use that everywhere.

[Halli, 2012/08/24 02:56:49]

Done.

+
+  size_t profile_index = cache.GetIndexOfProfileWithPath(profile_path);
+  if (profile_index == std::string::npos)
+      return;
+
+  string16 new_profile_name =
+      cache.GetNameOfProfileAtIndex(profile_index);
+
+  string16 old_shortcut;
+  string16 new_shortcut;
+  BrowserDistribution* dist = BrowserDistribution::GetDistribution();
+  if (ShellUtil::GetChromeShortcutName(
+          dist, false, old_profile_name, &old_shortcut) &&
+      ShellUtil::GetChromeShortcutName(
+          dist, false, new_profile_name, &new_shortcut)) {
+    BrowserThread::PostTask(
+        BrowserThread::FILE, FROM_HERE,
+        base::Bind(&RenameChromeDesktopShortcutForProfile,
+                   old_shortcut,
+                   new_shortcut));
+  }
+}
+
+void ProfileShortcutManagerWin::OnProfileAvatarChanged(
+    const FilePath& profile_path) {
+  ProfileInfoCache& cache =
+      g_browser_process->profile_manager()->GetProfileInfoCache();
+
+  size_t profile_index = cache.GetIndexOfProfileWithPath(profile_path);
+  if (profile_index == std::string::npos)
+      return;
+
+  string16 profile_name =
+      cache.GetNameOfProfileAtIndex(profile_index);
+  string16 shortcut;
+
+  BrowserDistribution* dist = BrowserDistribution::GetDistribution();
+  if (ShellUtil::GetChromeShortcutName(
+         dist, false,  profile_name, &shortcut)) {
+    size_t new_icon_index = cache.GetAvatarIconIndexOfProfileAtIndex(
+        profile_index);
+    gfx::Image avatar_image =
+         ResourceBundle::GetSharedInstance().GetNativeImageNamed(
+         cache.GetDefaultAvatarIconResourceIDAtIndex(new_icon_index));

[sail, 2012/08/23 18:46:44]

indentation is wrong

[Halli, 2012/08/24 02:56:49]

Done.

+
+    const SkBitmap* avatar_bitmap = avatar_image.ToSkBitmap();
+    SkBitmap avatar_bitmap_copy;
+    if (avatar_bitmap->deepCopyTo(&avatar_bitmap_copy,

[sail, 2012/08/23 18:46:44]

same comment as the above deepCopyTo call

[Halli, 2012/08/24 02:56:49]

Done.

+                                  avatar_bitmap->getConfig())) {
+      BrowserThread::PostTask(
+          BrowserThread::FILE, FROM_HERE,
+          base::Bind(&UpdateChromeDesktopShortcutForProfile,
+                     shortcut,
+                     CreateProfileShortcutFlags(profile_path),
+                     profile_path, avatar_bitmap_copy));
+    }
+  }
+}
+