[Win] Profile desktop shortcuts should use the Gaia avatar if it's in use.

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 53 matching lines @@
 // differently than it was when a shortcut was originally created.
 const int kMaxProfileShortcutFileNameLength = 64;
 
 // The avatar badge size needs to be half of the shortcut icon size because
 // the Windows taskbar icon is 32x32 and the avatar icon overlay is 16x16. So to
 // get the shortcut avatar badge and the avatar icon overlay to match up, we
 // need to preserve those ratios when creating the shortcut icon.
 const int kShortcutIconSize = 48;
 const int kProfileAvatarBadgeSize = kShortcutIconSize / 2;
 
 const int kCurrentProfileIconVersion = 2;

[gab, 2014/08/11 15:27:59]

If the icon is changed after the profile shortcut was created, you'll have to
update this (or the algorithm using this value) to force an icon refresh on the
existing shortcuts.

[noms (inactive), 2014/08/11 19:14:22]

Done.

 
 // 2x sized profile avatar icons. Mirrors |kDefaultAvatarIconResources| in
 // profile_info_cache.cc.
 const int kProfileAvatarIconResources2x[] = {
   IDR_PROFILE_AVATAR_2X_0,
   IDR_PROFILE_AVATAR_2X_1,
   IDR_PROFILE_AVATAR_2X_2,
   IDR_PROFILE_AVATAR_2X_3,
   IDR_PROFILE_AVATAR_2X_4,
   IDR_PROFILE_AVATAR_2X_5,
@@ skipping 497 matching lines @@
   }
 
   base::TrimWhitespace(sanitized, base::TRIM_LEADING, &sanitized);
   if (sanitized.size() > kMaxProfileShortcutFileNameLength)
     sanitized.erase(kMaxProfileShortcutFileNameLength);
   base::TrimWhitespace(sanitized, base::TRIM_TRAILING, &sanitized);
 
   return sanitized;
 }
 
+// Returns a copied SkBitmap for the given image that can be safely passed to
+// another thread.
+SkBitmap GetSkBitmapCopy(const gfx::Image& image) {
+  const SkBitmap* image_bitmap = image.ToSkBitmap();
+  SkBitmap bitmap_copy;
+  image_bitmap->deepCopyTo(&bitmap_copy);
+  return bitmap_copy;

[gab, 2014/08/11 15:27:59]

This will create a copy of the SkBitmap (from the spec it looks like this copy
will still share the pixels so it may be "okay", but ideally you'd make this
explicit by using a scoped_ptr<SkBitmap> return value).

[noms (inactive), 2014/08/11 15:44:39]

I just moved the code from one function to another -- I can make this change,
but I have no idea what the original intent behind it was :)

On 2014/08/11 15:27:59, gab wrote:

[gab, 2014/08/11 16:13:09]

Ah indeed GetImageResourceSkBitmapCopy() already returns a copy, but now there's
another layer of return-by-copy making this worse; worth using scoped_ptr's IMO,
the result is the same, but you pass the object itself around rather than a copy
of a temporary object...


On 2014/08/11 15:44:39, Monica Dinculescu wrote:

[noms (inactive), 2014/08/11 19:57:07]

Done.

[Alexei Svitkine (slow), 2014/08/11 20:02:31]

I don't think we need extra scoped_ptr's here. SkBitmaps are safe to return by
value because they share pixels underneath. (This is what I've heard from Skia
reviewers before). Sorry, should have jumped in and mentioned it before Monica
made the change.

[gab, 2014/08/11 20:29:44]

Ah ok, I'd noticed they were sharing pixels, but it looked like they had many
other fields so I wasn't convinced... if the Skia guys say so then I agree it's
fine.

[noms (inactive), 2014/08/11 20:37:53]

Reverted! :(

On 2014/08/11 20:29:44, gab wrote:

+}
 // Returns a copied SkBitmap for the given resource id that can be safely passed

[gab, 2014/08/11 15:27:59]

nit: empty line above

[noms (inactive), 2014/08/11 19:14:22]

Done.

 // to another thread.
 SkBitmap GetImageResourceSkBitmapCopy(int resource_id) {
   const gfx::Image image =
       ResourceBundle::GetSharedInstance().GetNativeImageNamed(resource_id);
   DCHECK(!image.IsEmpty());
-
-  const SkBitmap* image_bitmap = image.ToSkBitmap();
-  SkBitmap bitmap_copy;
-  image_bitmap->deepCopyTo(&bitmap_copy);
-  return bitmap_copy;
+  return GetSkBitmapCopy(image);
 }
 
 }  // namespace
 
 namespace profiles {
 namespace internal {
 
 base::FilePath GetProfileIconPath(const base::FilePath& profile_path) {
   return profile_path.AppendASCII(kProfileIconFileName);
 }
@@ skipping 188 matching lines @@
   // set in the profile info cache.
   if (params.old_profile_name.empty() &&
       create_mode == UPDATE_EXISTING_ONLY &&
       action == IGNORE_NON_PROFILE_SHORTCUTS) {
     return;
   }
 
   if (!remove_badging) {
     params.profile_name = cache->GetNameOfProfileAtIndex(profile_index);
 
-    const size_t icon_index =
-        cache->GetAvatarIconIndexOfProfileAtIndex(profile_index);
-    const int resource_id_1x =
-        profiles::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.
-    params.avatar_image_1x = GetImageResourceSkBitmapCopy(resource_id_1x);
-    params.avatar_image_2x = GetImageResourceSkBitmapCopy(resource_id_2x);
+    // The profile might be using the Gaia avatar, which is not in the
+    // resources array.
+    if (cache->IsUsingGAIAPictureOfProfileAtIndex(profile_index)) {
+      const gfx::Image* image =
+          cache->GetGAIAPictureOfProfileAtIndex(profile_index);
+      params.avatar_image_1x = GetSkBitmapCopy(*image);

[Alexei Svitkine (slow), 2014/08/11 19:27:39]

To avoid copying the image twice, is it possible to change the downstream code
to use the same image if the 2x image specified is empty?

+      params.avatar_image_2x = GetSkBitmapCopy(*image);

[gab, 2014/08/11 15:27:59]

You use the same image for the 2x image?

[noms (inactive), 2014/08/11 15:44:39]

Yup, we only download one image from the userInfo service.

On 2014/08/11 15:27:59, gab wrote:

[gab, 2014/08/11 16:13:09]

Okay but shouldn't one of them be scaled up/down? Aren't these fixed size
bitmaps? (I == graphics noob)

On 2014/08/11 15:44:39, Monica Dinculescu wrote:

[noms (inactive), 2014/08/11 19:14:22]

The Gaia image is way bigger than the 1x or 2x avatars, so I think it will get
scaled down correctly (i.e. prettily) either way. The reason that you want a 2x
version is so that it's shiny on the retina. The resizing is done in BadgeIcon,
so all these avatars are way to big for the shortcut badge at this point in the
code, which is why I don't think any resizing is needed.

On 2014/08/11 16:13:09, gab wrote:

[Alexei Svitkine (slow), 2014/08/11 19:27:39]

Can you post pictures on the bug of how this looks on the desktop both in Large
Icon and regular modes?

[noms (inactive), 2014/08/11 19:57:07]

Also added to the description:
https://drive.google.com/folderview?id=0B1B1Up4p2NRMUjlTSU1EYUlBVG8&usp=sharing

+    } else {
+      const size_t icon_index =
+          cache->GetAvatarIconIndexOfProfileAtIndex(profile_index);
+      const int resource_id_1x =
+          profiles::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.
+      params.avatar_image_1x = GetImageResourceSkBitmapCopy(resource_id_1x);
+      params.avatar_image_2x = GetImageResourceSkBitmapCopy(resource_id_2x);
+    }
   }
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&CreateOrUpdateDesktopShortcutsAndIconForProfile, params));
 
   cache->SetShortcutNameOfProfileAtIndex(profile_index,
                                          params.profile_name);
 }
 
 void ProfileShortcutManagerWin::Observe(
@@ skipping 10 matching lines @@
         // Ensure the profile's icon file has been created.
         CreateOrUpdateProfileIcon(profile->GetPath());
       }
       break;
     }
     default:
       NOTREACHED();
       break;
   }
 }