Create profile .ico file on profile creation

chrome/browser/profiles/profile_shortcut_manager_win.cc

 // Copyright (c) 2012 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/profiles/profile_shortcut_manager_win.h"
 
 #include <shlobj.h>  // For SHChangeNotify().
 
 #include <string>
 #include <vector>
@@ skipping 121 matching lines @@
       offscreen_canvas->getDevice()->accessBitmap(false);
   SkBitmap badged_bitmap_copy;
   badged_bitmap.deepCopyTo(&badged_bitmap_copy, badged_bitmap.getConfig());
   return badged_bitmap_copy;
 }
 
 // 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(
+void CreateOrUpdateShortcutIconForProfile(
     const base::FilePath& profile_path,
     const SkBitmap& avatar_bitmap_1x,
     const SkBitmap& avatar_bitmap_2x) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   scoped_ptr<SkBitmap> app_icon_bitmap(GetAppIconForSize(kShortcutIconSize));
   if (!app_icon_bitmap.get())
-    return base::FilePath();
+    return;
 
   const SkBitmap 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);
 
   // Finally, write the .ico file containing this new bitmap.
   const base::FilePath icon_path =
       profile_path.AppendASCII(profiles::internal::kProfileIconFileName);
   if (!IconUtil::CreateIconFileFromSkBitmap(badged_bitmap, large_badged_bitmap,
                                             icon_path))
-    return base::FilePath();
-
-  return icon_path;
+    LOG(ERROR) << "Failed to create icon at " << icon_path.value();

[Alexei Svitkine (slow), 2013/04/26 21:16:23]

Just make it a NOTREACHED()?

Also, please add {}'s to the if.

[calamity, 2013/04/30 06:45:41]

Done.

 }
 
 // Gets the user and system directories for desktop shortcuts. Parameters may
 // be NULL if a directory type is not needed. Returns true on success.
 bool GetDesktopShortcutsDirectories(
     base::FilePath* user_shortcuts_directory,
     base::FilePath* system_shortcuts_directory) {
   BrowserDistribution* distribution = BrowserDistribution::GetDistribution();
   if (user_shortcuts_directory &&
       !ShellUtil::GetShortcutPath(ShellUtil::SHORTCUT_LOCATION_DESKTOP,
@@ skipping 129 matching lines @@
 }
 
 // Updates all desktop shortcuts for the given profile to have the specified
 // 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(
     const base::FilePath& profile_path,
     const string16& old_profile_name,
     const string16& profile_name,
-    const SkBitmap& avatar_image_1x,
-    const SkBitmap& avatar_image_2x,
     ProfileShortcutManagerWin::CreateOrUpdateMode create_mode,
     ProfileShortcutManagerWin::NonProfileShortcutAction action) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   base::FilePath chrome_exe;
   if (!PathService::Get(base::FILE_EXE, &chrome_exe)) {
     NOTREACHED();
     return;
   }
 
@@ skipping 17 matching lines @@
   installer::Product product(distribution);
   product.AddDefaultShortcutProperties(chrome_exe, &properties);
 
   const string16 command_line =
       profiles::internal::CreateProfileShortcutFlags(profile_path);
 
   // Only set the profile-specific properties when |profile_name| is non empty.
   // 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())
+    base::FilePath shortcut_icon =
+        profile_path.AppendASCII(profiles::internal::kProfileIconFileName);
+    if (file_util::PathExists(shortcut_icon))

[Alexei Svitkine (slow), 2013/04/26 21:16:23]

When would this be the case?

Wouldn't we want to re-create the icon if it doesn't exist?

[calamity, 2013/04/30 06:45:41]

Rolled the 2 functions back together and created the icon at the top of this
function.

       properties.set_icon(shortcut_icon, 0);
     properties.set_arguments(command_line);
   } else {
     // Set the arguments explicitly to the empty string to ensure that
     // |ShellUtil::CreateOrUpdateShortcut| updates that part of the shortcut.
     properties.set_arguments(string16());
   }
 
   ShellUtil::ShortcutOperation operation =
       ShellUtil::SHELL_SHORTCUT_REPLACE_EXISTING;
@@ skipping 63 matching lines @@
     // Use file_util::Delete() instead of ShellUtil::RemoveShortcut(), as the
     // latter causes non-profile taskbar shortcuts to be unpinned.
     file_util::Delete(shortcuts[i], false);
     // Notify the shell that the shortcut was deleted to ensure desktop refresh.
     SHChangeNotify(SHCNE_DELETE, SHCNF_PATH, shortcuts[i].value().c_str(),
                    NULL);
   }
 
   const base::FilePath icon_path =
       profile_path.AppendASCII(profiles::internal::kProfileIconFileName);
   file_util::Delete(icon_path, false);

[Alexei Svitkine (slow), 2013/04/26 21:16:23]

I think you need to remove this line, else when someone deletes their shortcut
(from the chrome://setting user editor), it will delete the icon.

Can you add a test that verifies this doesn't happen?

[calamity, 2013/04/30 06:45:41]

Done.

The change I've made to the tests assert that the icon file remains as a
post-condition at cleanup of each test. Do you want an explicit test for this?

 
   // 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();
   if (ensure_shortcuts_remain && had_shortcuts &&
       !ChromeDesktopShortcutsExist(chrome_exe)) {
     BrowserDistribution* distribution = BrowserDistribution::GetDistribution();
     // Ensure that the distribution supports creating shortcuts. If it doesn't,
     // the following code may result in NOTREACHED() being hit.
     DCHECK(distribution->CanCreateDesktopShortcuts());
@@ skipping 135 matching lines @@
 void ProfileShortcutManagerWin::HasProfileShortcuts(
     const base::FilePath& profile_path,
     const base::Callback<void(bool)>& callback) {
   BrowserThread::PostTaskAndReplyWithResult(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&HasAnyProfileShortcuts, profile_path), callback);
 }
 
 void ProfileShortcutManagerWin::OnProfileAdded(
     const base::FilePath& profile_path) {
+  CreateOrUpdateShortcutIconForProfileAtPath(profile_path);
   const size_t profile_count =
       profile_manager_->GetProfileInfoCache().GetNumberOfProfiles();
   if (profile_count == 1) {
     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);
   }
 }
 
 void ProfileShortcutManagerWin::OnProfileWillBeRemoved(
     const base::FilePath& profile_path) {
 }
 
 void ProfileShortcutManagerWin::OnProfileWasRemoved(
     const base::FilePath& profile_path,
     const string16& profile_name) {
   const ProfileInfoCache& cache = profile_manager_->GetProfileInfoCache();
   // If there is only one profile remaining, remove the badging information
   // from an existing shortcut.
   const bool deleting_down_to_last_profile = (cache.GetNumberOfProfiles() == 1);
   if (deleting_down_to_last_profile) {
-    CreateOrUpdateShortcutsForProfileAtPath(cache.GetPathOfProfileAtIndex(0),
+    base::FilePath profile_path = cache.GetPathOfProfileAtIndex(0);
+    CreateOrUpdateShortcutIconForProfileAtPath(profile_path);

[Alexei Svitkine (slow), 2013/04/26 21:16:23]

Add a comment mentioning that this is needed to "unbadge" the icon.

[calamity, 2013/04/30 06:45:41]

Done. I actually want to make an unbadged icon that overwrites the profile icon.
Should I do this in this patch? (It currently crashes on the no badge case, the
changes are above in CreateOrUpdateShortcutIconForProfile)

If you're interested, the reason I need an unbadged icon to overwrite the
profile icon is that if the user pins a profile shortcut, that shortcut points
to the profile icon. So we can either make the profile icon an unbadged chrome
icon in the final delete case or we can try to find every profile shortcut and
change the icon to point back at chrome.exe. I think the old code explicitly set
no icon which made it point to chrome.exe by default but only had to deal with
the desktop shortcuts. Let me know which you think is better.

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

I think it's fine to create an un-badged icon for the profile, in that case. At
the same time, I think we should continue to create "vanilla" shortcuts when
deleting down to 1 profile. The unbadged icon would just be there for the case
of pinned shortcuts or shortcuts in other locations that we don't look for as
well as for the window decoration (which will be associated with the icon now).

+    CreateOrUpdateShortcutsForProfileAtPath(profile_path,
                                             UPDATE_EXISTING_ONLY,
                                             IGNORE_NON_PROFILE_SHORTCUTS);
   }
 
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
                           base::Bind(&DeleteDesktopShortcutsAndIconFile,
                                      profile_path,
                                      deleting_down_to_last_profile));
 }
 
 void ProfileShortcutManagerWin::OnProfileNameChanged(
     const base::FilePath& profile_path,
     const string16& old_profile_name) {
   CreateOrUpdateShortcutsForProfileAtPath(profile_path, UPDATE_EXISTING_ONLY,
                                           IGNORE_NON_PROFILE_SHORTCUTS);
 }
 
 void ProfileShortcutManagerWin::OnProfileAvatarChanged(
     const base::FilePath& profile_path) {
+  CreateOrUpdateShortcutIconForProfileAtPath(profile_path);
   CreateOrUpdateShortcutsForProfileAtPath(profile_path, UPDATE_EXISTING_ONLY,

[Alexei Svitkine (slow), 2013/04/26 21:16:23]

Is this call necessary if we're already calling
CreateOrUpdateShortcutIconForProfileAtPath() to update the image with the
avatar? (please test this)

If yes, please add a comment why.

[calamity, 2013/04/30 06:45:41]

Not updating the shortcut seems to be fine.

                                           IGNORE_NON_PROFILE_SHORTCUTS);
 }
 
 base::FilePath ProfileShortcutManagerWin::GetOtherProfilePath(
     const base::FilePath& profile_path) {
   const ProfileInfoCache& cache = profile_manager_->GetProfileInfoCache();
   DCHECK_EQ(2U, cache.GetNumberOfProfiles());
   // Get the index of the current profile, in order to find the index of the
   // other profile.
   size_t current_profile_index = cache.GetIndexOfProfileWithPath(profile_path);
   size_t other_profile_index = (current_profile_index == 0) ? 1 : 0;
   return cache.GetPathOfProfileAtIndex(other_profile_index);
 }
 
 void ProfileShortcutManagerWin::CreateOrUpdateShortcutsForProfileAtPath(
     const base::FilePath& profile_path,
     CreateOrUpdateMode create_mode,
     NonProfileShortcutAction action) {
   ProfileInfoCache* cache = &profile_manager_->GetProfileInfoCache();
   size_t profile_index = cache->GetIndexOfProfileWithPath(profile_path);
   if (profile_index == std::string::npos)
     return;
-  bool remove_badging = cache->GetNumberOfProfiles() == 1;
+  bool is_last_profile = cache->GetNumberOfProfiles() == 1;
 
   string16 old_shortcut_appended_name =
       cache->GetShortcutNameOfProfileAtIndex(profile_index);
 
   // Exit early if the mode is to update existing profile shortcuts only and
   // none were ever created for this profile, per the shortcut name not being
   // set in the profile info cache.
   if (old_shortcut_appended_name.empty() &&
       create_mode == UPDATE_EXISTING_ONLY &&
       action == IGNORE_NON_PROFILE_SHORTCUTS) {
     return;
   }
 
   string16 new_shortcut_appended_name;
-  if (!remove_badging)
+  if (!is_last_profile)
     new_shortcut_appended_name = cache->GetNameOfProfileAtIndex(profile_index);
 
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&CreateOrUpdateDesktopShortcutsForProfile, profile_path,
+                 old_shortcut_appended_name, new_shortcut_appended_name,
+                 create_mode, action));
+
+  cache->SetShortcutNameOfProfileAtIndex(profile_index,
+                                         new_shortcut_appended_name);
+}
+
+void ProfileShortcutManagerWin::CreateOrUpdateShortcutIconForProfileAtPath(
+    const base::FilePath& profile_path) {
+  ProfileInfoCache* cache = &profile_manager_->GetProfileInfoCache();
+  size_t profile_index = cache->GetIndexOfProfileWithPath(profile_path);
+  if (profile_index == std::string::npos)
+    return;
+  bool remove_badging = cache->GetNumberOfProfiles() == 1;
+
   SkBitmap avatar_bitmap_copy_1x;
   SkBitmap avatar_bitmap_copy_2x;
   if (!remove_badging) {
     const size_t icon_index =
         cache->GetAvatarIconIndexOfProfileAtIndex(profile_index);
     const int resource_id_1x =
         cache->GetDefaultAvatarIconResourceIDAtIndex(icon_index);
     const int resource_id_2x = kProfileAvatarIconResources2x[icon_index];
     // Make a copy of the SkBitmaps to ensure that we can safely use the image
     // data on the FILE thread.
     avatar_bitmap_copy_1x = GetImageResourceSkBitmapCopy(resource_id_1x);
     avatar_bitmap_copy_2x = GetImageResourceSkBitmapCopy(resource_id_2x);
   }
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&CreateOrUpdateDesktopShortcutsForProfile, profile_path,
-                 old_shortcut_appended_name, new_shortcut_appended_name,
-                 avatar_bitmap_copy_1x, avatar_bitmap_copy_2x, create_mode,
-                 action));
-
-  cache->SetShortcutNameOfProfileAtIndex(profile_index,
-                                         new_shortcut_appended_name);
+      base::Bind(&CreateOrUpdateShortcutIconForProfile, profile_path,
+                 avatar_bitmap_copy_1x, avatar_bitmap_copy_2x));
 }