Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10827154:
Preload default wallpaper. (Closed)
|
Created:
8 years, 4 months ago by bshe Modified:
8 years, 4 months ago CC:
chromium-reviews, sadrul, nkostylev+watch_chromium.org, ben+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPreload default wallpaper.
BUG=139929
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150726
Patch Set 1 #
Total comments: 3
Patch Set 2 : Nikita's reivew #
Total comments: 10
Patch Set 3 : Address nikita's comment 8 #Patch Set 4 : Delay start cache wallpapers after wallpaper animation finished #Patch Set 5 : Merge to trunk #Patch Set 6 : Compile error fix #
Total comments: 4
Patch Set 7 : Use DCHECK replace if cause. #
Messages
Total messages: 22 (0 generated)
Hi Nikita. Could you take a look at this CL? I rely on the resouces bundle's cache mechanism to preload the default wallpaper. So I dont need to duplicate the data in memory. Thanks, Biao
looks good overall, is there a visible speed improvement seen?
http://codereview.chromium.org/10827154/diff/1/chrome/browser/chromeos/login/... File chrome/browser/chromeos/login/user_manager_impl.cc (right): http://codereview.chromium.org/10827154/diff/1/chrome/browser/chromeos/login/... chrome/browser/chromeos/login/user_manager_impl.cc:735: WallpaperManager::Get()->CacheUserWallpaper(email); I've just realized that this place doesn't seem like a correct for this call. EnsureUsersLoaded might be called multiple times per the existence of of UserManager. 1. Cached should be executed only while at the login screen. 2. It should be executed exactly once.
http://codereview.chromium.org/10827154/diff/1/chrome/browser/chromeos/login/... File chrome/browser/chromeos/login/user_manager_impl.cc (right): http://codereview.chromium.org/10827154/diff/1/chrome/browser/chromeos/login/... chrome/browser/chromeos/login/user_manager_impl.cc:735: WallpaperManager::Get()->CacheUserWallpaper(email); I see. Where do you think is the best place to start the caching operations? It will be heavy io operations if we have lots of user logged in. So we probably want to only start these operations when IO is not busy. On 2012/08/03 18:09:39, Nikita Kostylev wrote: > I've just realized that this place doesn't seem like a correct for this call. > > EnsureUsersLoaded might be called multiple times per the existence of of > UserManager. > > 1. Cached should be executed only while at the login screen. > 2. It should be executed exactly once.
http://codereview.chromium.org/10827154/diff/1/chrome/browser/chromeos/login/... File chrome/browser/chromeos/login/user_manager_impl.cc (right): http://codereview.chromium.org/10827154/diff/1/chrome/browser/chromeos/login/... chrome/browser/chromeos/login/user_manager_impl.cc:735: WallpaperManager::Get()->CacheUserWallpaper(email); On 2012/08/03 18:14:54, bshe wrote: > I see. Where do you think is the best place to start the caching operations? It > will be heavy io operations if we have lots of user logged in. So we probably > want to only start these operations when IO is not busy. > > On 2012/08/03 18:09:39, Nikita Kostylev wrote: > > I've just realized that this place doesn't seem like a correct for this call. > > > > EnsureUsersLoaded might be called multiple times per the existence of of > > UserManager. > > > > 1. Cached should be executed only while at the login screen. > > 2. It should be executed exactly once. > Correct. Now we have 2 ways how first user wallpaper might be loaded: 1. Loaded on boot, login WebUI waits for it. loadWallpaper is not called on first user selection as it is already done. Caching operation might be executed as a delayed task after it receives NOTIFICATION_LOGIN_WEBUI_VISIBLE notification. See http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/chrome_notifi... 2. Flag --disable-boot-animation is passed (might be the case on mario/alex/zgb when I decide to add that flag). Login WebUI is loaded right away. First user gets preselected, in 500ms after that loadWallpaper is called. In this case caching operation should be scheduled as a delayed one once it completes.
On 2012/08/03 18:21:31, Nikita Kostylev wrote: > http://codereview.chromium.org/10827154/diff/1/chrome/browser/chromeos/login/... > File chrome/browser/chromeos/login/user_manager_impl.cc (right): > > http://codereview.chromium.org/10827154/diff/1/chrome/browser/chromeos/login/... > chrome/browser/chromeos/login/user_manager_impl.cc:735: > WallpaperManager::Get()->CacheUserWallpaper(email); > On 2012/08/03 18:14:54, bshe wrote: > > I see. Where do you think is the best place to start the caching operations? > It > > will be heavy io operations if we have lots of user logged in. So we probably > > want to only start these operations when IO is not busy. > > > > On 2012/08/03 18:09:39, Nikita Kostylev wrote: > > > I've just realized that this place doesn't seem like a correct for this > call. > > > > > > EnsureUsersLoaded might be called multiple times per the existence of of > > > UserManager. > > > > > > 1. Cached should be executed only while at the login screen. > > > 2. It should be executed exactly once. > > > > Correct. Now we have 2 ways how first user wallpaper might be loaded: > > 1. Loaded on boot, login WebUI waits for it. loadWallpaper is not called on > first user selection as it is already done. > > Caching operation might be executed as a delayed task after it receives > NOTIFICATION_LOGIN_WEBUI_VISIBLE notification. See > http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/chrome_notifi... > > 2. Flag --disable-boot-animation is passed (might be the case on mario/alex/zgb > when I decide to add that flag). > > Login WebUI is loaded right away. > First user gets preselected, in 500ms after that loadWallpaper is called. > > In this case caching operation should be scheduled as a delayed one once it > completes. Could you take another look? I will add comments if you think this is the right way to go. Basically, for case 1, I post a delayed task (500ms) to cache all wallpapers (except the wallpaper of first user in the user list, it should have loaded). For case 2, I start the cache in the OnUserSelected.
On 2012/08/03 22:21:36, bshe wrote: > On 2012/08/03 18:21:31, Nikita Kostylev wrote: > > > http://codereview.chromium.org/10827154/diff/1/chrome/browser/chromeos/login/... > > File chrome/browser/chromeos/login/user_manager_impl.cc (right): > > > > > http://codereview.chromium.org/10827154/diff/1/chrome/browser/chromeos/login/... > > chrome/browser/chromeos/login/user_manager_impl.cc:735: > > WallpaperManager::Get()->CacheUserWallpaper(email); > > On 2012/08/03 18:14:54, bshe wrote: > > > I see. Where do you think is the best place to start the caching operations? > > It > > > will be heavy io operations if we have lots of user logged in. So we > probably > > > want to only start these operations when IO is not busy. > > > > > > On 2012/08/03 18:09:39, Nikita Kostylev wrote: > > > > I've just realized that this place doesn't seem like a correct for this > > call. > > > > > > > > EnsureUsersLoaded might be called multiple times per the existence of of > > > > UserManager. > > > > > > > > 1. Cached should be executed only while at the login screen. > > > > 2. It should be executed exactly once. > > > > > > > Correct. Now we have 2 ways how first user wallpaper might be loaded: > > > > 1. Loaded on boot, login WebUI waits for it. loadWallpaper is not called on > > first user selection as it is already done. > > > > Caching operation might be executed as a delayed task after it receives > > NOTIFICATION_LOGIN_WEBUI_VISIBLE notification. See > > > http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/chrome_notifi... > > > > 2. Flag --disable-boot-animation is passed (might be the case on > mario/alex/zgb > > when I decide to add that flag). > > > > Login WebUI is loaded right away. > > First user gets preselected, in 500ms after that loadWallpaper is called. > > > > In this case caching operation should be scheduled as a delayed one once it > > completes. > > Could you take another look? I will add comments if you think this is the right > way to go. Basically, for case 1, I post a delayed task (500ms) to cache all > wallpapers (except the wallpaper of first user in the user list, it should have > loaded). For case 2, I start the cache in the OnUserSelected. But that does mean that for 2nd case caching will be competing with first user wallpaper being loaded.
http://codereview.chromium.org/10827154/diff/7/ash/desktop_background/desktop... File ash/desktop_background/desktop_background_controller.cc (right): http://codereview.chromium.org/10827154/diff/7/ash/desktop_background/desktop... ash/desktop_background/desktop_background_controller.cc:138: if (index == ash::GetInvalidWallpaperIndex() || just check for index < 0 ? http://codereview.chromium.org/10827154/diff/7/ash/desktop_background/desktop... File ash/desktop_background/desktop_background_controller.h (right): http://codereview.chromium.org/10827154/diff/7/ash/desktop_background/desktop... ash/desktop_background/desktop_background_controller.h:76: void TriggerCacheDefaultWallpaper(int index); nit: just CacheDefaultWallpaper ? http://codereview.chromium.org/10827154/diff/7/chrome/browser/chromeos/login/... File chrome/browser/chromeos/login/user_manager_impl.cc (right): http://codereview.chromium.org/10827154/diff/7/chrome/browser/chromeos/login/... chrome/browser/chromeos/login/user_manager_impl.cc:735: WallpaperManager::Get()->CacheUserWallpaper(email); As discussed, this call should be removed from here. http://codereview.chromium.org/10827154/diff/7/chrome/browser/chromeos/login/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): http://codereview.chromium.org/10827154/diff/7/chrome/browser/chromeos/login/... chrome/browser/chromeos/login/wallpaper_manager.cc:402: base::Bind(&WallpaperManager::CacheAllUsersWallpapers, We should no cache all users wallpapers when single user is selected. Precaching should be single time operation. http://codereview.chromium.org/10827154/diff/7/chrome/browser/chromeos/login/... chrome/browser/chromeos/login/wallpaper_manager.cc:458: PrefService* local_state = g_browser_process->local_state(); Use user_manager->GetUsers() instead.
nikita@ This patch addressed your comment 8 and I am working on your comment 7. Thanks! https://chromiumcodereview.appspot.com/10827154/diff/7/ash/desktop_background... File ash/desktop_background/desktop_background_controller.cc (right): https://chromiumcodereview.appspot.com/10827154/diff/7/ash/desktop_background... ash/desktop_background/desktop_background_controller.cc:138: if (index == ash::GetInvalidWallpaperIndex() || I was trying to avoid magic number like 0. But i am fine either way. So using < 0. On 2012/08/06 16:36:01, Nikita Kostylev wrote: > just check for index < 0 ? https://chromiumcodereview.appspot.com/10827154/diff/7/ash/desktop_background... File ash/desktop_background/desktop_background_controller.h (right): https://chromiumcodereview.appspot.com/10827154/diff/7/ash/desktop_background... ash/desktop_background/desktop_background_controller.h:76: void TriggerCacheDefaultWallpaper(int index); On 2012/08/06 16:36:01, Nikita Kostylev wrote: > nit: just CacheDefaultWallpaper ? Done. https://chromiumcodereview.appspot.com/10827154/diff/7/chrome/browser/chromeo... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://chromiumcodereview.appspot.com/10827154/diff/7/chrome/browser/chromeo... chrome/browser/chromeos/login/user_manager_impl.cc:735: WallpaperManager::Get()->CacheUserWallpaper(email); Ahh. Must be a merge issue. Sorry about it. Deleted it. On 2012/08/06 16:36:01, Nikita Kostylev wrote: > As discussed, this call should be removed from here. https://chromiumcodereview.appspot.com/10827154/diff/7/chrome/browser/chromeo... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://chromiumcodereview.appspot.com/10827154/diff/7/chrome/browser/chromeo... chrome/browser/chromeos/login/wallpaper_manager.cc:402: base::Bind(&WallpaperManager::CacheAllUsersWallpapers, It should be a single time operation. The only time |should_cache_wallpaper_| is true is when we receive a NOTIFICATION_LOGIN_WEBUI_VISIBLE notification and DisableBootAnimation is false. The LOGIN_WEBUI_VISIBLE should only fired once on each login. On 2012/08/06 16:36:01, Nikita Kostylev wrote: > We should no cache all users wallpapers when single user is selected. > Precaching should be single time operation. https://chromiumcodereview.appspot.com/10827154/diff/7/chrome/browser/chromeo... chrome/browser/chromeos/login/wallpaper_manager.cc:458: PrefService* local_state = g_browser_process->local_state(); On 2012/08/06 16:36:01, Nikita Kostylev wrote: > Use user_manager->GetUsers() instead. Done.
On 2012/08/04 03:38:17, Nikita Kostylev wrote: > On 2012/08/03 22:21:36, bshe wrote: > > On 2012/08/03 18:21:31, Nikita Kostylev wrote: > > > > > > http://codereview.chromium.org/10827154/diff/1/chrome/browser/chromeos/login/... > > > File chrome/browser/chromeos/login/user_manager_impl.cc (right): > > > > > > > > > http://codereview.chromium.org/10827154/diff/1/chrome/browser/chromeos/login/... > > > chrome/browser/chromeos/login/user_manager_impl.cc:735: > > > WallpaperManager::Get()->CacheUserWallpaper(email); > > > On 2012/08/03 18:14:54, bshe wrote: > > > > I see. Where do you think is the best place to start the caching > operations? > > > It > > > > will be heavy io operations if we have lots of user logged in. So we > > probably > > > > want to only start these operations when IO is not busy. > > > > > > > > On 2012/08/03 18:09:39, Nikita Kostylev wrote: > > > > > I've just realized that this place doesn't seem like a correct for this > > > call. > > > > > > > > > > EnsureUsersLoaded might be called multiple times per the existence of of > > > > > UserManager. > > > > > > > > > > 1. Cached should be executed only while at the login screen. > > > > > 2. It should be executed exactly once. > > > > > > > > > > Correct. Now we have 2 ways how first user wallpaper might be loaded: > > > > > > 1. Loaded on boot, login WebUI waits for it. loadWallpaper is not called on > > > first user selection as it is already done. > > > > > > Caching operation might be executed as a delayed task after it receives > > > NOTIFICATION_LOGIN_WEBUI_VISIBLE notification. See > > > > > > http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/chrome_notifi... > > > > > > 2. Flag --disable-boot-animation is passed (might be the case on > > mario/alex/zgb > > > when I decide to add that flag). > > > > > > Login WebUI is loaded right away. > > > First user gets preselected, in 500ms after that loadWallpaper is called. > > > > > > In this case caching operation should be scheduled as a delayed one once it > > > completes. > > > > Could you take another look? I will add comments if you think this is the > right > > way to go. Basically, for case 1, I post a delayed task (500ms) to cache all > > wallpapers (except the wallpaper of first user in the user list, it should > have > > loaded). For case 2, I start the cache in the OnUserSelected. > > But that does mean that for 2nd case caching will be competing with first user > wallpaper being loaded. nikita@ patchset 4 for this comment. I delayed the cache after first wallpaper animation finished. Could you please take a look? Thanks!
lgtm
+sky for OWNERS: ash/* Thanks!
http://codereview.chromium.org/10827154/diff/12005/ash/desktop_background/des... File ash/desktop_background/desktop_background_controller.cc (right): http://codereview.chromium.org/10827154/diff/12005/ash/desktop_background/des... ash/desktop_background/desktop_background_controller.cc:154: if (index < 0) Should this be a DCHECK? http://codereview.chromium.org/10827154/diff/12005/ash/desktop_background/des... ash/desktop_background/desktop_background_controller.cc:364: Shell::RootWindowList root_windows = Shell::GetAllRootWindows(); None of the ui code is thread safe.
sky@ http://codereview.chromium.org/10827154/diff/12005/ash/desktop_background/des... File ash/desktop_background/desktop_background_controller.cc (right): http://codereview.chromium.org/10827154/diff/12005/ash/desktop_background/des... ash/desktop_background/desktop_background_controller.cc:154: if (index < 0) Done On 2012/08/07 20:44:18, sky wrote: > Should this be a DCHECK? http://codereview.chromium.org/10827154/diff/12005/ash/desktop_background/des... ash/desktop_background/desktop_background_controller.cc:364: Shell::RootWindowList root_windows = Shell::GetAllRootWindows(); Sorry. do you mean the code that I add is not thread safe or do you mean the function I use in my code is not thread safe? All the code which I add should only be executed UI thread. Not sure if I understand your comment correctly. Could you give more details about this comment? Thanks! On 2012/08/07 20:44:18, sky wrote: > None of the ui code is thread safe.
Sorry, I was reading the code wrong.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bshe@chromium.org/10827154/15010
Try job failure for 10827154-15010 (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bshe@chromium.org/10827154/15010
Try job failure for 10827154-15010 (retry) (retry) (retry) on linux_rel for step "nacl_integration". It's a second try, previously, step "nacl_integration" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bshe@chromium.org/10827154/15010
Change committed as 150726 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
