Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
462713002:
[Win] Profile desktop shortcuts should use the Gaia avatar if it's in use. (Closed)
|
Created:
6 years, 4 months ago by noms (inactive) Modified:
6 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Win] Profile desktop shortcuts should use the Gaia avatar if it's in use.
With --google-profile-info or --enable-new-avatar-menu turned on, signed in
profiles can use the account's Gaia image as the profile avatar. The
desktop shortcut shouldn't be surprised about this :)
Screenshot of the badged shortcuts in all their glory:
https://drive.google.com/folderview?id=0B1B1Up4p2NRMUjlTSU1EYUlBVG8&usp=sharing
BUG=402162
TEST=Flip --google-profile-info or --enable-new-avatar-menu. Sign in a profile,
and change the profile's avatar to the account's Gaia image. The desktop shortcut
should update and use the Gaia image correctly.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289100
Patch Set 1 #
Total comments: 18
Patch Set 2 : everyone gets a scoped_ptr #Patch Set 3 : alexei comments and revert them scoped_ptrs #
Total comments: 6
Patch Set 4 : rebase #Patch Set 5 : gab + alexei comments #Patch Set 6 : moved dcheck #
Total comments: 4
Patch Set 7 : s/_eq/_ge #
Messages
Total messages: 25 (0 generated)
Hi Gab, Added support for Gaia avatars to desktop shortcuts. Please take a look. Thanks!
Comments below, you'll also want Alexei to take a look. Cheers, Gab https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:74: const int kCurrentProfileIconVersion = 2; 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. https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:598: return bitmap_copy; 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). https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:600: // Returns a copied SkBitmap for the given resource id that can be safely passed nit: empty line above https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:821: params.avatar_image_2x = GetSkBitmapCopy(*image); You use the same image for the 2x image?
https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:598: return bitmap_copy; 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: > 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). https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:821: params.avatar_image_2x = GetSkBitmapCopy(*image); Yup, we only download one image from the userInfo service. On 2014/08/11 15:27:59, gab wrote: > You use the same image for the 2x image?
https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:598: return bitmap_copy; 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: > 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: > > 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). > https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:821: params.avatar_image_2x = GetSkBitmapCopy(*image); 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: > Yup, we only download one image from the userInfo service. > > On 2014/08/11 15:27:59, gab wrote: > > You use the same image for the 2x image? >
Should the profile manager listen for when a user signs in, so that it could update the icon?
On 2014/08/11 19:00:08, Alexei Svitkine wrote: > Should the profile manager listen for when a user signs in, so that it could > update the icon? The profile shortcut manager, that is.
Hmmm, just because a user signs in doesn't mean the user might elect to use the Gaia avatar -- for example they could have synced Lemonade as their avatar. All I am adding is code that *if* the avatar in use is actually the Gaia one, we should be able to badge the shortcut (at the moment we are badging it with the previously used avatar, which the ProfileInfoCache remembers as well). On 2014/08/11 19:00:18, Alexei Svitkine wrote: > On 2014/08/11 19:00:08, Alexei Svitkine wrote: > > Should the profile manager listen for when a user signs in, so that it could > > update the icon? > > The profile shortcut manager, that is.
I see, so at the moment it's possible for the profile to be represented by a GAIA picture while the profile shortcut is badged with an old-style-avatar? And this change fixes that? If so then you should update kCurrentProfileIconVersion to 3 so that all existing profile shortcuts get fixed (instead of forcing the user to go through an avatar change for it to get updated). On 2014/08/11 19:02:14, Monica Dinculescu wrote: > Hmmm, just because a user signs in doesn't mean the user might elect to use the > Gaia avatar -- for example they could have synced Lemonade as their avatar. > > All I am adding is code that *if* the avatar in use is actually the Gaia one, we > should be able to badge the shortcut (at the moment we are badging it with the > previously used avatar, which the ProfileInfoCache remembers as well). > > On 2014/08/11 19:00:18, Alexei Svitkine wrote: > > On 2014/08/11 19:00:08, Alexei Svitkine wrote: > > > Should the profile manager listen for when a user signs in, so that it could > > > update the icon? > > > > The profile shortcut manager, that is.
Yup, that's exactly correct. I have a patch ++'ing the version, but everything is crashing because of refptrs, so it's not ready for public consumption yet :) On 2014/08/11 19:05:40, gab wrote: > I see, so at the moment it's possible for the profile to be represented by a > GAIA picture while the profile shortcut is badged with an old-style-avatar? And > this change fixes that? > > If so then you should update kCurrentProfileIconVersion to 3 so that all > existing profile shortcuts get fixed (instead of forcing the user to go through > an avatar change for it to get updated). > > > On 2014/08/11 19:02:14, Monica Dinculescu wrote: > > Hmmm, just because a user signs in doesn't mean the user might elect to use > the > > Gaia avatar -- for example they could have synced Lemonade as their avatar. > > > > All I am adding is code that *if* the avatar in use is actually the Gaia one, > we > > should be able to badge the shortcut (at the moment we are badging it with the > > previously used avatar, which the ProfileInfoCache remembers as well). > > > > On 2014/08/11 19:00:18, Alexei Svitkine wrote: > > > On 2014/08/11 19:00:08, Alexei Svitkine wrote: > > > > Should the profile manager listen for when a user signs in, so that it > could > > > > update the icon? > > > > > > The profile shortcut manager, that is.
Sorry, to be clear, I meant "should we listen for the event where a profile switches its should_use_gaia_pic state"? Otherwise, when that setting changes for a user, then they'll have old icons. Or is it somehow not possible for that setting to change? On Mon, Aug 11, 2014 at 3:07 PM, <noms@chromium.org> wrote: > Yup, that's exactly correct. > > I have a patch ++'ing the version, but everything is crashing because of > refptrs, so it's not ready for public consumption yet :) > > > On 2014/08/11 19:05:40, gab wrote: > >> I see, so at the moment it's possible for the profile to be represented >> by a >> GAIA picture while the profile shortcut is badged with an >> old-style-avatar? >> > And > >> this change fixes that? >> > > If so then you should update kCurrentProfileIconVersion to 3 so that all >> existing profile shortcuts get fixed (instead of forcing the user to go >> > through > >> an avatar change for it to get updated). >> > > > On 2014/08/11 19:02:14, Monica Dinculescu wrote: >> > Hmmm, just because a user signs in doesn't mean the user might elect to >> use >> the >> > Gaia avatar -- for example they could have synced Lemonade as their >> avatar. >> > >> > All I am adding is code that *if* the avatar in use is actually the Gaia >> > one, > >> we >> > should be able to badge the shortcut (at the moment we are badging it >> with >> > the > >> > previously used avatar, which the ProfileInfoCache remembers as well). >> > >> > On 2014/08/11 19:00:18, Alexei Svitkine wrote: >> > > On 2014/08/11 19:00:08, Alexei Svitkine wrote: >> > > > Should the profile manager listen for when a user signs in, so that >> it >> could >> > > > update the icon? >> > > >> > > The profile shortcut manager, that is. >> > > > > https://codereview.chromium.org/462713002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Oh! We listen to OnProfileAvatarChanged, which is triggered by ProfileInfoCache::SetGAIAPictureOfProfileAtIndex and SetIsUsingGAIAPictureOfProfileAtIndex, which is the scenario when the profile switches its Gaia avatar OR elects for the first time to use the Gaia avatar. https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:74: const int kCurrentProfileIconVersion = 2; On 2014/08/11 15:27:59, gab wrote: > 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. Done. https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:600: // Returns a copied SkBitmap for the given resource id that can be safely passed On 2014/08/11 15:27:59, gab wrote: > nit: empty line above Done. https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:821: params.avatar_image_2x = GetSkBitmapCopy(*image); 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: > 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: > > Yup, we only download one image from the userInfo service. > > > > On 2014/08/11 15:27:59, gab wrote: > > > You use the same image for the 2x image? > > >
https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:820: params.avatar_image_1x = GetSkBitmapCopy(*image); 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? https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:821: params.avatar_image_2x = GetSkBitmapCopy(*image); On 2014/08/11 19:14:22, Monica Dinculescu wrote: > 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. Can you post pictures on the bug of how this looks on the desktop both in Large Icon and regular modes?
Fixed a test! Added a screenshot! Added scoped_ptrs! Please take a look while I'm also sorting out Alexei's yak shave :) https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:598: return bitmap_copy; On 2014/08/11 16:13:09, gab wrote: > 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: > > 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: > > > 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). > > > Done. https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:821: params.avatar_image_2x = GetSkBitmapCopy(*image); On 2014/08/11 19:27:39, Alexei Svitkine wrote: > On 2014/08/11 19:14:22, Monica Dinculescu wrote: > > 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. Also added to the description: https://drive.google.com/folderview?id=0B1B1Up4p2NRMUjlTSU1EYUlBVG8&usp=sharing > > Can you post pictures on the bug of how this looks on the desktop both in Large > Icon and regular modes?
https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:598: return bitmap_copy; On 2014/08/11 15:27:59, gab wrote: > 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). 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.
https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:598: return bitmap_copy; On 2014/08/11 20:02:31, Alexei Svitkine wrote: > On 2014/08/11 15:27:59, gab wrote: > > 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). > > 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. 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.
https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/462713002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:598: return bitmap_copy; Reverted! :( On 2014/08/11 20:29:44, gab wrote: > On 2014/08/11 20:02:31, Alexei Svitkine wrote: > > On 2014/08/11 15:27:59, gab wrote: > > > This will create a copy of the SkBitmap (from the spec it looks like this > copyReverted! > > > 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). > > > > 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. > > 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.
lgtm w/ small request https://codereview.chromium.org/462713002/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/462713002/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_shortcut_manager_win.cc:822: params.avatar_image_2x = params.avatar_image_1x; Maybe add a comment about explaining why it's okay to re-use the same picture? Even better, a DCHECK that the size is bigger then what's required for the 2x picture?
https://codereview.chromium.org/462713002/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/462713002/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_shortcut_manager_win.cc:606: DCHECK(!image.IsEmpty()); Nit: Move this check to GetSkBitmapCopy() since this is what image.ToSkBitmap() documents as being required. https://codereview.chromium.org/462713002/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_shortcut_manager_win.cc:820: cache->GetGAIAPictureOfProfileAtIndex(profile_index); According to the docs for that method: // Returns the GAIA picture for the given profile. This may return NULL // if the profile does not have a GAIA picture or if the picture must be // loaded from disk. I don't see a NULL check in your code, but the comment for the method sounds like we should have one (though it's not clear to me what's the best way to handle it - I guess if there's no GAIA picture than using the other icon is fine, but if it's just not loaded... perhaps it needs to load it?). Or perhaps the comment on that API method isn't quite accurate?
https://codereview.chromium.org/462713002/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/462713002/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_shortcut_manager_win.cc:606: DCHECK(!image.IsEmpty()); On 2014/08/12 14:54:07, Alexei Svitkine wrote: > Nit: Move this check to GetSkBitmapCopy() since this is what image.ToSkBitmap() > documents as being required. Done. https://codereview.chromium.org/462713002/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_shortcut_manager_win.cc:820: cache->GetGAIAPictureOfProfileAtIndex(profile_index); On 2014/08/12 14:54:07, Alexei Svitkine wrote: > According to the docs for that method: > > // Returns the GAIA picture for the given profile. This may return NULL > // if the profile does not have a GAIA picture or if the picture must be > // loaded from disk. > > I don't see a NULL check in your code, but the comment for the method sounds > like we should have one (though it's not clear to me what's the best way to > handle it - I guess if there's no GAIA picture than using the other icon is > fine, but if it's just not loaded... perhaps it needs to load it?). Or perhaps > the comment on that API method isn't quite accurate? Done. https://codereview.chromium.org/462713002/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_shortcut_manager_win.cc:822: params.avatar_image_2x = params.avatar_image_1x; On 2014/08/11 20:49:34, gab wrote: > Maybe add a comment about explaining why it's okay to re-use the same picture? > > Even better, a DCHECK that the size is bigger then what's required for the 2x > picture? Done.
Very nice, one last question below (still lgtm though) https://codereview.chromium.org/462713002/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/462713002/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:821: cache->GetGAIAPictureOfProfileAtIndex(profile_index); When would IsUsingGAIAPictureOfProfileAtIndex() return true yet GetGAIAPictureOfProfileAtIndex() return NULL? If an async task isn't complete perhaps? How do we ensure the shortcut is updated when the picture becomes available?
lgtm https://codereview.chromium.org/462713002/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/462713002/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:826: DCHECK(image->Width() >= IconUtil::kLargeIconSize); Nit: Can this use DCHECK_GE?
https://codereview.chromium.org/462713002/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/462713002/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:821: cache->GetGAIAPictureOfProfileAtIndex(profile_index); When the file is saved, the ProfileInfoCache sends a OnProfileAvatarChanged notification, which we listen to and update the shortcut. I think we set "IsUsingGAIAPictureOfProfileAtIndex" independently of when we save the file (we could revert to not using the gaia picture, so just flip the IsUsing boolean, but GetGAIAPicture would still return a picture, even though we're not using). I imagine GetGAIAPicture could return NULL if the file is hosed, maybe, and can't be saved? Not much we can do in that case ... On 2014/08/12 18:56:17, gab wrote: > When would IsUsingGAIAPictureOfProfileAtIndex() return true yet > GetGAIAPictureOfProfileAtIndex() return NULL? > > If an async task isn't complete perhaps? > > How do we ensure the shortcut is updated when the picture becomes available? https://codereview.chromium.org/462713002/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:826: DCHECK(image->Width() >= IconUtil::kLargeIconSize); On 2014/08/12 18:57:56, Alexei Svitkine wrote: > Nit: Can this use DCHECK_GE? Done.
The CQ bit was checked by noms@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/462713002/120001
Message was sent while issue was closed.
Change committed as 289100 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
