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

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 72256196a964929786ec37c968159783a6a13dde..3f8141e6808b77128c67884f9b6ba7f21ceede87 100644
--- a/chrome/browser/profiles/profile_shortcut_manager_win.cc
+++ b/chrome/browser/profiles/profile_shortcut_manager_win.cc
@@ -589,17 +589,21 @@ base::string16 SanitizeShortcutProfileNameString(
   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
@@ -808,15 +812,24 @@ void ProfileShortcutManagerWin::CreateOrUpdateShortcutsForProfileAtPath(
   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,