Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
222313005:
[Profiles] Download high-res avatars using the --new-profile-management flag (Closed)
|
Created:
6 years, 8 months ago by noms (inactive) Modified:
6 years, 7 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, Finnur, Aaron Boodman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDownload the high-res avatars when using the --new-profile-management flag
The high res avatar is downloaded:
- when we launch a new browser for a profile
- when we switch the avatar for a profile.
For now we re-download the file every time one of these actions is taken,
regardless of whether the file exists on disk. Since this is still in development
and the avatar resources keep changing, we want to make sure someone isn't
stuck with an ugly resource. There's a todo in the code to stop doing this as
soon as the resources settle a bit :)
BUG=305048
TEST=Start Chrome with --new-profile-management. Create a new profile.
In the avatar bubble, the active profile card should show a high-res (gray
background) avatar. If you go to the user manager, the avatar displayed
should be the same high res one (and not at all pixelated one). To double
check, the file for that avatar should have been added to
$USER_DATA_DIR/Avatars/
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267529
Patch Set 1 : #
Total comments: 2
Patch Set 2 : rebase #Patch Set 3 : fix broken test, add a test(!), rachel nit & use correct cmd flag #Patch Set 4 : typo #
Total comments: 38
Patch Set 5 : msw comments #
Total comments: 12
Patch Set 6 : msw comments part 2 #
Total comments: 9
Patch Set 7 : msw nits #
Total comments: 2
Patch Set 8 : fix broken unit tests & make new test better #
Total comments: 4
Patch Set 9 : eeeee final nits! ^____^ #Patch Set 10 : rebase #Messages
Total messages: 30 (0 generated)
Hi Rachel, Here's the last CL about downloading high res avatars, that actually downloads the files! I don't have a test yet (working on it!), but I wanted to get you looking at it sooner than later, just in case I'm doing all bad things in the ProfileInfoCache. I've tried to consolidate the Gaia profile/high res avatar saving and loading functions. Ideally these should be possibly taken out into a class that deals with all of the loading/saving/decoding stuff, but I'm not sure if it should be in this CL.
∩∩ （ﾟωﾟ） Ping! │ │ │ └─┐○ ヽ 丿 ∥￣∥
LGTM with a nit You might consider breaking out the bitmap_fetcher stuff into a separate CL. https://codereview.chromium.org/222313005/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.h (right): https://codereview.chromium.org/222313005/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.h:141: // when the download comples and OnAvatarPictureSaved is called, or when comples -> completes?
+ Mike for owner stamp on chrome/browser/ui. Rachel: I've also added a unit test to the ProfileInfoCache, as I was having a pretty guilty conscience submitting this without a test :) Please take a look. Thanks! https://codereview.chromium.org/222313005/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.h (right): https://codereview.chromium.org/222313005/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.h:141: // when the download comples and OnAvatarPictureSaved is called, or when On 2014/04/24 23:40:32, rpetterson wrote: > comples -> completes? Done.
This CL sure does a lot, did you consider Rachel's comment: "You might consider breaking out the bitmap_fetcher stuff into a separate CL."? https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_downloader.cc (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.cc:39: if (!bitmap) { nit: drop unnecessary braces. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.cc:44: // |cached_avatar_images_|. nit: mention ProfileInfoCache, not an internal member, cached_avatar_images_. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.cc:46: cache_->SaveAvatarImageAtPath(image, Is the cache guaranteed to be alive when this is called? https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_icon_util.cc:340: return image_path; nit: inline the return value. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:855: std::string key = profiles::GetDefaultAvatarIconFileNameAtIndex(avatar_index); This is no longer checked against GetNoHighResAvatarFileName , is that okay? (I guess this is somehow related to GetPathOfHighResAvatarAtIndex using GetDefaultAvatarIconResourceInfo instead of GetDefaultAvatarIconFileNameAtIndex for the image path?) https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:907: nit: remove new blank line https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:912: chrome::NOTIFICATION_PROFILE_CACHE_PICTURE_SAVED, nit: these should be indented two more spaces. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:949: void ProfileInfoCache::DownloadHighResAvatar( The order of all the function definitions in this file should match the declaration order in the header. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:965: nit: remove blank line https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:975: bool* success = new bool; Why is this allocated here? SaveBitmap can call OnAvatarPictureSaved with its own success value. I realize you haven't changed the original pattern, but I would encourage you to revise this to something sane. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... File chrome/browser/profiles/profiles_state.cc (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profiles_state.cc:119: // TODO(noms): We should check whether the file already exists on disk This seems like a fairly straightforward duty of ProfileInfoCache, is it really important to land this CL before it can avoid re-downloading high-res resources? https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profiles_state.cc:121: // the resources are still changing, re-download it every time the profile What is the plan going forward if the resources change? Do we plan to always re-download the resource, use versions/timestamps, or naively expected resources to never change and allow some clients to go out of date if they do? How is this handled for other downloaded resources? https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profiles_state.cc:129: ProfileAvatarDownloader* avatar_dowloader = new ProfileAvatarDownloader( Is this object leaked if DownloadHighResAvatar returns early because a download of the same file is already in progress? Perhaps pass a scoped_ptr, or even better, have DownloadHighResAvatar construct its own ProfileAvatarDownloader internally, only when needed. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profiles_state.cc:130: g_browser_process->system_request_context(), Can't ProfileAvatarDownloader obtain this value internally? Instead of passing values associated with |g_browser_process| or TestingBrowserProcess, ProfileInfoCache should have a SetBrowserProcessForTest helper that will override it's internal use of either value, or similar. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... File chrome/browser/profiles/profiles_state.h (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profiles_state.h:51: // If the file doesn't already exist, starts a download for the high res This seems out of place here. Aren't icon indices generally disliked? https://codereview.chromium.org/222313005/diff/190001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:275: // If needed, start downloading the high-res avatar. Is this actually needed on browser launch? Is there a better profile/UI initialization codepath to start this task? I don't think I'd be comfortable approving this change as-is. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_handler.cc (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_handler.cc:353: if (switches::IsNewAvatarMenu()) { nit: drop unnecessary braces. https://codereview.chromium.org/222313005/diff/190001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/222313005/diff/190001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:1759: 'browser/profiles/profile_avatar_downloader.cc', You need to add the header file too...
Oh, I did break up the BitmapFetcher changes. They went here: https://codereview.chromium.org/253623002/ :)
Hi Mike, Thanks for the review; it was really helpful! I've addressed all of the comments, and I think things are looking better. Please take another look. :) https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_downloader.cc (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.cc:39: if (!bitmap) { On 2014/04/28 20:46:37, msw wrote: > nit: drop unnecessary braces. Done. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.cc:44: // |cached_avatar_images_|. On 2014/04/28 20:46:37, msw wrote: > nit: mention ProfileInfoCache, not an internal member, cached_avatar_images_. Done. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.cc:46: cache_->SaveAvatarImageAtPath(image, It should be. I have added a check to bail early if this is not the case, though. On 2014/04/28 20:46:37, msw wrote: > Is the cache guaranteed to be alive when this is called? https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_icon_util.cc:340: return image_path; On 2014/04/28 20:46:37, msw wrote: > nit: inline the return value. Done. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:855: std::string key = profiles::GetDefaultAvatarIconFileNameAtIndex(avatar_index); Argh, no, that was a rebase disaster. Thank you for catching it! Fixed. On 2014/04/28 20:46:37, msw wrote: > This is no longer checked against GetNoHighResAvatarFileName , is that okay? (I > guess this is somehow related to GetPathOfHighResAvatarAtIndex using > GetDefaultAvatarIconResourceInfo instead of GetDefaultAvatarIconFileNameAtIndex > for the image path?) https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:907: On 2014/04/28 20:46:37, msw wrote: > nit: remove new blank line Done. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:912: chrome::NOTIFICATION_PROFILE_CACHE_PICTURE_SAVED, On 2014/04/28 20:46:37, msw wrote: > nit: these should be indented two more spaces. Done. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:949: void ProfileInfoCache::DownloadHighResAvatar( On 2014/04/28 20:46:37, msw wrote: > The order of all the function definitions in this file should match the > declaration order in the header. Done. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:965: On 2014/04/28 20:46:37, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:975: bool* success = new bool; I've changed it to passing the callback, and only running it in the successful case. Is this better? On 2014/04/28 20:46:37, msw wrote: > Why is this allocated here? SaveBitmap can call OnAvatarPictureSaved with its > own success value. I realize you haven't changed the original pattern, but I > would encourage you to revise this to something sane. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... File chrome/browser/profiles/profiles_state.cc (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profiles_state.cc:119: // TODO(noms): We should check whether the file already exists on disk We'd like to test it for a bit, given that it's such a giant change (it's also been in review for a month, so I was trying to get the smallest CL possible out). On 2014/04/28 20:46:37, msw wrote: > This seems like a fairly straightforward duty of ProfileInfoCache, is it really > important to land this CL before it can avoid re-downloading high-res resources? https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profiles_state.cc:121: // the resources are still changing, re-download it every time the profile I don't think we plan to do any versioning. The idea is that the resources will not change once this feature goes out (much like the avatars haven't change since they got introduced). If the avatars need to be redesigned any time in the future, we will rename the files, and get the new ones like that. On 2014/04/28 20:46:37, msw wrote: > What is the plan going forward if the resources change? Do we plan to always > re-download the resource, use versions/timestamps, or naively expected resources > to never change and allow some clients to go out of date if they do? How is this > handled for other downloaded resources? https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profiles_state.cc:129: ProfileAvatarDownloader* avatar_dowloader = new ProfileAvatarDownloader( Ah yes, this is much better. This didn't work before because it relied on the BitmapFetcher refactor, but I should've fixed it after it landed. Sorry & done! On 2014/04/28 20:46:37, msw wrote: > Is this object leaked if DownloadHighResAvatar returns early because a download > of the same file is already in progress? Perhaps pass a scoped_ptr, or even > better, have DownloadHighResAvatar construct its own ProfileAvatarDownloader > internally, only when needed. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profiles_state.cc:130: g_browser_process->system_request_context(), On 2014/04/28 20:46:37, msw wrote: > Can't ProfileAvatarDownloader obtain this value internally? > Instead of passing values associated with |g_browser_process| or > TestingBrowserProcess, ProfileInfoCache should have a SetBrowserProcessForTest > helper that will override it's internal use of either value, or similar. Done. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... File chrome/browser/profiles/profiles_state.h (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profiles_state.h:51: // If the file doesn't already exist, starts a download for the high res I'm sure they are disliked, but the avatar index is the only thing that's available (this function needs to be called when changing a profile's avatar, which just gives you the index of the new avatar). It's indexes all the way down :( On 2014/04/28 20:46:37, msw wrote: > This seems out of place here. Aren't icon indices generally disliked? https://codereview.chromium.org/222313005/diff/190001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:275: // If needed, start downloading the high-res avatar. You're right, now that the ProfileInfoCache can create an AvatarDownloader, it doesn't need to be here. Done. On 2014/04/28 20:46:37, msw wrote: > Is this actually needed on browser launch? Is there a better profile/UI > initialization codepath to start this task? I don't think I'd be comfortable > approving this change as-is. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_handler.cc (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_handler.cc:353: if (switches::IsNewAvatarMenu()) { On 2014/04/28 20:46:37, msw wrote: > nit: drop unnecessary braces. Done. https://codereview.chromium.org/222313005/diff/190001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/222313005/diff/190001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:1759: 'browser/profiles/profile_avatar_downloader.cc', Eeeek! Done. On 2014/04/28 20:46:37, msw wrote: > You need to add the header file too...
Looking much better, I really only have two big comments. https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... File chrome/browser/profiles/profiles_state.h (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profiles_state.h:51: // If the file doesn't already exist, starts a download for the high res On 2014/04/29 19:12:27, Monica Dinculescu wrote: > I'm sure they are disliked, but the avatar index is the only thing that's > available (this function needs to be called when changing a profile's avatar, > which just gives you the index of the new avatar). It's indexes all the way down > :( > > On 2014/04/28 20:46:37, msw wrote: > > This seems out of place here. Aren't icon indices generally disliked? > This can be removed; the only caller is ManageProfileHandler::SetProfileIconAndName, which already uses its ProfileInfoCache locally, so it can easily call cache.DownloadHighResAvatar(new_icon_index); directly. Please move the TODO comment inside this function impl (about avoiding re-downloading avatar resources) to the DownloadHighResAvatar declaration. https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_downloader.cc (right): https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.cc:42: gfx::Image* image = new gfx::Image(gfx::Image::CreateFrom1xBitmap(*bitmap)); You allocate a new image here and allocate a copy in SaveAvatarImageAtPath. It looks like this |image| is leaked and should be scrapped, for something similar to gaia_info_update_service.cc:130: gfx::Image image = gfx::Image::CreateFrom1xBitmap(*bitmap); cache_->SaveAvatarImageAtPath(&image, ... https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_downloader.h (right): https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.h:14: ProfileInfoCache* cache); nit: move to the line above. https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.h:15: nit: remove blank line https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:192: for (size_t i = 0; i < GetNumberOfProfiles(); i++) { nit: remove unnecessary braces. https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:403: size_t index) const { nit: this fits on the line above. https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:812: base::Bind(&SaveBitmap, data.release(), image_path, callback)); nit: do base::Passed(&data) and have SaveBitmap take a scoped_ptr<ImageData>.
https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... File chrome/browser/profiles/profiles_state.h (right): https://codereview.chromium.org/222313005/diff/190001/chrome/browser/profiles... chrome/browser/profiles/profiles_state.h:51: // If the file doesn't already exist, starts a download for the high res On 2014/04/29 20:11:44, msw wrote: > On 2014/04/29 19:12:27, Monica Dinculescu wrote: > > I'm sure they are disliked, but the avatar index is the only thing that's > > available (this function needs to be called when changing a profile's avatar, > > which just gives you the index of the new avatar). It's indexes all the way > down > > :( > > > > On 2014/04/28 20:46:37, msw wrote: > > > This seems out of place here. Aren't icon indices generally disliked? > > > > This can be removed; the only caller is > ManageProfileHandler::SetProfileIconAndName, which already uses its > ProfileInfoCache locally, so it can easily call > cache.DownloadHighResAvatar(new_icon_index); directly. Please move the TODO > comment inside this function impl (about avoiding re-downloading avatar > resources) to the DownloadHighResAvatar declaration. Done. https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_downloader.cc (right): https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.cc:42: gfx::Image* image = new gfx::Image(gfx::Image::CreateFrom1xBitmap(*bitmap)); Oh, you're totally right. I don't know why I'm making a copy. On 2014/04/29 20:11:44, msw wrote: > You allocate a new image here and allocate a copy in SaveAvatarImageAtPath. It > looks like this |image| is leaked and should be scrapped, for something similar > to gaia_info_update_service.cc:130: > gfx::Image image = gfx::Image::CreateFrom1xBitmap(*bitmap); > cache_->SaveAvatarImageAtPath(&image, ... https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_downloader.h (right): https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.h:14: ProfileInfoCache* cache); On 2014/04/29 20:11:44, msw wrote: > nit: move to the line above. Done. https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.h:15: On 2014/04/29 20:11:44, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:192: for (size_t i = 0; i < GetNumberOfProfiles(); i++) { On 2014/04/29 20:11:44, msw wrote: > nit: remove unnecessary braces. Done. https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:403: size_t index) const { On 2014/04/29 20:11:44, msw wrote: > nit: this fits on the line above. Done. https://codereview.chromium.org/222313005/diff/230001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:812: base::Bind(&SaveBitmap, data.release(), image_path, callback)); On 2014/04/29 20:11:44, msw wrote: > nit: do base::Passed(&data) and have SaveBitmap take a scoped_ptr<ImageData>. Done.
https://codereview.chromium.org/222313005/diff/240001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_downloader.h (right): https://codereview.chromium.org/222313005/diff/240001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.h:11: class ProfileAvatarDownloader : public chrome::BitmapFetcherDelegate { Is there any plan to consolidate this with ProfileDownloader (which downloads profile picture), or any other mechanism that downloads GAIA/profile images? Have we ruled out shipping the high-res avatar assets with Chrome (perhaps if/when the new profile management feature ships)? Are there other scenarios where we download high-res resources at runtime? Having some better notion of precedent, alternatives, and plans for this feature would make me more comfortable. Sorry for not having asked this earlier. https://codereview.chromium.org/222313005/diff/240001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.h (right): https://codereview.chromium.org/222313005/diff/240001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.h:214: // ProfileAvatarDownloader pointers are deleted when the download completes or nit: s/pointers/instances/ https://codereview.chromium.org/222313005/diff/240001/chrome/browser/profiles... File chrome/browser/profiles/profiles_state.cc (right): https://codereview.chromium.org/222313005/diff/240001/chrome/browser/profiles... chrome/browser/profiles/profiles_state.cc:13: #include "chrome/browser/profiles/profile_avatar_downloader.h" Remove these includes. https://codereview.chromium.org/222313005/diff/240001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (left): https://codereview.chromium.org/222313005/diff/240001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:274: nit: revert this change (to just avoid touching this file). https://codereview.chromium.org/222313005/diff/240001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_handler.cc (right): https://codereview.chromium.org/222313005/diff/240001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_handler.cc:353: if (switches::IsNewAvatarMenu()) Do other paths that set the profile's icon also need to initiate high res resource downloads? For example, ProfileManager::InitProfileUserPrefs and SupervisedUserPrefMappingService::OnSharedSettingChanged. I think this and the two calls below ought to be an internal details of the cache, invoked through a public method like ProfileInfoCache::SetAvatarIconIndexOfProfileAtIndex (like GetAvatarIconIndexOfProfileAtIndex).
Addressing the high level comments first: - We have considered shipping the avatars with Chrome, but this idea was rejected and insta-reverted, because it increased the binary size. Here's a design document that discusses this in more detail: https://docs.google.com/a/google.com/document/d/1KU9gvlXg33rwcvr24NmReXwYRZ1K... - We can't easily merge the avatar download with the Gaia photo download, because they come from different places, and in different ways. The Gaia photo comes from a G+ webservice, which requires an oAuth token, and is only used for signed in users. The avatars are just static resources that need to be used by local profiles.
On 2014/04/30 13:58:32, Monica Dinculescu wrote: > Addressing the high level comments first: > > - We have considered shipping the avatars with Chrome, but this idea was > rejected and insta-reverted, because it increased the binary size. Here's a > design document that discusses this in more detail: > https://docs.google.com/a/google.com/document/d/1KU9gvlXg33rwcvr24NmReXwYRZ1K... Thanks for the document, I have a couple followup questions: 1) RE sizes: Did you tools/resources/optimize-png-files.sh or consider WebP? 2) RE sizes: Is it possible to replace existing icons, reducing sizes impact? 3) RE caching: Why are they saved in a user-data-dir and not the install dir? (we'll need to re-download for --user-data-dir=<override> and similar) > - We can't easily merge the avatar download with the Gaia photo download, > because they come from different places, and in different ways. The Gaia photo > comes from a G+ webservice, which requires an oAuth token, and is only used for > signed in users. The avatars are just static resources that need to be used by > local profiles. Okay, that's reasonable.
> 1) RE sizes: Did you tools/resources/optimize-png-files.sh or consider WebP? I looked at optimized pngs, jpgs, and WebP. WebP was the most promising one, but unfortunately grit only works with pngs, and changing it to use webP looked terrifying. Also, see the point below. > 2) RE sizes: Is it possible to replace existing icons, reducing sizes impact? The existing icons are 38x38 pixels, which means even at 200x they are 100 bytes. We need resources that are at least 180x180 pixels, which are around 16kb. Replacing the existing one will save you maybe 2 whole images, which isn't not very much. We had a conversation last year with laforge@ about this, and the conclusion was that since this affects a minority of users (noone needs ALL the avatars), the size increase would be unacceptable. I will forward you the email thread. > 3) RE caching: Why are they saved in a user-data-dir and not the install dir? > (we'll need to re-download for --user-data-dir=<override> and similar) I don't have a strong argument towards this, other than user-data-dir seemed simpler, and it's where the other state-like things go (so I had code examples). If you feel that it should go in the install_dir, I can try to take a look at that.
https://codereview.chromium.org/222313005/diff/240001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.h (right): https://codereview.chromium.org/222313005/diff/240001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.h:214: // ProfileAvatarDownloader pointers are deleted when the download completes or On 2014/04/29 21:17:33, msw wrote: > nit: s/pointers/instances/ Done. https://codereview.chromium.org/222313005/diff/240001/chrome/browser/profiles... File chrome/browser/profiles/profiles_state.cc (right): https://codereview.chromium.org/222313005/diff/240001/chrome/browser/profiles... chrome/browser/profiles/profiles_state.cc:13: #include "chrome/browser/profiles/profile_avatar_downloader.h" On 2014/04/29 21:17:33, msw wrote: > Remove these includes. Done. https://codereview.chromium.org/222313005/diff/240001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (left): https://codereview.chromium.org/222313005/diff/240001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:274: On 2014/04/29 21:17:33, msw wrote: > nit: revert this change (to just avoid touching this file). Done. https://codereview.chromium.org/222313005/diff/240001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_handler.cc (right): https://codereview.chromium.org/222313005/diff/240001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_handler.cc:353: if (switches::IsNewAvatarMenu()) On 2014/04/29 21:17:33, msw wrote: > Do other paths that set the profile's icon also need to initiate high res > resource downloads? For example, ProfileManager::InitProfileUserPrefs and > SupervisedUserPrefMappingService::OnSharedSettingChanged. > > I think this and the two calls below ought to be an internal details of the > cache, invoked through a public method like > ProfileInfoCache::SetAvatarIconIndexOfProfileAtIndex (like > GetAvatarIconIndexOfProfileAtIndex). Done.
On 2014/04/30 15:58:04, Monica Dinculescu wrote: > > 3) RE caching: Why are they saved in a user-data-dir and not the install dir? > > (we'll need to re-download for --user-data-dir=<override> and similar) > I don't have a strong argument towards this, other than user-data-dir seemed > simpler, > and it's where the other state-like things go (so I had code examples). If you > feel that > it should go in the install_dir, I can try to take a look at that. A given installation running with multiple user-data-dirs over time (or having a user-data-dir be deleted) is likely rare, but <DIR_USER_DATA>/Avatars doesn't seem right. Perhaps try <DIR_RESOURCES>/Avatars which should be per-installation, rather than per-user-data-dir. DIR_RESOURCES is used by extensions for resources; you should seek a knowledgable reviewer for this aspect (Aaron or Finnur, can you advise?). https://codereview.chromium.org/222313005/diff/260001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/222313005/diff/260001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:521: // If needed, start downloading the high-res avatar. Cool, this seems like a better place for this code to live, but is it actually called properly by code paths that change the icon index? I don't see a call to this in ManageProfileHandler::SetProfileIconAndName (where you previously had this code), or in SupervisedUserPrefMappingService::OnSharedSettingChanged (which sets prefs::kProfileAvatarIndex). Perhaps I'm missing something.
https://codereview.chromium.org/222313005/diff/260001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/222313005/diff/260001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:521: // If needed, start downloading the high-res avatar. Oh, I know this one! :) Profiles listen to preferences changes, and setting prefs::kProfileAvatarIndex will call ProfileImpl::UpdateProfileAvatarCache, which calls SetAvatarIconOfProfileAtIndex. On 2014/04/30 17:26:14, msw wrote: > Cool, this seems like a better place for this code to live, but is it actually > called properly by code paths that change the icon index? I don't see a call to > this in ManageProfileHandler::SetProfileIconAndName (where you previously had > this code), or in SupervisedUserPrefMappingService::OnSharedSettingChanged > (which sets prefs::kProfileAvatarIndex). Perhaps I'm missing something.
As discussed offline, DIR_RESOURCES is bad news bears, as it seems to contain the release version in the path, so I'm going to stick to user_data_dir for now. I've also fixed the broken unit tests (which were sad because of the request_context).
LGTM with nits, thanks for addressing all my comments. https://codereview.chromium.org/222313005/diff/280001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_downloader.cc (right): https://codereview.chromium.org/222313005/diff/280001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.cc:32: // In unit tests, there is no g_browser_process, so we do not have a nit: This comment would seem to indicate that g_browser_process is NULL (not a TestingBrowserProcess that returns a NULL URLRequestContextGetter), can you rephrase the comment or handle the NULL URLRequestContextGetter if that's really the case? https://codereview.chromium.org/222313005/diff/280001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache_unittest.cc (right): https://codereview.chromium.org/222313005/diff/280001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache_unittest.cc:545: profiles::GetPathOfHighResAvatarAtIndex(kIconIndex); nit: do you want to test the return value here at all?
Thank you *so much* for the review. I'm way happier with the code now! https://codereview.chromium.org/222313005/diff/280001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_downloader.cc (right): https://codereview.chromium.org/222313005/diff/280001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.cc:32: // In unit tests, there is no g_browser_process, so we do not have a Updated the comment. On 2014/04/30 19:46:21, msw wrote: > nit: This comment would seem to indicate that g_browser_process is NULL (not a > TestingBrowserProcess that returns a NULL URLRequestContextGetter), can you > rephrase the comment or handle the NULL URLRequestContextGetter if that's really > the case? https://codereview.chromium.org/222313005/diff/280001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache_unittest.cc (right): https://codereview.chromium.org/222313005/diff/280001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache_unittest.cc:545: profiles::GetPathOfHighResAvatarAtIndex(kIconIndex); Done. I've checked whether the filename matches. On 2014/04/30 19:46:21, msw wrote: > nit: do you want to test the return value here at all?
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/222313005/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium
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/222313005/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium
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/222313005/320001
Message was sent while issue was closed.
Change committed as 267529 |
