Create profile .ico file on profile creation

chrome/browser/profiles/profile_shortcut_manager_win.cc

Index: chrome/browser/profiles/profile_shortcut_manager_win.cc
diff --git a/chrome/browser/profiles/profile_shortcut_manager_win.cc b/chrome/browser/profiles/profile_shortcut_manager_win.cc
index 4620240825ae897b36b72fe078729a08d424487c..6262dd9d21379c0d6c4d34373057c8dadf458650 100644
--- a/chrome/browser/profiles/profile_shortcut_manager_win.cc
+++ b/chrome/browser/profiles/profile_shortcut_manager_win.cc
@@ -138,8 +138,9 @@ SkBitmap BadgeIcon(const SkBitmap& app_icon_bitmap,
 // Creates a desktop shortcut icon file (.ico) on the disk for a given profile,
 // badging the browser distribution icon with the profile avatar.
 // Returns a path to the shortcut icon file on disk, which is empty if this
-// fails. Use index 0 when assigning the resulting file as the icon.
-base::FilePath CreateChromeDesktopShortcutIconForProfile(
+// fails. Use index 0 when assigning the resulting file as the icon. If both
+// given bitmaps are empty, an unbadged icon is created.

[Alexei Svitkine (slow), 2013/05/01 15:27:30]

Please add a TODO that ideally we'd just want to copy the app icon verbatim from
the exe's resources in this case.

[calamity, 2013/05/03 04:40:28]

Done.

+base::FilePath CreateOrUpdateShortcutIconForProfile(
     const base::FilePath& profile_path,
     const SkBitmap& avatar_bitmap_1x,
     const SkBitmap& avatar_bitmap_2x) {
@@ -148,21 +149,32 @@ base::FilePath CreateChromeDesktopShortcutIconForProfile(
   if (!app_icon_bitmap.get())
     return base::FilePath();
 
-  const SkBitmap badged_bitmap = BadgeIcon(*app_icon_bitmap,
-                                           avatar_bitmap_1x, 1);
+  SkBitmap badged_bitmap;

[Alexei Svitkine (slow), 2013/05/01 15:27:30]

How about:

SkBitmap badged_bitmap = avatar_bitmap_1x.empty() ? *app_icon_bitmap :
                                            BadgeIcon(*app_icon_bitmap,
avatar_bitmap_1x, 1);

Same thing for large_badged_bitmap.

Then you can avoid the if later on.

[calamity, 2013/05/03 04:40:28]

I think that making these changes decreases the robustness of this icon
creation? What should happen if only the 2x bitmap is available? The ImageFamily
icon creation should handle only having one size of icon to work with.

I've refactored this a little so the logic is clearer. Let me know if this is
sufficient.

On 2013/05/01 15:27:30, Alexei Svitkine wrote:

+  if (!avatar_bitmap_1x.empty())
+    badged_bitmap = BadgeIcon(*app_icon_bitmap, avatar_bitmap_1x, 1);
 
   SkBitmap large_badged_bitmap;
-  app_icon_bitmap = GetAppIconForSize(IconUtil::kLargeIconSize);
-  if (app_icon_bitmap.get())
-    large_badged_bitmap = BadgeIcon(*app_icon_bitmap, avatar_bitmap_2x, 2);
+  scoped_ptr<SkBitmap> large_app_icon_bitmap(
+      GetAppIconForSize(IconUtil::kLargeIconSize));
+  if (large_app_icon_bitmap.get() && !avatar_bitmap_2x.empty()) {
+    large_badged_bitmap =
+        BadgeIcon(*large_app_icon_bitmap, avatar_bitmap_2x, 2);
+  }
 
   // Finally, write the .ico file containing this new bitmap.
   const base::FilePath icon_path =
-      profile_path.AppendASCII(profiles::internal::kProfileIconFileName);
+      profiles::internal::GetProfileIconPath(profile_path);
+  if (avatar_bitmap_1x.empty() && avatar_bitmap_2x.empty()) {
+    badged_bitmap = *app_icon_bitmap;
+    if (large_app_icon_bitmap.get())
+      large_badged_bitmap = *large_app_icon_bitmap;
+  }
   if (!IconUtil::CreateIconFileFromSkBitmap(badged_bitmap, large_badged_bitmap,
-                                            icon_path))
-    return base::FilePath();
-
+                                            icon_path)) {
+    NOTREACHED();

[Alexei Svitkine (slow), 2013/05/01 15:27:30]

Re-add the return base::FilePath() to propagate the failure, just in case.

[calamity, 2013/05/03 04:40:28]

Done.

+  }
+  SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT,
+                 NULL, NULL);
   return icon_path;
 }
 
@@ -297,7 +309,8 @@ void RenameChromeDesktopShortcutForProfile(
     // It's also possible that a system-level shortcut exists instead - this
     // should only be the case for the original Chrome shortcut from an
     // installation. If that's the case, copy that one over - it will get its
-    // properties updated by |CreateOrUpdateDesktopShortcutsForProfile()|.
+    // properties updated by
+    // |CreateOrUpdateDesktopShortcutsAndIconForProfile()|.
     const base::FilePath possible_old_system_shortcut =
         system_shortcuts_directory.Append(old_shortcut_filename);
     if (file_util::PathExists(possible_old_system_shortcut))
@@ -309,7 +322,7 @@ void RenameChromeDesktopShortcutForProfile(
 // parameters. If |create_mode| is CREATE_WHEN_NONE_FOUND, a new shortcut is
 // created if no existing ones were found. Whether non-profile shortcuts should
 // be updated is specified by |action|. Must be called on the FILE thread.
-void CreateOrUpdateDesktopShortcutsForProfile(
+void CreateOrUpdateDesktopShortcutsAndIconForProfile(
     const base::FilePath& profile_path,
     const string16& old_profile_name,
     const string16& profile_name,
@@ -319,6 +332,17 @@ void CreateOrUpdateDesktopShortcutsForProfile(
     ProfileShortcutManagerWin::NonProfileShortcutAction action) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
+  base::FilePath shortcut_icon =
+      CreateOrUpdateShortcutIconForProfile(profile_path,
+                                           avatar_image_1x,
+                                           avatar_image_2x);
+  if (shortcut_icon.empty()) {
+    NOTREACHED();

[Alexei Svitkine (slow), 2013/05/01 15:27:30]

Add a return.

[calamity, 2013/05/03 04:40:28]

Done.

+  }
+  if (create_mode == ProfileShortcutManagerWin::CREATE_ICON_ONLY) {

[Alexei Svitkine (slow), 2013/05/01 15:27:30]

No need for {}'s.

[calamity, 2013/05/03 04:40:28]

Done.

+    return;
+  }
+
   base::FilePath chrome_exe;
   if (!PathService::Get(base::FILE_EXE, &chrome_exe)) {
     NOTREACHED();
@@ -352,10 +376,6 @@ void CreateOrUpdateDesktopShortcutsForProfile(
   // If it is empty, it means the shortcut being created should be a regular,
   // non-profile Chrome shortcut.
   if (!profile_name.empty()) {
-    const base::FilePath shortcut_icon =
-        CreateChromeDesktopShortcutIconForProfile(profile_path,
-                                                  avatar_image_1x,
-                                                  avatar_image_2x);
     if (!shortcut_icon.empty())
       properties.set_icon(shortcut_icon, 0);
     properties.set_arguments(command_line);
@@ -412,7 +432,7 @@ bool ChromeDesktopShortcutsExist(const base::FilePath& chrome_exe) {
 // corresponding icon file. If |ensure_shortcuts_remain| is true, then a regular
 // non-profile shortcut will be created if this function would otherwise delete
 // the last Chrome desktop shortcut(s). Must be called on the FILE thread.
-void DeleteDesktopShortcutsAndIconFile(const base::FilePath& profile_path,
+void DeleteDesktopShortcuts(const base::FilePath& profile_path,
                                        bool ensure_shortcuts_remain) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
@@ -438,10 +458,6 @@ void DeleteDesktopShortcutsAndIconFile(const base::FilePath& profile_path,
                    NULL);
   }
 
-  const base::FilePath icon_path =
-      profile_path.AppendASCII(profiles::internal::kProfileIconFileName);
-  file_util::Delete(icon_path, false);
-
   // If |ensure_shortcuts_remain| is true and deleting this profile caused the
   // last shortcuts to be removed, re-create a regular non-profile shortcut.
   const bool had_shortcuts = !shortcuts.empty();
@@ -523,6 +539,10 @@ SkBitmap GetImageResourceSkBitmapCopy(int resource_id) {
 namespace profiles {
 namespace internal {
 
+base::FilePath GetProfileIconPath(const base::FilePath& profile_path) {
+  return profile_path.AppendASCII(profiles::internal::kProfileIconFileName);
+}
+
 const char kProfileIconFileName[] = "Google Profile.ico";
 
 string16 GetShortcutFilenameForProfile(const string16& profile_name,
@@ -583,7 +603,7 @@ void ProfileShortcutManagerWin::RemoveProfileShortcuts(
     const base::FilePath& profile_path) {
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DeleteDesktopShortcutsAndIconFile, profile_path, false));
+      base::Bind(&DeleteDesktopShortcuts, profile_path, false));
 }
 
 void ProfileShortcutManagerWin::HasProfileShortcuts(
@@ -602,10 +622,14 @@ void ProfileShortcutManagerWin::OnProfileAdded(
     CreateOrUpdateShortcutsForProfileAtPath(profile_path,
                                             CREATE_WHEN_NONE_FOUND,
                                             UPDATE_NON_PROFILE_SHORTCUTS);
-  } else if (profile_count == 2) {
-    CreateOrUpdateShortcutsForProfileAtPath(GetOtherProfilePath(profile_path),
-                                            UPDATE_EXISTING_ONLY,
-                                            UPDATE_NON_PROFILE_SHORTCUTS);
+  } else {
+    CreateOrUpdateShortcutsForProfileAtPath(profile_path, CREATE_ICON_ONLY,
+                                            IGNORE_NON_PROFILE_SHORTCUTS);
+    if (profile_count == 2) {
+      CreateOrUpdateShortcutsForProfileAtPath(GetOtherProfilePath(profile_path),
+                                              UPDATE_EXISTING_ONLY,
+                                              UPDATE_NON_PROFILE_SHORTCUTS);
+    }
   }
 }
 
@@ -621,13 +645,15 @@ void ProfileShortcutManagerWin::OnProfileWasRemoved(
   // from an existing shortcut.
   const bool deleting_down_to_last_profile = (cache.GetNumberOfProfiles() == 1);
   if (deleting_down_to_last_profile) {
-    CreateOrUpdateShortcutsForProfileAtPath(cache.GetPathOfProfileAtIndex(0),
+    // This is needed to unbadge the icon.
+    base::FilePath profile_path = cache.GetPathOfProfileAtIndex(0);
+    CreateOrUpdateShortcutsForProfileAtPath(profile_path,
                                             UPDATE_EXISTING_ONLY,
                                             IGNORE_NON_PROFILE_SHORTCUTS);
   }
 
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-                          base::Bind(&DeleteDesktopShortcutsAndIconFile,
+                          base::Bind(&DeleteDesktopShortcuts,
                                      profile_path,
                                      deleting_down_to_last_profile));
 }
@@ -641,7 +667,7 @@ void ProfileShortcutManagerWin::OnProfileNameChanged(
 
 void ProfileShortcutManagerWin::OnProfileAvatarChanged(
     const base::FilePath& profile_path) {
-  CreateOrUpdateShortcutsForProfileAtPath(profile_path, UPDATE_EXISTING_ONLY,
+  CreateOrUpdateShortcutsForProfileAtPath(profile_path, CREATE_ICON_ONLY,
                                           IGNORE_NON_PROFILE_SHORTCUTS);
 }
 
@@ -697,7 +723,7 @@ void ProfileShortcutManagerWin::CreateOrUpdateShortcutsForProfileAtPath(
   }
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&CreateOrUpdateDesktopShortcutsForProfile, profile_path,
+      base::Bind(&CreateOrUpdateDesktopShortcutsAndIconForProfile, profile_path,
                  old_shortcut_appended_name, new_shortcut_appended_name,
                  avatar_bitmap_copy_1x, avatar_bitmap_copy_2x, create_mode,
                  action));