Speed up custom wallpaper switching time and wallpaper manager code refactor

Speed up custom wallpaper loading and wallpaper manager code refactor


BUG=138588, 131203, 139887
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149796

[bshe, 2012-07-26 19:46:54]

Hi Nikita.

Could you please take a look at this CL? 

I found that the most time consuming operation for custom wallpaper is not
loading and encoding wallpaper. It's actually creating a new UserImage
object(around 2/3 of total time) in UserImageLoader from a SkBitmap. However,
for wallpapers, we dont need to use UserImage object. So instead of reuse
UserImageLoader to load wallpaper, I created a WallpaperLoader class to
load/save wallpaper. It reduced the time to switch custom wallpaper to 1s on
desktop.

I also implemented a cache logic in wallpaper_manager. If directly fetching from
cache, the custom wallpaper switching time reduced to 2ms on desktop. 

Some code refactor also included in this CL.

Thanks!

[Nikita (slow), 2012-07-27 22:09:20]

+Ivan for reviewers

Btw, Ivan has a fix in the pipeline that improves custom wallpaper loading on
boot. There's was an extra encode operation happening.

[Nikita (slow), 2012-07-27 22:11:13]

On 2012/07/26 19:46:54, bshe wrote:
> Hi Nikita.
> 
> Could you please take a look at this CL? 
> 
> I found that the most time consuming operation for custom wallpaper is not
> loading and encoding wallpaper. It's actually creating a new UserImage
> object(around 2/3 of total time) in UserImageLoader from a SkBitmap. However,
> for wallpapers, we dont need to use UserImage object. So instead of reuse
> UserImageLoader to load wallpaper, I created a WallpaperLoader class to
> load/save wallpaper. It reduced the time to switch custom wallpaper to 1s on
> desktop.

Right, that's the same issue that Ivan discovered. His fix is in UserImage. I
think both should land, so that UserImage doesn't have that issue going
forwards.

[Ivan Korotkov, 2012-07-27 23:02:02]

Hi Biao,

right, there was a big slowdown with UserImage because it always PNG-encoded the
image, despite the fact that we already have the raw data we just decoded. I'm
preparing a CL with a fix for that and another optimizations of UserImage.
So, I'd ask you to stick to UserImageLoader/UserImage and revert
WallpaperLoader, unless you really need it for other purposes. Otherwise it's
code duplication with no good reason.

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_manager_impl.cc (left):

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:377: void
UserManagerImpl::InitializeWallpaper() {
Is this method still needed? Why can't caller user WallpaperManager directly?

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:100: LoadedCallback loaded_cb
= base::Bind(&WallpaperManager::CacheCallback,
Here and below: there is no point in storing a callback in a special variable.
Most of Chrome's code calls base::Bind right in argument list.

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:114:
UserSelected(UserManager::Get()->GetLoggedInUser().email());
Please don't make manual call of event handlers (like UserSelected). This looks
very confusing. Instead, create an action method like SetCurrentWallpaper (or
what seems appropriate) and call it from OnUserSelected (also please rename it,
becase event handlers should really begin with 'On' or 'Handle').

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:132:
CustomWallpaperMap::iterator it = custom_wallpaper_cache_.find(email);
Here and below: const_iterator

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:136: } else {
Chrome's style is "no else after return"

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:193:
UserSelected(user_manager->GetLoggedInUser().email());
Same note about calling event handler.

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:382: LOG(ERROR) << "Try to
cache a wallpaper for the same user twice.";
No need in braces

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:382: LOG(ERROR) << "Try to
cache a wallpaper for the same user twice.";
"Attemped to cache wallpaper for the same user twice."

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:382: LOG(ERROR) << "Try to
cache a wallpaper for the same user twice.";
If this can happen only due to incorrect programming logic (not due to user
opening chrome:// URLs, etc), this should be a NOTREACHED or DCHECK.

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:401:
CustomWallpaperMap::iterator it =
Just custom_wallpaper_thumbnail_cache_[email] = thumbnail

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:469:
CustomWallpaperMap::iterator it =
ditto

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:486: // TODO(bshe): Refactor
GetWallpaperPathForUser to this class.
Time to do it maybe? :)
Or put it into UserManager, if that's more appropriate. This downcasting to Impl
is really ugly.

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:492: base::Bind(loaded_cb));
Should be no need in empty Bind

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wallpaper_manager.h (right):

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:112: void CacheCallback(const
std::string& email, const SkBitmap& wallpaper);
Please remove 'Callback' sufffix from names. Something like
CacheWallpaper/FetchWallpaper will do.

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:162: // Stores logged in user
wallpaper index.
Please remove word 'stores' from field descriptions, just 'Logged-in user
wallpaper type', etc.

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:165: typedef
std::map<std::string, SkBitmap> CustomWallpaperMap;
typedefs go before methods and fields.

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source.cc
(right):

http://codereview.chromium.org/10832019/diff/4001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source.cc:56:
WallpaperManager::Get()->GetCustomWallpaperThumbnail(user_->email()),
What about caching encoded thumbnails? Actually, do you need to store SkBitmaps
of thumbnails at all, maybe encoded blobs only?

[bshe, 2012-07-31 14:27:54]

Hi Ivan. Could you take another look?

It addresses all your comments except the wallpaper_loader part. Thanks

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
File chrome/browser/chromeos/login/user_manager_impl.cc (left):

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/login/user_manager_impl.cc:377: void
UserManagerImpl::InitializeWallpaper() {
On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> Is this method still needed? Why can't caller user WallpaperManager directly?

Done.

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.cc:100: LoadedCallback loaded_cb
= base::Bind(&WallpaperManager::CacheCallback,
On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> Here and below: there is no point in storing a callback in a special variable.
> Most of Chrome's code calls base::Bind right in argument list.

Done.

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.cc:114:
UserSelected(UserManager::Get()->GetLoggedInUser().email());
On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> Please don't make manual call of event handlers (like UserSelected). This
looks
> very confusing. Instead, create an action method like SetCurrentWallpaper (or
> what seems appropriate) and call it from OnUserSelected (also please rename
it,
> becase event handlers should really begin with 'On' or 'Handle').

Done.

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.cc:132:
CustomWallpaperMap::iterator it = custom_wallpaper_cache_.find(email);
On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> Here and below: const_iterator

Done.

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.cc:136: } else {
On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> Chrome's style is "no else after return"

Done.

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.cc:193:
UserSelected(user_manager->GetLoggedInUser().email());
On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> Same note about calling event handler.

Done.

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.cc:382: LOG(ERROR) << "Try to
cache a wallpaper for the same user twice.";
Done. Using a DCHECK instead.

On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> If this can happen only due to incorrect programming logic (not due to user
> opening chrome:// URLs, etc), this should be a NOTREACHED or DCHECK.

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.cc:401:
CustomWallpaperMap::iterator it =
On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> Just custom_wallpaper_thumbnail_cache_[email] = thumbnail

Done.

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.cc:469:
CustomWallpaperMap::iterator it =
On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.cc:486: // TODO(bshe): Refactor
GetWallpaperPathForUser to this class.
On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> Time to do it maybe? :)
> Or put it into UserManager, if that's more appropriate. This downcasting to
Impl
> is really ugly.

Done.

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.cc:492: base::Bind(loaded_cb));
On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> Should be no need in empty Bind

Done.

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
File chrome/browser/chromeos/login/wallpaper_manager.h (right):

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.h:112: void CacheCallback(const
std::string& email, const SkBitmap& wallpaper);
On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> Please remove 'Callback' sufffix from names. Something like
> CacheWallpaper/FetchWallpaper will do.

Done.

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.h:162: // Stores logged in user
wallpaper index.
On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> Please remove word 'stores' from field descriptions, just 'Logged-in user
> wallpaper type', etc.

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.h:165: typedef
std::map<std::string, SkBitmap> CustomWallpaperMap;
On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> typedefs go before methods and fields.

Done.

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source.cc
(right):

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source.cc:56:
WallpaperManager::Get()->GetCustomWallpaperThumbnail(user_->email()),
Yes. Only blobs is needed. 

Could I do it in a separate Cl though? This CL starts to become large. And since
this code path is only used for encoding a small custom wallpaper
thumbnail(128x80), the performance penalty shouldn't be very bad for now.

On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> What about caching encoded thumbnails? Actually, do you need to store
SkBitmaps
> of thumbnails at all, maybe encoded blobs only?

[bshe, 2012-07-31 19:31:12]

Ivan@

I have removed wallpaper_loader class. Could you please take another look?
Thanks!

On 2012/07/31 14:27:54, bshe wrote:
> Hi Ivan. Could you take another look?
> 
> It addresses all your comments except the wallpaper_loader part. Thanks
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> File chrome/browser/chromeos/login/user_manager_impl.cc (left):
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> chrome/browser/chromeos/login/user_manager_impl.cc:377: void
> UserManagerImpl::InitializeWallpaper() {
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > Is this method still needed? Why can't caller user WallpaperManager
directly?
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> File chrome/browser/chromeos/login/wallpaper_manager.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> chrome/browser/chromeos/login/wallpaper_manager.cc:100: LoadedCallback
loaded_cb
> = base::Bind(&WallpaperManager::CacheCallback,
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > Here and below: there is no point in storing a callback in a special
variable.
> > Most of Chrome's code calls base::Bind right in argument list.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> chrome/browser/chromeos/login/wallpaper_manager.cc:114:
> UserSelected(UserManager::Get()->GetLoggedInUser().email());
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > Please don't make manual call of event handlers (like UserSelected). This
> looks
> > very confusing. Instead, create an action method like SetCurrentWallpaper
(or
> > what seems appropriate) and call it from OnUserSelected (also please rename
> it,
> > becase event handlers should really begin with 'On' or 'Handle').
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> chrome/browser/chromeos/login/wallpaper_manager.cc:132:
> CustomWallpaperMap::iterator it = custom_wallpaper_cache_.find(email);
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > Here and below: const_iterator
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> chrome/browser/chromeos/login/wallpaper_manager.cc:136: } else {
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > Chrome's style is "no else after return"
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> chrome/browser/chromeos/login/wallpaper_manager.cc:193:
> UserSelected(user_manager->GetLoggedInUser().email());
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > Same note about calling event handler.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> chrome/browser/chromeos/login/wallpaper_manager.cc:382: LOG(ERROR) << "Try to
> cache a wallpaper for the same user twice.";
> Done. Using a DCHECK instead.
> 
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > If this can happen only due to incorrect programming logic (not due to user
> > opening chrome:// URLs, etc), this should be a NOTREACHED or DCHECK.
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> chrome/browser/chromeos/login/wallpaper_manager.cc:401:
> CustomWallpaperMap::iterator it =
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > Just custom_wallpaper_thumbnail_cache_[email] = thumbnail
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> chrome/browser/chromeos/login/wallpaper_manager.cc:469:
> CustomWallpaperMap::iterator it =
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > ditto
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> chrome/browser/chromeos/login/wallpaper_manager.cc:486: // TODO(bshe):
Refactor
> GetWallpaperPathForUser to this class.
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > Time to do it maybe? :)
> > Or put it into UserManager, if that's more appropriate. This downcasting to
> Impl
> > is really ugly.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> chrome/browser/chromeos/login/wallpaper_manager.cc:492:
base::Bind(loaded_cb));
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > Should be no need in empty Bind
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> File chrome/browser/chromeos/login/wallpaper_manager.h (right):
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> chrome/browser/chromeos/login/wallpaper_manager.h:112: void
CacheCallback(const
> std::string& email, const SkBitmap& wallpaper);
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > Please remove 'Callback' sufffix from names. Something like
> > CacheWallpaper/FetchWallpaper will do.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> chrome/browser/chromeos/login/wallpaper_manager.h:162: // Stores logged in
user
> wallpaper index.
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > Please remove word 'stores' from field descriptions, just 'Logged-in user
> > wallpaper type', etc.
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/chro...
> chrome/browser/chromeos/login/wallpaper_manager.h:165: typedef
> std::map<std::string, SkBitmap> CustomWallpaperMap;
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > typedefs go before methods and fields.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/ui/w...
> File chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/ui/w...
> chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source.cc:56:
> WallpaperManager::Get()->GetCustomWallpaperThumbnail(user_->email()),
> Yes. Only blobs is needed. 
> 
> Could I do it in a separate Cl though? This CL starts to become large. And
since
> this code path is only used for encoding a small custom wallpaper
> thumbnail(128x80), the performance penalty shouldn't be very bad for now.
> 
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > What about caching encoded thumbnails? Actually, do you need to store
> SkBitmaps
> > of thumbnails at all, maybe encoded blobs only?

[Ivan Korotkov, 2012-07-31 21:18:15]

Thanks for fixes, LGTM.
Please check this CL against the tip of trunk since my CL has just landed.

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source.cc
(right):

https://chromiumcodereview.appspot.com/10832019/diff/4001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source.cc:56:
WallpaperManager::Get()->GetCustomWallpaperThumbnail(user_->email()),
On 2012/07/31 14:27:54, bshe wrote:
> Yes. Only blobs is needed. 
> 
> Could I do it in a separate Cl though? This CL starts to become large. And
since
> this code path is only used for encoding a small custom wallpaper
> thumbnail(128x80), the performance penalty shouldn't be very bad for now.
> 
> On 2012/07/27 23:02:02, Ivan Korotkov wrote:
> > What about caching encoded thumbnails? Actually, do you need to store
> SkBitmaps
> > of thumbnails at all, maybe encoded blobs only?
> 

Yes, sure.

https://chromiumcodereview.appspot.com/10832019/diff/8003/chrome/browser/chro...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://chromiumcodereview.appspot.com/10832019/diff/8003/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.cc:163: User::WallpaperType*
type,
It looks like there're many functions taking these 3 arguments --
type/index/date. Please consider combining them in a separate struct/class in
future CLs.

https://chromiumcodereview.appspot.com/10832019/diff/8003/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler.cc
(right):

https://chromiumcodereview.appspot.com/10832019/diff/8003/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler.cc:165:
WallpaperManager::Get()->GetLoggedInUserWallpaperProperties(&type, &index,
Maybe move all arguments to the next line?

https://chromiumcodereview.appspot.com/10832019/diff/8003/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler.cc:241:
&date);
Ditto

[bshe, 2012-08-01 15:07:35]

done.

+jhawkins for owners:

chrome/browser/ui/webui/*

Thanks

http://codereview.chromium.org/10832019/diff/8003/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

http://codereview.chromium.org/10832019/diff/8003/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:163: User::WallpaperType*
type,
Will do.

On 2012/07/31 21:18:16, Ivan Korotkov wrote:
> It looks like there're many functions taking these 3 arguments --
> type/index/date. Please consider combining them in a separate struct/class in
> future CLs.

http://codereview.chromium.org/10832019/diff/8003/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler.cc
(right):

http://codereview.chromium.org/10832019/diff/8003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler.cc:165:
WallpaperManager::Get()->GetLoggedInUserWallpaperProperties(&type, &index,
On 2012/07/31 21:18:16, Ivan Korotkov wrote:
> Maybe move all arguments to the next line?

Done.

http://codereview.chromium.org/10832019/diff/8003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler.cc:241:
&date);
On 2012/07/31 21:18:16, Ivan Korotkov wrote:
> Ditto

Done.

[bshe, 2012-08-02 18:01:32]

friendly ping?


On 2012/08/01 15:07:35, bshe wrote:
> done.
> 
> +jhawkins for owners:
> 
> chrome/browser/ui/webui/*
> 
> Thanks
> 
>
http://codereview.chromium.org/10832019/diff/8003/chrome/browser/chromeos/log...
> File chrome/browser/chromeos/login/wallpaper_manager.cc (right):
> 
>
http://codereview.chromium.org/10832019/diff/8003/chrome/browser/chromeos/log...
> chrome/browser/chromeos/login/wallpaper_manager.cc:163: User::WallpaperType*
> type,
> Will do.
> 
> On 2012/07/31 21:18:16, Ivan Korotkov wrote:
> > It looks like there're many functions taking these 3 arguments --
> > type/index/date. Please consider combining them in a separate struct/class
in
> > future CLs.
> 
>
http://codereview.chromium.org/10832019/diff/8003/chrome/browser/ui/webui/opt...
> File
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler.cc
> (right):
> 
>
http://codereview.chromium.org/10832019/diff/8003/chrome/browser/ui/webui/opt...
>
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler.cc:165:
> WallpaperManager::Get()->GetLoggedInUserWallpaperProperties(&type, &index,
> On 2012/07/31 21:18:16, Ivan Korotkov wrote:
> > Maybe move all arguments to the next line?
> 
> Done.
> 
>
http://codereview.chromium.org/10832019/diff/8003/chrome/browser/ui/webui/opt...
>
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler.cc:241:
> &date);
> On 2012/07/31 21:18:16, Ivan Korotkov wrote:
> > Ditto
> 
> Done.

[James Hawkins, 2012-08-02 18:30:50]

LGTM

[Nikita (slow), 2012-08-02 20:40:26]

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:85: last_selected_user_(""),
not needed

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:159: std::string filename =
username +
nit: use StringPrintf and 2 format strings?

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:257: if
(UserManager::Get()->IsCurrentUserEphemeral()) {
nit: drop {}

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:292: // TODO(bshe): Ideally,
wallpaper should start to load as soon as user click
Could you please clarify this comment?
Is this about a timeout we have?

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:342: if
(UserManager::Get()->IsKnownUser(email)) {
Simplify with

if (!UserManager::Get()->IsKnownUser(email))
  return;

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:464:
skia::ImageOperations::Resize(wallpaper,
Doesn't seem like FILE thread is a right place for something like this i.e.
doing image resizing.

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:464:
skia::ImageOperations::Resize(wallpaper,
Why thumbnail for custom wallpaper is not loaded from file anyway?

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:526:
skia::ImageOperations::Resize(wallpaper,
Doesn't seem like a right thread to do a resize.

[bshe, 2012-08-02 21:59:28]

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:85: last_selected_user_(""),
On 2012/08/02 20:40:26, Nikita Kostylev wrote:
> not needed

Done.

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:159: std::string filename =
username +
On 2012/08/02 20:40:26, Nikita Kostylev wrote:
> nit: use StringPrintf and 2 format strings?

Used StringPrintf. But I am not sure if I am doing it the way you are thinking.
This looks less intuitive..

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:257: if
(UserManager::Get()->IsCurrentUserEphemeral()) {
On 2012/08/02 20:40:26, Nikita Kostylev wrote:
> nit: drop {}

Done.

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:292: // TODO(bshe): Ideally,
wallpaper should start to load as soon as user click
It is not about the timeout we have.

When a new user want to login through the "Add user" button in the login screen,
I was proposing that we should start to load a default wallpaper as soon as user
click that button (instead of after that user login). But I am not sure if this
is case now since I added it a long time ago. I will remove it if it is not the
case.

On 2012/08/02 20:40:26, Nikita Kostylev wrote:
> Could you please clarify this comment?
> Is this about a timeout we have?

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:342: if
(UserManager::Get()->IsKnownUser(email)) {
On 2012/08/02 20:40:26, Nikita Kostylev wrote:
> Simplify with
> 
> if (!UserManager::Get()->IsKnownUser(email))
>   return;

Done.

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:464:
skia::ImageOperations::Resize(wallpaper,
Since we always have the large size wallpaper loaded, I think that it maybe ok
to just regenerate thumbnail every time a custom wallpaper loaded. And the
resize operation is not intense/heavy(less than 8ms). So it maybe also OK to
execute it on FILE thread? if not, what thread do you suggest that I should move
to?

Compare to save it to file and loaded it from file when using it, this might be
slightly more efficient.

On 2012/08/02 20:40:26, Nikita Kostylev wrote:
> Why thumbnail for custom wallpaper is not loaded from file anyway?

[Nikita (slow), 2012-08-02 22:52:56]

lgtm

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:292: // TODO(bshe): Ideally,
wallpaper should start to load as soon as user click
On 2012/08/02 21:59:28, bshe wrote:
> It is not about the timeout we have.
> 
> When a new user want to login through the "Add user" button in the login
screen,
> I was proposing that we should start to load a default wallpaper as soon as
user
> click that button (instead of after that user login). But I am not sure if
this
> is case now since I added it a long time ago. I will remove it if it is not
the
> case.
> 
> On 2012/08/02 20:40:26, Nikita Kostylev wrote:
> > Could you please clarify this comment?
> > Is this about a timeout we have?
> 

Yes, when we click AddUser we need to load default wallpaper. It is an updated
spec. No need to display white (solid color) background any more.

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:464:
skia::ImageOperations::Resize(wallpaper,
On 2012/08/02 21:59:28, bshe wrote:
> Since we always have the large size wallpaper loaded, I think that it maybe ok
> to just regenerate thumbnail every time a custom wallpaper loaded. And the
> resize operation is not intense/heavy(less than 8ms). So it maybe also OK to
> execute it on FILE thread? if not, what thread do you suggest that I should
move
> to?
> 
> Compare to save it to file and loaded it from file when using it, this might
be
> slightly more efficient.
> 
> On 2012/08/02 20:40:26, Nikita Kostylev wrote:
> > Why thumbnail for custom wallpaper is not loaded from file anyway?
> 

Usually they should be done on WorkerPool (intensive) or on UI thread (simple
ones).

http://code.google.com/codesearch#OAMlx_jo-ck/src/ash/launcher/launcher_butto...
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/profiles/pro...

http://code.google.com/codesearch#OAMlx_jo-ck/src/ui/app_list/app_list_item_v...

https://chromiumcodereview.appspot.com/10832019/diff/4033/chrome/browser/chro...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://chromiumcodereview.appspot.com/10832019/diff/4033/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.cc:160: std::string filename =
base::StringPrintf("%s%s%s%s", username.c_str(),
Format should be something like
"%s_wallpaper%s.png"

https://chromiumcodereview.appspot.com/10832019/diff/4033/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.cc:198: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType))
You don't need this here as it will be done on higher level:
http://codereview.chromium.org/10834081/diff/1043/chrome/browser/chromeos/chr...

[bshe, 2012-08-03 00:23:35]

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://chromiumcodereview.appspot.com/10832019/diff/12012/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:292: // TODO(bshe): Ideally,
wallpaper should start to load as soon as user click
On 2012/08/02 22:52:57, Nikita Kostylev wrote:
> On 2012/08/02 21:59:28, bshe wrote:
> > It is not about the timeout we have.
> > 
> > When a new user want to login through the "Add user" button in the login
> screen,
> > I was proposing that we should start to load a default wallpaper as soon as
> user
> > click that button (instead of after that user login). But I am not sure if
> this
> > is case now since I added it a long time ago. I will remove it if it is not
> the
> > case.
> > 
> > On 2012/08/02 20:40:26, Nikita Kostylev wrote:
> > > Could you please clarify this comment?
> > > Is this about a timeout we have?
> > 
> 
> Yes, when we click AddUser we need to load default wallpaper. It is an updated
> spec. No need to display white (solid color) background any more.

Done.

https://chromiumcodereview.appspot.com/10832019/diff/4033/chrome/browser/chro...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://chromiumcodereview.appspot.com/10832019/diff/4033/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.cc:160: std::string filename =
base::StringPrintf("%s%s%s%s", username.c_str(),
On 2012/08/02 22:52:57, Nikita Kostylev wrote:
> Format should be something like
> "%s_wallpaper%s.png"

Done.

https://chromiumcodereview.appspot.com/10832019/diff/4033/chrome/browser/chro...
chrome/browser/chromeos/login/wallpaper_manager.cc:198: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType))
I tried patch your cl and test it. Somehow the zero delay is not setted in
ExistingUserControllerTest.*. Maybe I did something wrong. Will verify it again.


On 2012/08/02 22:52:57, Nikita Kostylev wrote:
> You don't need this here as it will be done on higher level:
>
http://codereview.chromium.org/10834081/diff/1043/chrome/browser/chromeos/chr...

[commit-bot: I haz the power, 2012-08-03 01:22:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/10832019/16020

[commit-bot: I haz the power, 2012-08-03 02:27:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/10832019/15044

[commit-bot: I haz the power, 2012-08-03 07:18:35]

Change committed as 149796