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 119 matching lines @@
                                app_icon_bitmap.width() - sk_icon.width(),
                                app_icon_bitmap.height() - sk_icon.height());
   const SkBitmap& badged_bitmap =
       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.

[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) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   scoped_ptr<SkBitmap> app_icon_bitmap(GetAppIconForSize(kShortcutIconSize));
   if (!app_icon_bitmap.get())
     return base::FilePath();
 
   gfx::ImageFamily badged_bitmaps;
-  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()) {
+  if (!avatar_bitmap_1x.empty()) {
     badged_bitmaps.Add(gfx::Image::CreateFrom1xBitmap(
-        BadgeIcon(*app_icon_bitmap, avatar_bitmap_2x, 2)));
+        BadgeIcon(*app_icon_bitmap, avatar_bitmap_1x, 1)));
   }
 
+  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(*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;
 }
 
 // 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 &&
@@ skipping 114 matching lines @@
     if (file_util::PathExists(possible_new_system_shortcut))
       file_util::Delete(old_shortcut_path, false);
     else if (!RenameDesktopShortcut(old_shortcut_path, new_shortcut_path))
       DLOG(ERROR) << "Could not rename Windows profile desktop shortcut.";
   } else {
     // If the shortcut does not exist, it may have been renamed by the user. In
     // that case, its name should not be changed.
     // 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))
       file_util::CopyFile(possible_old_system_shortcut, new_shortcut_path);
   }
 }
 
 // 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(
+void CreateOrUpdateDesktopShortcutsAndIconForProfile(
     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 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();
     return;
   }
 
   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 13 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())
       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 =
@@ skipping 33 matching lines @@
   for (base::FilePath path = enumerator.Next(); !path.empty();
        path = enumerator.Next()) {
     if (IsChromeShortcut(path, chrome_exe, NULL))
       return true;
   }
 
   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

[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));
 
   base::FilePath chrome_exe;
   if (!PathService::Get(base::FILE_EXE, &chrome_exe)) {
     NOTREACHED();
     return;
   }
 
   const string16 command_line =
       profiles::internal::CreateProfileShortcutFlags(profile_path);
   std::vector<base::FilePath> shortcuts;
   ListDesktopShortcutsWithCommandLine(chrome_exe, command_line, false,
                                       &shortcuts);
 
   BrowserDistribution* distribution = BrowserDistribution::GetDistribution();
   for (size_t i = 0; i < shortcuts.size(); ++i) {
     // 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);
-
   // 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());
     installer::Product product(distribution);
@@ skipping 61 matching lines @@
   SkBitmap bitmap_copy;
   image_bitmap->deepCopyTo(&bitmap_copy, image_bitmap->getConfig());
   return bitmap_copy;
 }
 
 }  // namespace
 
 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,
                                        BrowserDistribution* distribution) {
   string16 shortcut_name;
   if (!profile_name.empty()) {
     shortcut_name.append(SanitizeShortcutProfileNameString(profile_name));
     shortcut_name.append(L" - ");
     shortcut_name.append(l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME));
   } else {
     shortcut_name.append(distribution->GetAppShortCutName());
@@ skipping 29 matching lines @@
       arraysize(kProfileAvatarIconResources2x),
       profile_manager_->GetProfileInfoCache().GetDefaultAvatarIconCount());
 
   profile_manager_->GetProfileInfoCache().AddObserver(this);
 }
 
 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,
                                           IGNORE_NON_PROFILE_SHORTCUTS);
 }
 
 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(
     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) {
   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);
+  } 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);
+    }
   }
 }
 
 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),
+    // 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));
 }
 
 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) {
-  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);
 }
 
 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);
@@ skipping 35 matching lines @@
     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,
+      base::Bind(&CreateOrUpdateDesktopShortcutsAndIconForProfile, 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);
 }