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 3c038a1401b353bfdf5a75da6dbb27ceb768e5ec..d7c838c4ca57ab390dfc44586273e21b2dd91bed 100644
--- a/chrome/browser/profiles/profile_shortcut_manager_win.cc
+++ b/chrome/browser/profiles/profile_shortcut_manager_win.cc
@@ -13,6 +13,7 @@
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
+#include "base/prefs/pref_service.h"
 #include "base/string16.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
@@ -24,11 +25,14 @@
 #include "chrome/browser/profiles/profile_info_util.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/shell_integration.h"
+#include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/installer/util/browser_distribution.h"
 #include "chrome/installer/util/product.h"
 #include "chrome/installer/util/shell_util.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_service.h"
 #include "grit/chrome_unscaled_resources.h"
 #include "grit/chromium_strings.h"
 #include "skia/ext/image_operations.h"
@@ -45,6 +49,9 @@ using content::BrowserThread;
 
 namespace {
 
+// Name of the badged icon file generated for a given profile.
+const char kProfileIconFileName[] = "Google Profile.ico";
+
 // Characters that are not allowed in Windows filenames. Taken from
 // http://msdn.microsoft.com/en-us/library/aa365247.aspx
 const char16 kReservedCharacters[] = L"<>:\"/\\|?*\x01\x02\x03\x04\x05\x06\x07"
@@ -60,6 +67,8 @@ const int kMaxProfileShortcutFileNameLength = 64;
 const int kProfileAvatarBadgeSize = 28;
 const int kShortcutIconSize = 48;
 
+const int kCurrentProfileIconVersion = 1;
+
 // 2x sized profile avatar icons. Mirrors |kDefaultAvatarIconResources| in
 // profile_info_cache.cc.
 const int kProfileAvatarIconResources2x[] = {
@@ -140,35 +149,73 @@ 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.
+// |is_create| should be true if we are creating the icon at a time when it

[gab, 2013/06/18 15:36:58]

There is no such |is_create| parameter..?

[calamity, 2013/06/27 08:10:11]

Done.

+// is not possibly already in use. If it is false, we will refresh the Windows
+// icon cache.
+// TODO(calamity): Ideally we'd just copy the app icon verbatim from the exe's
+// resources in the case of an unbadged icon.
+base::FilePath CreateOrUpdateShortcutIconForProfile(
     const base::FilePath& profile_path,
     const SkBitmap& avatar_bitmap_1x,
-    const SkBitmap& avatar_bitmap_2x) {
+    const SkBitmap& avatar_bitmap_2x,
+    const base::Closure& callback) {

[gab, 2013/06/18 15:36:58]

Explain what is |callback| in the method comment above.

[calamity, 2013/06/27 08:10:11]

Done.

   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   scoped_ptr<SkBitmap> app_icon_bitmap(GetAppIconForSize(kShortcutIconSize));
   if (!app_icon_bitmap)
     return base::FilePath();

[gab, 2013/06/18 15:36:58]

Also add to the method comment about what it returns generally and in failure
cases.

[calamity, 2013/06/27 08:10:11]

Done.

 
   gfx::ImageFamily badged_bitmaps;
-  badged_bitmaps.Add(gfx::Image::CreateFrom1xBitmap(
-      BadgeIcon(*app_icon_bitmap, avatar_bitmap_1x, 1)));
+  if (!avatar_bitmap_1x.empty()) {
+    badged_bitmaps.Add(gfx::Image::CreateFrom1xBitmap(
+        BadgeIcon(*app_icon_bitmap, avatar_bitmap_1x, 1)));
+  }
 
-  app_icon_bitmap = GetAppIconForSize(IconUtil::kLargeIconSize);
-  if (app_icon_bitmap) {
+  scoped_ptr<SkBitmap> large_app_icon_bitmap(
+      GetAppIconForSize(IconUtil::kLargeIconSize));
+  if (large_app_icon_bitmap && !avatar_bitmap_2x.empty()) {
     badged_bitmaps.Add(gfx::Image::CreateFrom1xBitmap(
-        BadgeIcon(*app_icon_bitmap, avatar_bitmap_2x, 2)));
+        BadgeIcon(*large_app_icon_bitmap, avatar_bitmap_2x, 2)));
   }
 
+  // If we have no badged bitmaps, we should just use the default chrome icon.
+  if (badged_bitmaps.empty()) {
+    badged_bitmaps.Add(gfx::Image::CreateFrom1xBitmap(*app_icon_bitmap));
+    if (large_app_icon_bitmap) {
+      badged_bitmaps.Add(
+          gfx::Image::CreateFrom1xBitmap(*large_app_icon_bitmap));
+    }
+  }
   // Finally, write the .ico file containing this new bitmap.
   const base::FilePath icon_path =
-      profile_path.AppendASCII(profiles::internal::kProfileIconFileName);
-  if (!IconUtil::CreateIconFileFromImageFamily(badged_bitmaps, icon_path))
+      profiles::internal::GetProfileIconPath(profile_path);
+  const bool had_icon = file_util::PathExists(icon_path);
+
+  if (!IconUtil::CreateIconFileFromImageFamily(badged_bitmaps, icon_path)) {
+    NOTREACHED();
     return base::FilePath();
+  }
 
+  if (had_icon) {
+    // This invalidates the Windows icon cache and causes the icon changes to
+    // register with the taskbar and desktop. SHCNE_ASSOCCHANGED will cause a
+    // desktop flash and we would like to avoid that if possible.
+    SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT,

[gab, 2013/06/18 15:36:58]

I don't think SHCNF_FLUSHNOWAIT is necessary here, it is not used in similar
code
(https://code.google.com/p/chromium/codesearch#chromium/src/base/win/shortcut....)
and I have had trouble with SHCNF_FLUSHNOWAIT in the past
(https://chromiumcodereview.appspot.com/10692138).

[calamity, 2013/06/27 08:10:11]

Done.

+                   NULL, NULL);
+  } else {
+    SHChangeNotify(SHCNE_CREATE, SHCNF_PATH, icon_path.value().c_str(), NULL);
+  }
+  if (!callback.is_null())
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
   return icon_path;
 }
 
+void OnProfileIconCreateSuccess(Profile* profile) {

[Alexei Svitkine (slow), 2013/06/18 15:27:19]

Nit: Add a comment.

[calamity, 2013/06/27 08:10:11]

Done.

+  profile->GetPrefs()->SetInteger(prefs::kProfileIconVersion,
+                                  kCurrentProfileIconVersion);

[gab, 2013/06/18 15:36:58]

In the current design, this will never be tried again, see my suggestion on
ProfileShortcutManagerWin::Observe.

+}
+
 // 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(
@@ -300,7 +347,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))
@@ -308,20 +356,51 @@ void RenameChromeDesktopShortcutForProfile(
   }
 }
 
+struct CreateOrUpdateShortcutsParams {

[gab, 2013/06/18 15:36:58]

How about adding the callback to this list as well (instead of forcing most
callers to pass a null-callback)?

[calamity, 2013/06/27 08:10:11]

This doesn't avoid needing the parameter in
CreateOrUpdateShortcutsForProfileAtPath() where the null-callbacks used.

+  CreateOrUpdateShortcutsParams(
+      base::FilePath profile_path,
+      ProfileShortcutManagerWin::CreateOrUpdateMode create_mode,
+      ProfileShortcutManagerWin::NonProfileShortcutAction action)
+      : profile_path(profile_path), create_mode(create_mode), action(action) {}
+  ~CreateOrUpdateShortcutsParams() {}
+
+  ProfileShortcutManagerWin::CreateOrUpdateMode create_mode;
+  ProfileShortcutManagerWin::NonProfileShortcutAction action;
+
+  // The path for this profile.
+  base::FilePath profile_path;
+  // The profile name before this update. Empty on create.
+  string16 old_profile_name;
+  // The new profile name.
+  string16 profile_name;
+  // Avatar images for this profile.
+  SkBitmap avatar_image_1x;
+  SkBitmap avatar_image_2x;
+};
+
 // 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.

[gab, 2013/06/18 15:36:58]

This comment now refers to parameters that were moved to
CreateOrUpdateShortcutsParams, please move the comments to their respective
members as you see fit.

[gab, 2013/06/18 15:36:58]

Add a param comment for |callback|.

[calamity, 2013/06/27 08:10:11]

Done.

[calamity, 2013/06/27 08:10:11]

Done.

-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) {
+void CreateOrUpdateDesktopShortcutsAndIconForProfile(
+    const CreateOrUpdateShortcutsParams& params,
+    const base::Closure& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
+  const base::FilePath shortcut_icon =
+      CreateOrUpdateShortcutIconForProfile(params.profile_path,

[gab, 2013/06/18 15:36:58]

Shouldn't this only be called if the pref is < than kCurrentProfileIconVersion?

In fact, if we could guarantee the icon exists for all profiles (which we kind
of need actually); then we never need to call this here, no?

[calamity, 2013/06/27 08:10:11]

This is the codepath that CreateOrUpdateProfileIcon() takes and the only call to
CreateOrUpdateShortcutIconForProfile() which does the icon creation. Having the
icon and shortcut create asynchronously with each other could create an issue
where the wrong SHChangeNotify happens (e.g on shortcut creation: shortcut
created, icon created, SCHNE_CREATE). Also, this is the codepath taken to update
the avatar.

+                                           params.avatar_image_1x,
+                                           params.avatar_image_2x,
+                                           callback);
+  if (shortcut_icon.empty()) {
+    NOTREACHED();
+    return;
+  }
+  if (params.create_mode ==
+      ProfileShortcutManagerWin::CREATE_OR_UPDATE_ICON_ONLY) {
+    return;
+  }
+
   base::FilePath chrome_exe;
   if (!PathService::Get(base::FILE_EXE, &chrome_exe)) {
     NOTREACHED();
@@ -333,12 +412,13 @@ void CreateOrUpdateDesktopShortcutsForProfile(
   // the following code may result in NOTREACHED() being hit.
   DCHECK(distribution->CanCreateDesktopShortcuts());
 
-  if (old_profile_name != profile_name) {
+  if (params.old_profile_name != params.profile_name) {
     const string16 old_shortcut_filename =
-        profiles::internal::GetShortcutFilenameForProfile(old_profile_name,
-                                                          distribution);
+        profiles::internal::GetShortcutFilenameForProfile(
+            params.old_profile_name,
+            distribution);
     const string16 new_shortcut_filename =
-        profiles::internal::GetShortcutFilenameForProfile(profile_name,
+        profiles::internal::GetShortcutFilenameForProfile(params.profile_name,
                                                           distribution);
     RenameChromeDesktopShortcutForProfile(old_shortcut_filename,
                                           new_shortcut_filename);
@@ -349,19 +429,14 @@ void CreateOrUpdateDesktopShortcutsForProfile(
   product.AddDefaultShortcutProperties(chrome_exe, &properties);
 
   const string16 command_line =
-      profiles::internal::CreateProfileShortcutFlags(profile_path);
+      profiles::internal::CreateProfileShortcutFlags(params.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())
-      properties.set_icon(shortcut_icon, 0);
+  if (!params.profile_name.empty()) {
     properties.set_arguments(command_line);
+    properties.set_icon(shortcut_icon, 0);
   } else {
     // Set the arguments explicitly to the empty string to ensure that
     // |ShellUtil::CreateOrUpdateShortcut| updates that part of the shortcut.
@@ -369,19 +444,19 @@ void CreateOrUpdateDesktopShortcutsForProfile(
   }
 
   properties.set_app_id(
-      ShellIntegration::GetChromiumModelIdForProfile(profile_path));
+      ShellIntegration::GetChromiumModelIdForProfile(params.profile_path));
 
   ShellUtil::ShortcutOperation operation =
       ShellUtil::SHELL_SHORTCUT_REPLACE_EXISTING;
 
   std::vector<base::FilePath> shortcuts;
   ListDesktopShortcutsWithCommandLine(chrome_exe, command_line,
-      action == ProfileShortcutManagerWin::UPDATE_NON_PROFILE_SHORTCUTS,
+      params.action == ProfileShortcutManagerWin::UPDATE_NON_PROFILE_SHORTCUTS,
       &shortcuts);
-  if (create_mode == ProfileShortcutManagerWin::CREATE_WHEN_NONE_FOUND &&
+  if (params.create_mode == ProfileShortcutManagerWin::CREATE_WHEN_NONE_FOUND &&
       shortcuts.empty()) {
     const string16 shortcut_name =
-        profiles::internal::GetShortcutFilenameForProfile(profile_name,
+        profiles::internal::GetShortcutFilenameForProfile(params.profile_name,
                                                           distribution);
     shortcuts.push_back(base::FilePath(shortcut_name));
     operation = ShellUtil::SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL;
@@ -414,12 +489,12 @@ bool ChromeDesktopShortcutsExist(const base::FilePath& chrome_exe) {
   return false;
 }
 
-// Deletes all desktop shortcuts for the specified profile and also removes the
-// 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,
-                                       bool ensure_shortcuts_remain) {
+// Deletes all desktop shortcuts for the specified profile. 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 DeleteDesktopShortcuts(const base::FilePath& profile_path,
+                            bool ensure_shortcuts_remain) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   base::FilePath chrome_exe;
@@ -444,10 +519,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();
@@ -529,7 +600,9 @@ SkBitmap GetImageResourceSkBitmapCopy(int resource_id) {
 namespace profiles {
 namespace internal {
 
-const char kProfileIconFileName[] = "Google Profile.ico";
+base::FilePath GetProfileIconPath(const base::FilePath& profile_path) {
+  return profile_path.AppendASCII(kProfileIconFileName);
+}
 
 string16 GetShortcutFilenameForProfile(const string16& profile_name,
                                        BrowserDistribution* distribution) {
@@ -556,8 +629,7 @@ string16 CreateProfileShortcutFlags(const base::FilePath& profile_path) {
 // static
 bool ProfileShortcutManager::IsFeatureEnabled() {
   return BrowserDistribution::GetDistribution()->CanCreateDesktopShortcuts() &&
-      !CommandLine::ForCurrentProcess()->HasSwitch(switches::kUserDataDir) &&
-      !CommandLine::ForCurrentProcess()->HasSwitch(switches::kShowAppList);

[gab, 2013/06/18 15:36:58]

Can this be done in a precursor CL? I feel this one-liner needs to be considered
on its own instead of slammed in this big CL where it might have gone unnoticed.

[calamity, 2013/06/27 08:10:11]

Done.

+      !CommandLine::ForCurrentProcess()->HasSwitch(switches::kUserDataDir);

[gab, 2013/06/18 15:36:58]

I still don't like that this is disabled for --user-data-dir :(. This introduces
a regression since badged icons worked with --user-data-dir prior to this CL...

(well... this CL itself doesn't introduce a regression yet since only desktop
profile shortcuts use the new profile icon for now, but the goal should be that
all badged shortcuts use this icon (taskbar shortcut included) in the near
future or the complexity in this CL is pointless imo).

[calamity, 2013/06/27 08:10:11]

This isn't a problem. Once relaunch details on browser windows is set to point
at the app icon, the user data dir icons will be fine.

[gab, 2013/06/27 14:18:56]

Ah, so the plan is to keep the runtime badging code around? Doesn't that create
situations where we end up badging an already badged icon (perhaps breaking some
transparency properties of the badge)?

[Alexei Svitkine (slow), 2013/06/27 15:53:08]

I think this discussion is outside the scope of this CL, so perhaps it should be
taken to one of the relevant bugs. (FWIW I think once we set the relaunch
details, we can stop doing the runtime badging.)

[gab, 2013/06/27 17:55:44]

Right, what I meant is that if the relaunch details point to this new icon which
only exists for the default user-data-dir, then removing badging means breaking
profile taskbar icons for non-default user-data dirs, while not removing badging
means double-badging of the default user-data-dir's icons (I don't think we want
to keep code that is conditional on running in the non-default user-data-dir
either...).

This CL is only really useful if it can be used to set the relaunch details
properly later, no?

I think it's worth revisiting (in a follow-up CL) making ProfileShortcutManager
work (maybe at least to create profile icons) for non-default user-data-dirs...

 }
 
 // static
@@ -572,6 +644,9 @@ ProfileShortcutManagerWin::ProfileShortcutManagerWin(ProfileManager* manager)
       arraysize(kProfileAvatarIconResources2x),
       profile_manager_->GetProfileInfoCache().GetDefaultAvatarIconCount());
 
+  registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,

[Alexei Svitkine (slow), 2013/06/18 15:27:19]

Add a comment to explain this is for profile icon creation.

[Alexei Svitkine (slow), 2013/06/18 15:44:32]

Hmm, is this notified only when a profile gets _created_ or loaded? We want it
on load. (And I know the profile manager code uses "create" inconsistently. :\)

[calamity, 2013/06/27 08:10:11]

I'm fairly sure it's on load. It's in profile_impl.cc.

[gab, 2013/06/27 14:18:56]

Ah ok I see, it seems to be when the Profile object is created, not when the
profile itself is created.

You can easily confirm this by adding a NOTREACHED() here in your local build
and make sure you're hitting when chrome loads even if you already have a
profile on disk.

[gab, 2013/07/05 13:47:24]

Did you try this to confirm?

[calamity, 2013/07/08 07:45:49]

Yeah, works as expected. Put a print and it comes up on activating other
profiles while the browser is running too.

+                 content::NotificationService::AllSources());
+
   profile_manager_->GetProfileInfoCache().AddObserver(this);
 }
 
@@ -579,17 +654,27 @@ ProfileShortcutManagerWin::~ProfileShortcutManagerWin() {
   profile_manager_->GetProfileInfoCache().RemoveObserver(this);
 }
 
+void ProfileShortcutManagerWin::CreateOrUpdateProfileIcon(
+    const base::FilePath& profile_path,
+    const base::Closure& callback) {
+  CreateOrUpdateShortcutsForProfileAtPath(profile_path,
+                                          CREATE_OR_UPDATE_ICON_ONLY,

[gab, 2013/06/27 14:18:56]

Instead of introducing CREATE_OR_UPDATE_ICON_ONLY for this use case only, why
not call CreateOrUpdateShortcutIconForProfile() directly here?

Doesn't seem to me like CreateOrUpdateShortcutsForProfileAtPath(0 does any work
before making this call which we would need to duplicate here...

It makes this method more straight forward imo and helps making the interface
simpler by not adding CREATE_OR_UPDATE_ICON_ONLY.

[Alexei Svitkine (slow), 2013/06/27 15:53:08]

Seems like a reasonable suggestion to me, just add a BrowserThread::PostTask()
to call CreateOrUpdateShortcutIconForProfile with the callback and remove the
CREATE_OR_UPDATE_ICON_ONLY complexity.

Then, CreateOrUpdateDesktopShortcutsAndIconForProfile() doesn't need to take a
callback param and can just pass a base::Closure() when calling
CreateOrUpdateShortcutIconForProfile() from it.

[calamity, 2013/07/05 08:04:33]

I'll still need to get the avatar resources...? I'll get that code and factor it
out into a function in the anonymous namespace then I guess.

[gab, 2013/07/05 13:47:24]

Ah, yes, my bad I hadn't noticed this called into
CreateOrUpdateShortcutsForProfileAtPath() somehow thought it called straight
into CreateOrUpdateDesktopShortcutsAndIconForProfile() -- gah, these names are
all alike..!

By looking at it some more, it feels like the avatar resources need some logic
only available on the UI thread (e.g., ProfileInfoCache) to determine which icon
they should use exactly before copying the resources to make them available to
the FILE thread.
This is not trivial and I no longer think we should change this code here (we
could perhaps refactor this later if needed, but I think this is beyond the
scope of this CL -- unless you guys see something trivial which I don't?).

[Alexei Svitkine (slow), 2013/07/05 15:14:24]

Right, I remember now why I was okay with the current approach of re-using the
same function. Since Gab is fine with it now too, let's leave it the way it is
for this part.

+                                          IGNORE_NON_PROFILE_SHORTCUTS,
+                                          callback);
+}
+
 void ProfileShortcutManagerWin::CreateProfileShortcut(
     const base::FilePath& profile_path) {
   CreateOrUpdateShortcutsForProfileAtPath(profile_path, CREATE_WHEN_NONE_FOUND,
-                                          IGNORE_NON_PROFILE_SHORTCUTS);
+                                          IGNORE_NON_PROFILE_SHORTCUTS,
+                                          base::Closure());
 }
 
 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,12 +687,14 @@ void ProfileShortcutManagerWin::HasProfileShortcuts(
 
 void ProfileShortcutManagerWin::OnProfileAdded(
     const base::FilePath& profile_path) {
+  CreateOrUpdateProfileIcon(profile_path, base::Closure());
   if (profile_manager_->GetProfileInfoCache().GetNumberOfProfiles() == 2) {
     // When the second profile is added, make existing non-profile shortcuts
     // point to the first profile and be badged/named appropriately.
     CreateOrUpdateShortcutsForProfileAtPath(GetOtherProfilePath(profile_path),
                                             UPDATE_EXISTING_ONLY,
-                                            UPDATE_NON_PROFILE_SHORTCUTS);
+                                            UPDATE_NON_PROFILE_SHORTCUTS,
+                                            base::Closure());
   }
 }
 
@@ -623,13 +710,15 @@ void ProfileShortcutManagerWin::OnProfileWasRemoved(
   // from an existing shortcut.
   const bool deleting_down_to_last_profile = (cache.GetNumberOfProfiles() == 1);
   if (deleting_down_to_last_profile) {
+    // This is needed to unbadge the icon.
     CreateOrUpdateShortcutsForProfileAtPath(cache.GetPathOfProfileAtIndex(0),
                                             UPDATE_EXISTING_ONLY,
-                                            IGNORE_NON_PROFILE_SHORTCUTS);
+                                            IGNORE_NON_PROFILE_SHORTCUTS,
+                                            base::Closure());
   }
 
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-                          base::Bind(&DeleteDesktopShortcutsAndIconFile,
+                          base::Bind(&DeleteDesktopShortcuts,
                                      profile_path,
                                      deleting_down_to_last_profile));
 }
@@ -638,13 +727,13 @@ void ProfileShortcutManagerWin::OnProfileNameChanged(
     const base::FilePath& profile_path,
     const string16& old_profile_name) {
   CreateOrUpdateShortcutsForProfileAtPath(profile_path, UPDATE_EXISTING_ONLY,
-                                          IGNORE_NON_PROFILE_SHORTCUTS);
+                                          IGNORE_NON_PROFILE_SHORTCUTS,
+                                          base::Closure());
 }
 
 void ProfileShortcutManagerWin::OnProfileAvatarChanged(
     const base::FilePath& profile_path) {
-  CreateOrUpdateShortcutsForProfileAtPath(profile_path, UPDATE_EXISTING_ONLY,
-                                          IGNORE_NON_PROFILE_SHORTCUTS);
+  CreateOrUpdateProfileIcon(profile_path, base::Closure());
 }
 
 base::FilePath ProfileShortcutManagerWin::GetOtherProfilePath(
@@ -661,32 +750,31 @@ base::FilePath ProfileShortcutManagerWin::GetOtherProfilePath(
 void ProfileShortcutManagerWin::CreateOrUpdateShortcutsForProfileAtPath(
     const base::FilePath& profile_path,
     CreateOrUpdateMode create_mode,
-    NonProfileShortcutAction action) {
+    NonProfileShortcutAction action,
+    const base::Closure& callback) {
+  CreateOrUpdateShortcutsParams params(profile_path, create_mode, 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;
 
-  string16 old_shortcut_appended_name =
+  params.old_profile_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() &&
+  if (params.old_profile_name.empty() &&
       create_mode == UPDATE_EXISTING_ONLY &&
       action == IGNORE_NON_PROFILE_SHORTCUTS) {
     return;
   }
 
-  string16 new_shortcut_appended_name;
-  if (!remove_badging)
-    new_shortcut_appended_name = cache->GetNameOfProfileAtIndex(profile_index);
-
-  SkBitmap avatar_bitmap_copy_1x;
-  SkBitmap avatar_bitmap_copy_2x;
   if (!remove_badging) {
+    params.profile_name = cache->GetNameOfProfileAtIndex(profile_index);
+
     const size_t icon_index =
         cache->GetAvatarIconIndexOfProfileAtIndex(profile_index);
     const int resource_id_1x =
@@ -694,16 +782,36 @@ void ProfileShortcutManagerWin::CreateOrUpdateShortcutsForProfileAtPath(
     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);
+    params.avatar_image_1x = GetImageResourceSkBitmapCopy(resource_id_1x);
+    params.avatar_image_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));
+      base::Bind(&CreateOrUpdateDesktopShortcutsAndIconForProfile, params,
+                 callback));
 
   cache->SetShortcutNameOfProfileAtIndex(profile_index,
-                                         new_shortcut_appended_name);
+                                         params.profile_name);
+}
+
+void ProfileShortcutManagerWin::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  switch (type) {

[Alexei Svitkine (slow), 2013/06/18 15:27:19]

How about:

if (type != chrome::NOTIFICATION_PROFILE_CREATED) {
  NOTREACHED();
  return;
}

... rest of the code follows.

Makes it more readable by avoiding extra nesting.

[calamity, 2013/06/27 08:10:11]

Other files such as:
-chrome/browser/android/chrome_web_contents_delegate_android.cc
-chrome/browser/bookmarks/bookmark_model.cc

follow this pattern for single case Observe implementations. I think it would be
better to maintain the pattern.

[gab, 2013/06/27 14:18:56]

Yea, I like the switch personally and have seen this pattern all over chrome.

[Alexei Svitkine (slow), 2013/06/27 15:53:08]

Fair enough, keep the switch then. :)

+    case chrome::NOTIFICATION_PROFILE_CREATED: {

[gab, 2013/06/27 14:18:56]

Add a comment above this stipulating your findings on when this gets called
exactly.

[gab, 2013/07/05 13:47:24]

Ping on this -- mostly for my sanity as I don't like to keep track of multiple
unfinished review cycles :).

[calamity, 2013/07/08 07:45:49]

Done.

+      Profile* profile =
+          content::Source<Profile>(source).ptr()->GetOriginalProfile();
+      if (profile->GetPrefs()->GetInteger(prefs::kProfileIconVersion) <

[Alexei Svitkine (slow), 2013/06/18 15:27:19]

I suggest moving this to another anonymous function next to
OnProfileIconCreateSuccess() that takes |profile| as a param.

That way, the two pieces of code that deal with profile icon version pref are
together.

[calamity, 2013/06/27 08:10:11]

Done.

+          kCurrentProfileIconVersion) {

[gab, 2013/06/18 15:36:58]

I think this condition needs to be check every time the profile loads
(NOTIFICATION_PROFILE_URL_REQUEST_CONTEXT_GETTER_INITIALIZED???); otherwise a
developer might be fooled into thinking that upping kCurrentProfileIconVersion
is sufficient to force an update which it is not with the current code.

[calamity, 2013/06/27 08:10:11]

NOTIFICATION_PROFILE_CREATED was recommended by sail@ above. Despite its name, I
think it's actually the profile load notification.

+        // Ensure the profile's icon file has been created.
+        CreateOrUpdateProfileIcon(
+            profile->GetPath(),
+            base::Bind(&OnProfileIconCreateSuccess, profile));
+      }
+      break;
+    }
+    default:
+      NOTREACHED();

[sail, 2013/06/18 05:10:21]

break;?

[calamity, 2013/06/27 08:10:11]

Done.

+  }
 }