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 6d02c633533e49d4ca3cec7e603d2b5f223714f9..17a4fcf1cb11858c6380e225613489bf55ddd53b 100644
--- a/chrome/browser/profiles/profile_shortcut_manager_win.cc
+++ b/chrome/browser/profiles/profile_shortcut_manager_win.cc
@@ -139,8 +139,11 @@ 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.

[gab, 2013/05/07 12:38:39]

Why is "browser distribution" mentioned in this comment? It isn't used in this
method at all...

[calamity, 2013/05/08 08:15:42]

I think it means the blue chromium icon vs the google chrome vs the canary icon.

 // 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.
+// TODO(calamity): ideally we'd just copy the app icon verbatim from the exe's

[Alexei Svitkine (slow), 2013/05/03 17:20:47]

Nit: capitalize 'ideally'

[calamity, 2013/05/08 08:15:42]

Done.

+// 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) {
@@ -150,21 +153,36 @@ base::FilePath CreateChromeDesktopShortcutIconForProfile(
     return base::FilePath();
 
   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.get()) {
+  scoped_ptr<SkBitmap> large_app_icon_bitmap(
+      GetAppIconForSize(IconUtil::kLargeIconSize));
+  if (large_app_icon_bitmap.get() && !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.get()) {
+      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);
+  if (!IconUtil::CreateIconFileFromImageFamily(badged_bitmaps, icon_path)) {
+    NOTREACHED();
     return base::FilePath();
+  }
 
+  SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT,

[Alexei Svitkine (slow), 2013/05/03 17:20:47]

Add a comment mentioning why this is necessary. (To update any existing
shortcuts using the icon.)

[gab, 2013/05/07 12:38:39]

FYI, I have had issues with SHCNF_FLUSHNOWAIT in the past
(http://crrev.com/145987) where my guess it that: the flush would happen before
the Windows Shell was even notified of the shortcut creation (or maybe in my
specific case before the new PropertyStore was flushed out; which would result
in the shortcut appearing, but without the properties being written to the
PropertyStore...).

Perhaps notifying the shell first with SHChangeNotify(SHCNE_CREATE, SHCNF_PATH,
icon_path.c_str(), NULL) is safer here (this only makes sense if
CreateIconFileFromImageFamily() is synchronous, is it?)? (Or with
SHCNE_UPDATEITEM if the icon is being overwritten rather than created).

An SHCNE_ASSOCCHANGED notification should only be required imo if Chrome's
window is already telling the taskbar to use this icon (i.e., before it might
even exist); is that the case? Otherwise it's preferable to shy away from
SHCNE_ASSOCCHANGED as it results in a full desktop/taskbar icons flash.

[calamity, 2013/05/08 08:15:42]

CreateIconFileFromImageFamily is synchronous. I don't think there's an issue
with shortcut updates here because the icon file is maintained separately from
the shortcut creation. Does it make sense to notify the shell when the shortcut
hasn't been created? Should I try to create the shortcut ahead of the icon file?
 
This is possible on avatar change. The taskbar will already have the icon file
and we need to update it. I think SHCNE_ASSOCCHANGED is needed to invalidate the
icon cache and update the taskbar icon.

[gab, 2013/05/08 13:01:08]

SHChangeNotify(SHCNE_CREATE, SHCNF_PATH, icon_path.c_str(), NULL);
will be sufficient here just so that the shell knows the file exists when it is
used later.

In the startup sequence, is the icon created before SetAppIconForWindow ([1]) is
called? Otherwise the icon is indeed used before which forces us to do a
SHCNE_ASSOCCHANGED here :(...

We should also remove the runtime badging logic if this explicit icon is to
replace it (but I guess you planned to do this in a follow-up CL since I don't
see this icon being used yet?)

[1]
https://code.google.com/p/chromium/codesearch#search/&q=SetAppIconForWindow&s...
True, do we have a way to tell when this is the case? Perhaps rely on whether
the icon file existed already or not? (Ideally we only use SHCNE_ASSOCCHANGED
when absolutely necessary (and following user action since it is rather
disturbing, but slightly more acceptable if the user knows what triggered it)).

[calamity, 2013/05/09 06:12:16]

SetAppIconForWindow would probably be called in the constructor of browser.cc
since that's where the app id is set. Since this icon is created during profile
load, we won't have the browser at that point so I'm not sure how we'd know if
the SetAppIconForWindow had already been run at icon creation time. I could make
it synchronous by moving this icon creation into browser.cc and then having a
callback to browser that calls SetAppIconForWindow.
Avatar change is an event that's listened for by ProfileShortcutManagerWin so we
can definitely catch this case.

Given the information above, let me know how I should proceed with this
SHChangeNotify stuff.

[gab, 2013/05/09 13:43:56]

Ah right, makes sense, the current code is synchronous with profile creation
which is guaranteed to happen before a browser is created.

However I'm curious as to how this even works since chrome_browser_main.cc is
running on the UI thread I believe and you go straight to
CreateOrUpdateDesktopShortcutsAndIconForProfile() which DCHECKs it's on the FILE
thread... (it probably makes sense to only do this on the FILE thread too to
avoid delaying startup).

Currently Browsers do not even call SetAppIconForWindow() -- a follow-up CL will
actually need to do this and SetRelaunchDetailsForWindow() for pinning to work.

I'm not sure if callbacks are queued and guaranteed to happen in the order they
were queued on each thread, but if that's the case, then we can queue up
CreateProfileIcon on the FILE thread early and queue up SetAppIconForWindow()
the same way when creating a browser and the former should happen before the
later as desired...
The current code is perfect (i.e. only notify if the icon was created or
actually changed).

You can make it slightly better by only notifying SHCNE_CREATE when the icon is
created from scratch (i.e. the shell only needs to know about this new icon, not
refresh its whole cache), see
https://code.google.com/p/chromium/codesearch#chromium/src/base/win/shortcut....

+                 NULL, NULL);
   return icon_path;
 }
 
@@ -299,7 +317,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))
@@ -311,7 +330,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,
@@ -321,6 +340,17 @@ void CreateOrUpdateDesktopShortcutsForProfile(
     ProfileShortcutManagerWin::NonProfileShortcutAction action) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
+  base::FilePath shortcut_icon =

[Alexei Svitkine (slow), 2013/05/03 17:20:47]

Add a TODO that we should only update the icon if it doesn't exist or actually
needs to be updated (image change)?

[calamity, 2013/05/08 08:15:42]

Code to do this existed in web_app_win. I moved it to icon_util and used it
around the icon creation so that I could just call CreateProfileIcon on every
startup without having to worry about the SHChangeNotify unnecessarily flashing
the desktop.

+      CreateOrUpdateShortcutIconForProfile(profile_path,
+                                           avatar_image_1x,
+                                           avatar_image_2x);
+  if (shortcut_icon.empty()) {
+    NOTREACHED();
+    return;
+  }
+  if (create_mode == ProfileShortcutManagerWin::CREATE_ICON_ONLY)
+    return;
+
   base::FilePath chrome_exe;
   if (!PathService::Get(base::FILE_EXE, &chrome_exe)) {
     NOTREACHED();
@@ -354,10 +384,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);
@@ -414,7 +440,7 @@ bool ChromeDesktopShortcutsExist(const base::FilePath& chrome_exe) {
 // corresponding icon file. If |ensure_shortcuts_remain| is true, then a regular

[Alexei Svitkine (slow), 2013/05/03 17:20:47]

Update this comment.

[calamity, 2013/05/08 08:15:42]

Done.

 // 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) {

[gab, 2013/05/07 12:38:39]

nit: Indent.

[calamity, 2013/05/08 08:15:42]

Done.

   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
@@ -440,10 +466,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();
@@ -525,6 +547,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);

[gab, 2013/05/07 12:38:39]

nit: Remove profiles::internal:: namespace as this code is already in it.

[calamity, 2013/05/08 08:15:42]

Done.

+}
+
 const char kProfileIconFileName[] = "Google Profile.ico";

[gab, 2013/05/07 12:38:39]

nit: constants should remain at the top of the namespace

Also, since this is constant no longer needs to be accessed from outside of this
file, move it up into the unnamed namespace of this file.

[calamity, 2013/05/08 08:15:42]

Done.

 
 string16 GetShortcutFilenameForProfile(const string16& profile_name,
@@ -575,6 +601,12 @@ ProfileShortcutManagerWin::~ProfileShortcutManagerWin() {
   profile_manager_->GetProfileInfoCache().RemoveObserver(this);
 }
 
+void ProfileShortcutManagerWin::CreateProfileIcon(
+    const base::FilePath& profile_path) {
+  CreateOrUpdateShortcutsForProfileAtPath(profile_path, CREATE_ICON_ONLY,
+                                          IGNORE_NON_PROFILE_SHORTCUTS);
+}
+
 void ProfileShortcutManagerWin::CreateProfileShortcut(
     const base::FilePath& profile_path) {
   CreateOrUpdateShortcutsForProfileAtPath(profile_path, CREATE_WHEN_NONE_FOUND,
@@ -585,7 +617,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(
@@ -604,10 +636,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);
+    }
   }
 }
 
@@ -623,13 +659,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,

[gab, 2013/05/07 12:38:39]

Why was this un-inlined? |profile_path| only seems to be used once still...

[calamity, 2013/05/08 08:15:42]

Done.

                                             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));
 }
@@ -643,7 +681,7 @@ void ProfileShortcutManagerWin::OnProfileNameChanged(
 
 void ProfileShortcutManagerWin::OnProfileAvatarChanged(
     const base::FilePath& profile_path) {
-  CreateOrUpdateShortcutsForProfileAtPath(profile_path, UPDATE_EXISTING_ONLY,
+  CreateOrUpdateShortcutsForProfileAtPath(profile_path, CREATE_ICON_ONLY,

[gab, 2013/05/07 12:38:39]

Should still update the desktop shortcut or will updating the icon and notifying
the shell result in an updated shortcut?

[calamity, 2013/05/08 08:15:42]

Updating and notifying refreshes the shortcut.

                                           IGNORE_NON_PROFILE_SHORTCUTS);
 }
 
@@ -699,7 +737,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));