Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
215293003:
Move all wallpaper file loading and decoding from DesktopBackgroundController to WallpaperManager. (Closed)
|
Created:
6 years, 8 months ago by Alexander Alekseev Modified:
6 years, 8 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, sadrul, nkostylev+watch_chromium.org, ben+ash_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, kalyank, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, Dmitry Polukhin, Nikita (slow) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMove all wallpaper file loading and decoding from DesktopBackgroundController to WallpaperManager.
To simplify DesktopBackgroundController and allow customized default wallpapers,
we need to move all "default" wallpaper loading to WallpaperManager.
BUG=348136
TEST=unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262862
Patch Set 1 #Patch Set 2 : Fix windows build. #
Total comments: 25
Patch Set 3 : WallpaperManager should cache only one default wallpaper. #
Total comments: 49
Patch Set 4 : Rebase. #Patch Set 5 : Rebase. #Patch Set 6 : Update after review. #
Total comments: 44
Patch Set 7 : Implement DBC::TestAPI. #Patch Set 8 : Call WM::SetDefaultWallpaper() from ShellBrowserMainParts::PreMainMessageLoopRun(). #
Total comments: 3
Patch Set 9 : Fix windowd build. #Patch Set 10 : Fix windows build. #
Total comments: 2
Patch Set 11 : Do not reuse RunLoop object. #
Total comments: 1
Patch Set 12 : Do not set default wallpaper in ShellBrowserMainParts::PreMainMessageLoopRun(). #Patch Set 13 : Style. #Patch Set 14 : Fix clang debug build. #Messages
Total messages: 44 (0 generated)
derat@, bshe@: Please review. This is DBC + WM part of https://codereview.chromium.org/208273005/ .
thanks for splitting the other change https://codereview.chromium.org/215293003/diff/20001/ash/desktop_background/d... File ash/desktop_background/desktop_background_controller.h (right): https://codereview.chromium.org/215293003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.h:108: WallpaperResolution GetAppropriateResolution(); does this only get used by c/b/chromeos/login now? if so, please move it (and the k{Small,Large}WallpaperMax{Width,Height} and thumbnail constants and the WallpaperResolution enum) to c/b/chromeos/login https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:32: #include "chrome/browser/chromeos/customization_document.h" did you mean to add this include in the next change instead? https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:798: if (guest_default_small_wallpaper_image_.get() == NULL) { nit: just do "if (!guest_default_small_wallpaper_image_) {" (and same for other calls in this method) https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:806: if (guest_default_large_wallpaper_image_.get() == NULL) { there's a lot of duplicated code here. how about doing something like this instead? base::FilePath path; scoped_ptr<gfx::ImageSkia>* image; if (is_guest) { if (use_small) { path = guest_small_wallpaper_file_; image = &guest_small_wallpaper_image_; } else { // etc. } if (*image) { StartLoad... return; } ash::Shell...SetWallpaper(image, ...); https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1167: CommandLine* WallpaperManager::GetComandLine() { s/Comand/Command/ (please fix this typo elsewhere in the file, too) https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:231: // Result is true on success. nit: change to "Returns true on success." https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:472: void SetDefaultWallpaperPathFromCommandLine(base::CommandLine* command_line); nit: s/Path/Paths/ https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:545: base::FilePath guest_default_small_wallpaper_file_; mind renaming these to "guest_small_wallpaper_file_" and "guest_large_wallpaper_file_" so people will be less likely to confuse them with the non-guest defaults? https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:549: scoped_ptr<gfx::ImageSkia> default_small_wallpaper_image_; i'm pretty sure we don't want to hold all of these in memory. i believe that DesktopBackgroundController only stored the currently-in-use image.
Thanks for splitting the CL https://codereview.chromium.org/215293003/diff/20001/ash/desktop_background/d... File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/215293003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.cc:110: if (WallpaperIsAlreadyLoaded(image)) { should we check if layout is the same in the case? Otherwise, changing layout probably not gonna work. https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:762: MovableOnDestroyCallbackHolder on_finish) { I dont see anywhere using wallpaper that packed in Chrome resource (IDR_AURA_WALLPAPER_DEFAULT_LARGE) as fallback. I suspect the chromium build wont have a wallpaper in this case and some tests may broken. https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:478: const UserImage& wallpaper); nit: parameters that output result should appears after input. consider move |wallpaper| above |result|. https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:549: scoped_ptr<gfx::ImageSkia> default_small_wallpaper_image_; +1 On 2014/03/31 22:25:46, Daniel Erat wrote: > i'm pretty sure we don't want to hold all of these in memory. i believe that > DesktopBackgroundController only stored the currently-in-use image.
= // Decoded default images are stored here. = scoped_ptr<gfx::ImageSkia> default_small_wallpaper_image_; Daniel Erat 2014/03/31 22:25:46 > i'm pretty sure we don't want to hold all of these in > memory. i believe that > DesktopBackgroundController only stored the > currently-in-use image. This actually means that we need sandboxed image loader to test WM. So I moved most of the code of WallpaperManagerTest to WallpapermanagerBrowserTest. I also need help with DisplayChange test, because I couldn't make it work. Please review. https://codereview.chromium.org/215293003/diff/20001/ash/desktop_background/d... File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/215293003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.cc:110: if (WallpaperIsAlreadyLoaded(image)) { On 2014/04/01 15:32:56, bshe wrote: > should we check if layout is the same in the case? Otherwise, changing layout > probably not gonna work. Done. https://codereview.chromium.org/215293003/diff/20001/ash/desktop_background/d... File ash/desktop_background/desktop_background_controller.h (right): https://codereview.chromium.org/215293003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.h:108: WallpaperResolution GetAppropriateResolution(); On 2014/03/31 22:25:46, Daniel Erat wrote: > does this only get used by c/b/chromeos/login now? if so, please move it (and > the k{Small,Large}WallpaperMax{Width,Height} and thumbnail constants and the > WallpaperResolution enum) to c/b/chromeos/login Done. https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:32: #include "chrome/browser/chromeos/customization_document.h" On 2014/03/31 22:25:46, Daniel Erat wrote: > did you mean to add this include in the next change instead? Done. https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:762: MovableOnDestroyCallbackHolder on_finish) { On 2014/04/01 15:32:56, bshe wrote: > I dont see anywhere using wallpaper that packed in Chrome resource > (IDR_AURA_WALLPAPER_DEFAULT_LARGE) as fallback. I suspect the chromium build > wont have a wallpaper in this case and some tests may broken. > Done. https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:798: if (guest_default_small_wallpaper_image_.get() == NULL) { On 2014/03/31 22:25:46, Daniel Erat wrote: > nit: just do "if (!guest_default_small_wallpaper_image_) {" (and same for other > calls in this method) Done. https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:806: if (guest_default_large_wallpaper_image_.get() == NULL) { On 2014/03/31 22:25:46, Daniel Erat wrote: > there's a lot of duplicated code here. how about doing something like this > instead? > > base::FilePath path; > scoped_ptr<gfx::ImageSkia>* image; > > if (is_guest) { > if (use_small) { > path = guest_small_wallpaper_file_; > image = &guest_small_wallpaper_image_; > } else { > // etc. > } > > if (*image) { > StartLoad... > return; > } > ash::Shell...SetWallpaper(image, ...); Done. https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1167: CommandLine* WallpaperManager::GetComandLine() { On 2014/03/31 22:25:46, Daniel Erat wrote: > s/Comand/Command/ (please fix this typo elsewhere in the file, too) Done. https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:231: // Result is true on success. On 2014/03/31 22:25:46, Daniel Erat wrote: > nit: change to "Returns true on success." Done. https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:472: void SetDefaultWallpaperPathFromCommandLine(base::CommandLine* command_line); On 2014/03/31 22:25:46, Daniel Erat wrote: > nit: s/Path/Paths/ Done. https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:478: const UserImage& wallpaper); On 2014/04/01 15:32:56, bshe wrote: > nit: parameters that output result should appears after input. consider move > |wallpaper| above |result|. OnDefaultWallpaperDecoded is used to create callback (wallpaper) binding 3 first parameters. (The same as WallpaperManager::OnWallpaperDecoded). So the last parameter must be wallpaper const reference. https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:545: base::FilePath guest_default_small_wallpaper_file_; On 2014/03/31 22:25:46, Daniel Erat wrote: > mind renaming these to "guest_small_wallpaper_file_" and > "guest_large_wallpaper_file_" so people will be less likely to confuse them with > the non-guest defaults? Done. https://codereview.chromium.org/215293003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:549: scoped_ptr<gfx::ImageSkia> default_small_wallpaper_image_; On 2014/03/31 22:25:46, Daniel Erat wrote: > i'm pretty sure we don't want to hold all of these in memory. i believe that > DesktopBackgroundController only stored the currently-in-use image. Done. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: EXPECT_EQ(1, observer.GetUpdateWallpaperCountAndReset()); This check fails because UpdateWallpaper() doesn't happen here. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:797: EXPECT_EQ(1, observer.GetUpdateWallpaperCountAndReset()); This check fails because UpdateWallpaper() doesn't happen here. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:803: EXPECT_EQ(1, observer.GetUpdateWallpaperCountAndReset()); This check fails because UpdateWallpaper() doesn't happen here. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:809: EXPECT_EQ(1, observer.GetUpdateWallpaperCountAndReset()); This check fails because UpdateWallpaper() doesn't happen here. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:815: WallpaperManager::Get()->GetAppropriateResolution()); This check fails because: [wallpaper_manager.cc(663)] GetAppropriateResolution(): Actual MaxSize.width()=1576 vs kMaxWidth=1366, MaxSize.height()=2495 vs kMaxHeight=800 I do not know why GetAppropriateResolution() here returns these values. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:816: EXPECT_EQ(1, observer.GetUpdateWallpaperCountAndReset()); This check fails because UpdateWallpaper() doesn't happen here. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:821: WallpaperManager::Get()->GetAppropriateResolution()); This check fails because: [wallpaper_manager.cc(663)] GetAppropriateResolution(): MaxSize.width()=1576 vs kMaxWidth=1366, MaxSize.height()=2495 vs kMaxHeight=800 https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:827: EXPECT_EQ(1, observer.GetUpdateWallpaperCountAndReset()); This check fails because UpdateWallpaper() doesn't happen here.
thanks, i'm happy with this general approach! https://codereview.chromium.org/215293003/diff/40001/ash/desktop_background/d... File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/215293003/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.cc:207: if (image != NULL) nit: "if (image)"; also use curly brackets since the statement doesn't fit on a single line https://codereview.chromium.org/215293003/diff/40001/ash/desktop_background/d... File ash/desktop_background/desktop_background_controller.h (right): https://codereview.chromium.org/215293003/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.h:51: // Loads selected desktop wallpaper from file system asynchronously and updates update this comment https://codereview.chromium.org/215293003/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.h:87: bool SetWallpaper(int resource_id, WallpaperLayout layout); i'm not sure that we still need to support resources, but if we do, please give these methods different names instead of using function overloading, e.g. SetWallpaperImage and SetWallpaperResource. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://codereview.chromium.org/215293003/diff/40001/ash/desktop_background/w... File ash/desktop_background/wallpaper_resizer.h (right): https://codereview.chromium.org/215293003/diff/40001/ash/desktop_background/w... ash/desktop_background/wallpaper_resizer.h:68: // -1 if image was not obtained fromn resources. nit: s/fromn/from/ https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:770: void WallpaperManager::OnDefaultWallpaperDecoded( nit: please make this match its position in the header file https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:781: void WallpaperManager::StartLoadAndSetDefaultWallpaper( nit: please make this match its position in the header file https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:789: base::Unretained(this), can you use a weak_ptr here instead, or is this guaranteed not to run after WallpaperManager has been destroyed? https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:809: const bool is_guest = UserManager::Get()->IsLoggedInAsGuest(); nit: you don't use this anywhere else, and i don't see much reason to define this way up here. why not just move the UserManager::Get()->IsLoggedInAsGuest() directly into the if-condition where it's used below? https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:811: gfx::ImageSkia image; this doesn't get used anywhere, does it? if not, delete it https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:837: : IDR_AURA_WALLPAPER_DEFAULT_LARGE; hmm. so yeah, i think we should just drop support for using resources entirely, unless someone can suggest a good reason to keep them. i don't think that we ever intentionally use them on chrome os. if we need a fallback image, perhaps we can just create and pass a 1x1 black ImageSkia or something like that... https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:932: const char* sub_dir = (resolution == WALLPAPER_RESOLUTION_SMALL) this appears again below -- probably worthwhile to move it into a helper function: const char* GetCustomWallpaperSubdirForCurrentResolution() or something like that https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1568: void WallpaperManager::SetDefaultWallpaperPathsFromCommandLine( nit: please make this match its position in the header file https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:73: // small resolution wallpaper should be used. Otherwise, uses the large nit: s/uses/use/ https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:80: // The width and heigh of wallpaper thumbnails. nit: s/heigh/height/ https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:493: // Init "*default_*_wallpaper_file_" from given command line. nit: use pipes to refer to variable names in comments, e.g. |*_wallpaper_file_|, and also document that this clears |default_wallpaper_image_| https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:225: WallpaperManager::Get()->default_wallpaper_image_.reset(); please pass a command line with the appropriate flags into WallpaperManager instead of making the test modify the class's private members directly https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:716: class AshTestBaseAdapter : public ash::test::AshTestBase { this seems a bit strange. are the AshTestBase methods that you're calling protected static members or something like that? could you make them be public instead? https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: EXPECT_EQ(1, observer.GetUpdateWallpaperCountAndReset()); On 2014/04/03 19:15:45, alemate wrote: > This check fails because UpdateWallpaper() doesn't happen here. presumably the changes that UpdateDisplay() does don't make it back to WallpaperManager. i think that this usually goes through DesktopBackgroundController::OnDisplayConfigurationChanged(), but i don't think that there's much of a reason for DBC to be observing these changes -- all it does is call ash::Shell::GetInstance()->user_wallpaper_delegate()->UpdateWallpaper(), right? would it be possible to make WallpaperManager be a ash::DisplayController::Observer instead of DBC? i'm not sure what the right approach should be for DesktopBackgroundController::OnRootWindowAdded, though.
Please review. https://codereview.chromium.org/215293003/diff/40001/ash/desktop_background/d... File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/215293003/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.cc:207: if (image != NULL) On 2014/04/04 02:45:49, Daniel Erat wrote: > nit: "if (image)"; also use curly brackets since the statement doesn't fit on a > single line Done. https://codereview.chromium.org/215293003/diff/40001/ash/desktop_background/d... File ash/desktop_background/desktop_background_controller.h (right): https://codereview.chromium.org/215293003/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.h:87: bool SetWallpaper(int resource_id, WallpaperLayout layout); On 2014/04/04 02:45:49, Daniel Erat wrote: > i'm not sure that we still need to support resources, but if we do, please give > these methods different names instead of using function overloading, e.g. > SetWallpaperImage and SetWallpaperResource. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Done. https://codereview.chromium.org/215293003/diff/40001/ash/desktop_background/w... File ash/desktop_background/wallpaper_resizer.h (right): https://codereview.chromium.org/215293003/diff/40001/ash/desktop_background/w... ash/desktop_background/wallpaper_resizer.h:68: // -1 if image was not obtained fromn resources. On 2014/04/04 02:45:49, Daniel Erat wrote: > nit: s/fromn/from/ Done. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:770: void WallpaperManager::OnDefaultWallpaperDecoded( On 2014/04/04 02:45:49, Daniel Erat wrote: > nit: please make this match its position in the header file Done. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:781: void WallpaperManager::StartLoadAndSetDefaultWallpaper( On 2014/04/04 02:45:49, Daniel Erat wrote: > nit: please make this match its position in the header file Done. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:789: base::Unretained(this), On 2014/04/04 02:45:49, Daniel Erat wrote: > can you use a weak_ptr here instead, or is this guaranteed not to run after > WallpaperManager has been destroyed? Done. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:809: const bool is_guest = UserManager::Get()->IsLoggedInAsGuest(); On 2014/04/04 02:45:49, Daniel Erat wrote: > nit: you don't use this anywhere else, and i don't see much reason to define > this way up here. why not just move the UserManager::Get()->IsLoggedInAsGuest() > directly into the if-condition where it's used below? Done. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:811: gfx::ImageSkia image; On 2014/04/04 02:45:49, Daniel Erat wrote: > this doesn't get used anywhere, does it? if not, delete it Done. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:837: : IDR_AURA_WALLPAPER_DEFAULT_LARGE; On 2014/04/04 02:45:49, Daniel Erat wrote: > hmm. so yeah, i think we should just drop support for using resources entirely, > unless someone can suggest a good reason to keep them. i don't think that we > ever intentionally use them on chrome os. if we need a fallback image, perhaps > we can just create and pass a 1x1 black ImageSkia or something like that... This seems to me too much for this code refactoring. Let's drop resources support as a separate task. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:932: const char* sub_dir = (resolution == WALLPAPER_RESOLUTION_SMALL) On 2014/04/04 02:45:49, Daniel Erat wrote: > this appears again below -- probably worthwhile to move it into a helper > function: > > const char* GetCustomWallpaperSubdirForCurrentResolution() > > or something like that Done. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1568: void WallpaperManager::SetDefaultWallpaperPathsFromCommandLine( On 2014/04/04 02:45:49, Daniel Erat wrote: > nit: please make this match its position in the header file Done. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:73: // small resolution wallpaper should be used. Otherwise, uses the large On 2014/04/04 02:45:49, Daniel Erat wrote: > nit: s/uses/use/ Done. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:80: // The width and heigh of wallpaper thumbnails. On 2014/04/04 02:45:49, Daniel Erat wrote: > nit: s/heigh/height/ Done. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:493: // Init "*default_*_wallpaper_file_" from given command line. On 2014/04/04 02:45:49, Daniel Erat wrote: > nit: use pipes to refer to variable names in comments, e.g. |*_wallpaper_file_|, > and also document that this clears |default_wallpaper_image_| Done. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:225: WallpaperManager::Get()->default_wallpaper_image_.reset(); On 2014/04/04 02:45:49, Daniel Erat wrote: > please pass a command line with the appropriate flags into WallpaperManager > instead of making the test modify the class's private members directly Done. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:716: class AshTestBaseAdapter : public ash::test::AshTestBase { On 2014/04/04 02:45:49, Daniel Erat wrote: > this seems a bit strange. are the AshTestBase methods that you're calling > protected static members or something like that? could you make them be public > instead? Done. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: EXPECT_EQ(1, observer.GetUpdateWallpaperCountAndReset()); On 2014/04/04 02:45:49, Daniel Erat wrote: > On 2014/04/03 19:15:45, alemate wrote: > > This check fails because UpdateWallpaper() doesn't happen here. > > presumably the changes that UpdateDisplay() does don't make it back to > WallpaperManager. i think that this usually goes through > DesktopBackgroundController::OnDisplayConfigurationChanged(), but i don't think > that there's much of a reason for DBC to be observing these changes -- all it > does is call > ash::Shell::GetInstance()->user_wallpaper_delegate()->UpdateWallpaper(), right? > > would it be possible to make WallpaperManager be a > ash::DisplayController::Observer instead of DBC? i'm not sure what the right > approach should be for DesktopBackgroundController::OnRootWindowAdded, though. The problem is (probably) with UpdateDisplay. DBC::::OnDisplayConfigurationChanged() is analyzing display size, but as I've mentioned below, GetMaxDisplaySizeInNative() returns incorrect display size. Why?
lgtm after you figure out the testing issue https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: EXPECT_EQ(1, observer.GetUpdateWallpaperCountAndReset()); On 2014/04/05 03:35:13, alemate wrote: > On 2014/04/04 02:45:49, Daniel Erat wrote: > > On 2014/04/03 19:15:45, alemate wrote: > > > This check fails because UpdateWallpaper() doesn't happen here. > > > > presumably the changes that UpdateDisplay() does don't make it back to > > WallpaperManager. i think that this usually goes through > > DesktopBackgroundController::OnDisplayConfigurationChanged(), but i don't > think > > that there's much of a reason for DBC to be observing these changes -- all it > > does is call > > ash::Shell::GetInstance()->user_wallpaper_delegate()->UpdateWallpaper(), > right? > > > > would it be possible to make WallpaperManager be a > > ash::DisplayController::Observer instead of DBC? i'm not sure what the right > > approach should be for DesktopBackgroundController::OnRootWindowAdded, though. > > The problem is (probably) with UpdateDisplay. > DBC::::OnDisplayConfigurationChanged() is analyzing display size, but as I've > mentioned below, GetMaxDisplaySizeInNative() returns incorrect display size. > Why? maybe oshima has some ideas about this. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:1574: return (resolution == WALLPAPER_RESOLUTION_SMALL) ? kSmallWallpaperSubDir nit: remove parentheses around return value
https://codereview.chromium.org/215293003/diff/100001/ash/desktop_background/... File ash/desktop_background/wallpaper_resizer.cc (right): https://codereview.chromium.org/215293003/diff/100001/ash/desktop_background/... ash/desktop_background/wallpaper_resizer.cc:121: resource_id_(-1), nit: could you replace -1 with something like: const int kInvalidResourceID = -1; It would be easier to read. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:660: WallpaperManager::GetAppropriateResolution() { nit: I believe we should have 4 space indent when wrap to a new line. If the lint tool suggest differently, do you mind to confirm with the linter author to see if it's a linter bug or expected? https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:807: Should we fallback to resource wallpaper in OnDefaultWallpaperDecoded? If the wallpaper decoding is somehow failed (will return a null imageskia in this case), we should try to fall back to the resource wallpaper too. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:818: default_wallpaper_image_->image(), ash::WALLPAPER_LAYOUT_STRETCH); I believe for small resolution wallpaper, we should use WALLPAPER_LAYOUT_CENTER and for large resolution we should use WALLPAPER_LAYOUT_CENTER_CROPPED. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager_unittest.cc (right): https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_unittest.cc:37: #include "ui/gfx/rect.h" It looks like you are just removing a test here. Do you need all these new headers?
https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: EXPECT_EQ(1, observer.GetUpdateWallpaperCountAndReset()); On 2014/04/07 13:57:00, Daniel Erat wrote: > On 2014/04/05 03:35:13, alemate wrote: > > On 2014/04/04 02:45:49, Daniel Erat wrote: > > > On 2014/04/03 19:15:45, alemate wrote: > > > > This check fails because UpdateWallpaper() doesn't happen here. > > > > > > presumably the changes that UpdateDisplay() does don't make it back to > > > WallpaperManager. i think that this usually goes through > > > DesktopBackgroundController::OnDisplayConfigurationChanged(), but i don't > > think > > > that there's much of a reason for DBC to be observing these changes -- all > it > > > does is call > > > ash::Shell::GetInstance()->user_wallpaper_delegate()->UpdateWallpaper(), > > right? > > > > > > would it be possible to make WallpaperManager be a > > > ash::DisplayController::Observer instead of DBC? i'm not sure what the right > > > approach should be for DesktopBackgroundController::OnRootWindowAdded, > though. > > > > The problem is (probably) with UpdateDisplay. > > DBC::::OnDisplayConfigurationChanged() is analyzing display size, but as I've > > mentioned below, GetMaxDisplaySizeInNative() returns incorrect display size. > > Why? > > maybe oshima has some ideas about this. Let me look into this. It's hard to tell just by looking at the code. https://codereview.chromium.org/215293003/diff/100001/ash/desktop_background/... File ash/desktop_background/desktop_background_controller.h (right): https://codereview.chromium.org/215293003/diff/100001/ash/desktop_background/... ash/desktop_background/desktop_background_controller.h:27: class WallpaperManagerBrowserTestDefaultWallpaper; ash should not depend on chrome/browser/chromeos Can you provide testing api instaed or a public method for testing. https://codereview.chromium.org/215293003/diff/100001/ash/test/ash_test_base.h File ash/test/ash_test_base.h (left): https://codereview.chromium.org/215293003/diff/100001/ash/test/ash_test_base.... ash/test/ash_test_base.h:111: static bool SupportsHostWindowResize(); Would you mind moving the body of this method to AshTestHelper public method, and change this impl to simply call it, instead of exposing this to tests that doesn't use this class as a base. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:660: WallpaperManager::GetAppropriateResolution() { On 2014/04/07 15:49:16, bshe wrote: > nit: I believe we should have 4 space indent when wrap to a new line. If the > lint tool suggest differently, do you mind to confirm with the linter author to > see if it's a linter bug or expected? I believe this is fine. The recommended way nowadays is to just run git cl format, and I will simply accept it, even though it may not match surrounding code (as it eventually will). https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.h:119: virtual void OnUpdateWallpaperForTesting(); you may inline void empty method. (see ui/aura/window_observer.h for exmaple) https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.h:193: void set_command_line_for_testing(base::CommandLine* command_line); SetCommandLineForTesting if it's not inlined https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.h:256: gfx::ImageSkia* result) const; result_out https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.h:506: scoped_ptr<UserImage>* result, Looks like |result| output parameter? If so, move this after on_finish and rename to result_out. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:176: SkBitmap bitmap; optional: CreateTestImage(w,h,c).bitmap(); https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:256: wallpaper_manager_command_line_.get()); What's the reason why we can't do this in SetUpCommandLine? https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:737: LOG(ERROR) << "TestObserver: Created."; remove debug log. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:744: virtual void OnWallpaperAnimationFinished(const std::string&) { OVERRIDE https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:748: LOG(ERROR) << "TestObserver::OnUpdateWallpaperForTesting() called."; ditto https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:761: }; DISALLOW_COPY_AND_ASSIGN
On 2014/04/07 17:16:10, oshima wrote: > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: EXPECT_EQ(1, > observer.GetUpdateWallpaperCountAndReset()); > On 2014/04/07 13:57:00, Daniel Erat wrote: > > On 2014/04/05 03:35:13, alemate wrote: > > > On 2014/04/04 02:45:49, Daniel Erat wrote: > > > > On 2014/04/03 19:15:45, alemate wrote: > > > > > This check fails because UpdateWallpaper() doesn't happen here. > > > > > > > > presumably the changes that UpdateDisplay() does don't make it back to > > > > WallpaperManager. i think that this usually goes through > > > > DesktopBackgroundController::OnDisplayConfigurationChanged(), but i don't > > > think > > > > that there's much of a reason for DBC to be observing these changes -- all > > it > > > > does is call > > > > ash::Shell::GetInstance()->user_wallpaper_delegate()->UpdateWallpaper(), > > > right? > > > > > > > > would it be possible to make WallpaperManager be a > > > > ash::DisplayController::Observer instead of DBC? i'm not sure what the > right > > > > approach should be for DesktopBackgroundController::OnRootWindowAdded, > > though. > > > > > > The problem is (probably) with UpdateDisplay. > > > DBC::::OnDisplayConfigurationChanged() is analyzing display size, but as > I've > > > mentioned below, GetMaxDisplaySizeInNative() returns incorrect display size. > > > Why? > > > > maybe oshima has some ideas about this. > > Let me look into this. It's hard to tell just by looking at the code. nkostylev@ has shown to me today, that chrome command-line switch 'ash-host-window-bounds' doesn't allow multiple windows now. Like: --ash-host-window-bounds 800x600,1024x768 doesn't work anymore. It looks very similar to this.
On 2014/04/07 18:03:11, alemate wrote: > On 2014/04/07 17:16:10, oshima wrote: > > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > > File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): > > > > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > > chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: > EXPECT_EQ(1, > > observer.GetUpdateWallpaperCountAndReset()); > > On 2014/04/07 13:57:00, Daniel Erat wrote: > > > On 2014/04/05 03:35:13, alemate wrote: > > > > On 2014/04/04 02:45:49, Daniel Erat wrote: > > > > > On 2014/04/03 19:15:45, alemate wrote: > > > > > > This check fails because UpdateWallpaper() doesn't happen here. > > > > > > > > > > presumably the changes that UpdateDisplay() does don't make it back to > > > > > WallpaperManager. i think that this usually goes through > > > > > DesktopBackgroundController::OnDisplayConfigurationChanged(), but i > don't > > > > think > > > > > that there's much of a reason for DBC to be observing these changes -- > all > > > it > > > > > does is call > > > > > ash::Shell::GetInstance()->user_wallpaper_delegate()->UpdateWallpaper(), > > > > right? > > > > > > > > > > would it be possible to make WallpaperManager be a > > > > > ash::DisplayController::Observer instead of DBC? i'm not sure what the > > right > > > > > approach should be for DesktopBackgroundController::OnRootWindowAdded, > > > though. > > > > > > > > The problem is (probably) with UpdateDisplay. > > > > DBC::::OnDisplayConfigurationChanged() is analyzing display size, but as > > I've > > > > mentioned below, GetMaxDisplaySizeInNative() returns incorrect display > size. > > > > Why? > > > > > > maybe oshima has some ideas about this. > > > > Let me look into this. It's hard to tell just by looking at the code. > > > nkostylev@ has shown to me today, that chrome command-line switch > 'ash-host-window-bounds' doesn't allow multiple windows now. Like: > --ash-host-window-bounds 800x600,1024x768 > doesn't work anymore. It looks very similar to this. It should work and must be a bug if it doesn't. I'll look into this too. Thank you for reporting.
On 2014/04/07 18:09:07, oshima wrote: > On 2014/04/07 18:03:11, alemate wrote: > > On 2014/04/07 17:16:10, oshima wrote: > > > > > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > > > File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): > > > > > > > > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > > > chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: > > EXPECT_EQ(1, > > > observer.GetUpdateWallpaperCountAndReset()); > > > On 2014/04/07 13:57:00, Daniel Erat wrote: > > > > On 2014/04/05 03:35:13, alemate wrote: > > > > > On 2014/04/04 02:45:49, Daniel Erat wrote: > > > > > > On 2014/04/03 19:15:45, alemate wrote: > > > > > > > This check fails because UpdateWallpaper() doesn't happen here. > > > > > > > > > > > > presumably the changes that UpdateDisplay() does don't make it back to > > > > > > WallpaperManager. i think that this usually goes through > > > > > > DesktopBackgroundController::OnDisplayConfigurationChanged(), but i > > don't > > > > > think > > > > > > that there's much of a reason for DBC to be observing these changes -- > > all > > > > it > > > > > > does is call > > > > > > > ash::Shell::GetInstance()->user_wallpaper_delegate()->UpdateWallpaper(), > > > > > right? > > > > > > > > > > > > would it be possible to make WallpaperManager be a > > > > > > ash::DisplayController::Observer instead of DBC? i'm not sure what the > > > right > > > > > > approach should be for DesktopBackgroundController::OnRootWindowAdded, > > > > though. > > > > > > > > > > The problem is (probably) with UpdateDisplay. > > > > > DBC::::OnDisplayConfigurationChanged() is analyzing display size, but as > > > I've > > > > > mentioned below, GetMaxDisplaySizeInNative() returns incorrect display > > size. > > > > > Why? > > > > > > > > maybe oshima has some ideas about this. > > > > > > Let me look into this. It's hard to tell just by looking at the code. > > > > > > nkostylev@ has shown to me today, that chrome command-line switch > > 'ash-host-window-bounds' doesn't allow multiple windows now. Like: > > --ash-host-window-bounds 800x600,1024x768 > > doesn't work anymore. It looks very similar to this. > > It should work and must be a bug if it doesn't. I'll look into this too. > Thank you for reporting. Ah, the above parameter doesn't create another root window because both displays shares the same origin, in which case, chrome treat it as a mirror mode. (Mirrored displays shares the same origin) If you specify the origin, it should work. Note that this is "native" coordinates (in X), not a virtual coordinates that chrome sees. If you want to specify the display bounds that chrome uses, UpdateDisplay() is the right way.
https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: EXPECT_EQ(1, observer.GetUpdateWallpaperCountAndReset()); On 2014/04/07 17:16:11, oshima wrote: > On 2014/04/07 13:57:00, Daniel Erat wrote: > > On 2014/04/05 03:35:13, alemate wrote: > > > On 2014/04/04 02:45:49, Daniel Erat wrote: > > > > On 2014/04/03 19:15:45, alemate wrote: > > > > > This check fails because UpdateWallpaper() doesn't happen here. > > > > > > > > presumably the changes that UpdateDisplay() does don't make it back to > > > > WallpaperManager. i think that this usually goes through > > > > DesktopBackgroundController::OnDisplayConfigurationChanged(), but i don't > > > think > > > > that there's much of a reason for DBC to be observing these changes -- all > > it > > > > does is call > > > > ash::Shell::GetInstance()->user_wallpaper_delegate()->UpdateWallpaper(), > > > right? > > > > > > > > would it be possible to make WallpaperManager be a > > > > ash::DisplayController::Observer instead of DBC? i'm not sure what the > right > > > > approach should be for DesktopBackgroundController::OnRootWindowAdded, > > though. > > > > > > The problem is (probably) with UpdateDisplay. > > > DBC::::OnDisplayConfigurationChanged() is analyzing display size, but as > I've > > > mentioned below, GetMaxDisplaySizeInNative() returns incorrect display size. > > > Why? > > > > maybe oshima has some ideas about this. > > Let me look into this. It's hard to tell just by looking at the code. The failure was due to missing DesktopBackgroundController::set_wallpaper_reload_delay_for_test(0) (see DesktopBackgroundControllerTest::SetUp https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:96: base::MessageLoop::current()->RunUntilIdle(); MessageLoop::RunUntilIdle is deprecated. Would you mind changing to use RunLoop? https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:104: } This is ugly. Isn't there a way to reliably tell when loading is complete?
oshima@, Please review again. Test issue has been solved partially. https://codereview.chromium.org/215293003/diff/40001/ash/desktop_background/d... File ash/desktop_background/desktop_background_controller.h (right): https://codereview.chromium.org/215293003/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.h:51: // Loads selected desktop wallpaper from file system asynchronously and updates On 2014/04/04 02:45:49, Daniel Erat wrote: > update this comment Done. https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: EXPECT_EQ(1, observer.GetUpdateWallpaperCountAndReset()); On 2014/04/07 19:02:28, oshima wrote: > On 2014/04/07 17:16:11, oshima wrote: > > On 2014/04/07 13:57:00, Daniel Erat wrote: > > > On 2014/04/05 03:35:13, alemate wrote: > > > > On 2014/04/04 02:45:49, Daniel Erat wrote: > > > > > On 2014/04/03 19:15:45, alemate wrote: > > > > > > This check fails because UpdateWallpaper() doesn't happen here. > > > > > > > > > > presumably the changes that UpdateDisplay() does don't make it back to > > > > > WallpaperManager. i think that this usually goes through > > > > > DesktopBackgroundController::OnDisplayConfigurationChanged(), but i > don't > > > > think > > > > > that there's much of a reason for DBC to be observing these changes -- > all > > > it > > > > > does is call > > > > > ash::Shell::GetInstance()->user_wallpaper_delegate()->UpdateWallpaper(), > > > > right? > > > > > > > > > > would it be possible to make WallpaperManager be a > > > > > ash::DisplayController::Observer instead of DBC? i'm not sure what the > > right > > > > > approach should be for DesktopBackgroundController::OnRootWindowAdded, > > > though. > > > > > > > > The problem is (probably) with UpdateDisplay. > > > > DBC::::OnDisplayConfigurationChanged() is analyzing display size, but as > > I've > > > > mentioned below, GetMaxDisplaySizeInNative() returns incorrect display > size. > > > > Why? > > > > > > maybe oshima has some ideas about this. > > > > Let me look into this. It's hard to tell just by looking at the code. > > The failure was due to missing > > DesktopBackgroundController::set_wallpaper_reload_delay_for_test(0) (see > DesktopBackgroundControllerTest::SetUp > This helps only partially, here is my debug: [ RUN ] WallpaperManagerBrowserTestInstantiation/WallpaperManagerBrowserTest.DisplayChange/0 [22965:22965:0408/165937:WARNING:profile_io_data.cc(366)] no username_hash [22965:22965:0408/165937:WARNING:profile_io_data.cc(366)] no username_hash [22994:22994:0408/125937:WARNING:sandbox_linux.cc(56)] Activated seccomp-bpf sandbox for process type: gpu-process. [22965:22965:0408/165937:ERROR:wallpaper_manager_browsertest.cc(95)] UpdateDisplay(800x600) called. [22965:22965:0408/165937:ERROR:wallpaper_manager_browsertest.cc(95)] UpdateDisplay(800x600) called. [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=800 vs kSmallWallpaperMaxWidth=1366, height=600 vs kSmallWallpaperMaxHeight = 800 [22965:22965:0408/165937:ERROR:wallpaper_manager_browsertest.cc(95)] UpdateDisplay(800x600,800x600) called. [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=800 vs kSmallWallpaperMaxWidth=1366, height=600 vs kSmallWallpaperMaxHeight = 800 [22965:22965:0408/165937:ERROR:wallpaper_manager_browsertest.cc(95)] UpdateDisplay(1366x800) called. [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=1366 vs kSmallWallpaperMaxWidth=1366, height=800 vs kSmallWallpaperMaxHeight = 800 [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=1366 vs kSmallWallpaperMaxWidth=1366, height=800 vs kSmallWallpaperMaxHeight = 800 [22965:22965:0408/165937:ERROR:wallpaper_manager_browsertest.cc(95)] UpdateDisplay(1367x800) called. [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=1367 vs kSmallWallpaperMaxWidth=1366, height=800 vs kSmallWallpaperMaxHeight = 800 [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=1367 vs kSmallWallpaperMaxWidth=1366, height=800 vs kSmallWallpaperMaxHeight = 800 [22965:22965:0408/165937:ERROR:wallpaper_manager_browsertest.cc(95)] UpdateDisplay(1367x801) called. [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=1367 vs kSmallWallpaperMaxWidth=1366, height=801 vs kSmallWallpaperMaxHeight = 800 [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=1367 vs kSmallWallpaperMaxWidth=1366, height=801 vs kSmallWallpaperMaxHeight = 800 [22965:22965:0408/165937:ERROR:wallpaper_manager_browsertest.cc(95)] UpdateDisplay(2560x1700) called. [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=2493 vs kSmallWallpaperMaxWidth=1366, height=1547 vs kSmallWallpaperMaxHeight = 800 [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=2493 vs kSmallWallpaperMaxWidth=1366, height=1547 vs kSmallWallpaperMaxHeight = 800 [22965:22965:0408/165937:ERROR:wallpaper_manager_browsertest.cc(95)] UpdateDisplay(800x600/r) called. [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=600 vs kSmallWallpaperMaxWidth=1366, height=800 vs kSmallWallpaperMaxHeight = 800 [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=600 vs kSmallWallpaperMaxWidth=1366, height=800 vs kSmallWallpaperMaxHeight = 800 [22965:22965:0408/165937:ERROR:wallpaper_manager_browsertest.cc(95)] UpdateDisplay(800x600/r,800x600) called. [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=1576 vs kSmallWallpaperMaxWidth=1366, height=2495 vs kSmallWallpaperMaxHeight = 800 [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=1576 vs kSmallWallpaperMaxWidth=1366, height=2495 vs kSmallWallpaperMaxHeight = 800 ../../chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:835: Failure Value of: WallpaperManager::Get()->GetAppropriateResolution() Actual: 0 Expected: WallpaperManager::WALLPAPER_RESOLUTION_SMALL Which is: 1 [22965:22965:0408/165937:ERROR:wallpaper_manager_browsertest.cc(95)] UpdateDisplay(1366x800/r) called. [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=1576 vs kSmallWallpaperMaxWidth=1366, height=2495 vs kSmallWallpaperMaxHeight = 800 [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=1576 vs kSmallWallpaperMaxWidth=1366, height=2495 vs kSmallWallpaperMaxHeight = 800 [22965:22965:0408/165937:ERROR:wallpaper_manager_browsertest.cc(95)] UpdateDisplay(900x800/r,400x1366) called. [22965:22965:0408/165937:ERROR:wallpaper_manager.cc(661)] GetAppropriateResolution(: width=1576 vs kSmallWallpaperMaxWidth=1366, height=2495 vs kSmallWallpaperMaxHeight = 800 ../../chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:846: Failure Value of: observer.GetUpdateWallpaperCountAndReset() Actual: 1 Expected: 0 [ FAILED ] WallpaperManagerBrowserTestInstantiation/WallpaperManagerBrowserTest.DisplayChange/0, where GetParam() = false (1058 ms) https://codereview.chromium.org/215293003/diff/100001/ash/desktop_background/... File ash/desktop_background/desktop_background_controller.h (right): https://codereview.chromium.org/215293003/diff/100001/ash/desktop_background/... ash/desktop_background/desktop_background_controller.h:27: class WallpaperManagerBrowserTestDefaultWallpaper; On 2014/04/07 17:16:11, oshima wrote: > ash should not depend on chrome/browser/chromeos > Can you provide testing api instaed or a public method for testing. Done. https://codereview.chromium.org/215293003/diff/100001/ash/desktop_background/... File ash/desktop_background/wallpaper_resizer.cc (right): https://codereview.chromium.org/215293003/diff/100001/ash/desktop_background/... ash/desktop_background/wallpaper_resizer.cc:121: resource_id_(-1), On 2014/04/07 15:49:16, bshe wrote: > nit: could you replace -1 with something like: > const int kInvalidResourceID = -1; > It would be easier to read. Done. https://codereview.chromium.org/215293003/diff/100001/ash/test/ash_test_base.h File ash/test/ash_test_base.h (left): https://codereview.chromium.org/215293003/diff/100001/ash/test/ash_test_base.... ash/test/ash_test_base.h:111: static bool SupportsHostWindowResize(); On 2014/04/07 17:16:11, oshima wrote: > Would you mind moving the body of this method to > AshTestHelper public method, and change this impl to simply call it, > instead of exposing this to tests that doesn't use this class as a base. > Done. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:807: On 2014/04/07 15:49:16, bshe wrote: > Should we fallback to resource wallpaper in OnDefaultWallpaperDecoded? If the > wallpaper decoding is somehow failed (will return a null imageskia in this > case), we should try to fall back to the resource wallpaper too. This is different to the way it works now (I don't know why). I can implement it if you are sure we should change behavior here. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:818: default_wallpaper_image_->image(), ash::WALLPAPER_LAYOUT_STRETCH); On 2014/04/07 15:49:16, bshe wrote: > I believe for small resolution wallpaper, we should use WALLPAPER_LAYOUT_CENTER > and for large resolution we should use WALLPAPER_LAYOUT_CENTER_CROPPED. Done. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:1574: return (resolution == WALLPAPER_RESOLUTION_SMALL) ? kSmallWallpaperSubDir On 2014/04/07 13:57:00, Daniel Erat wrote: > nit: remove parentheses around return value Done. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.h:119: virtual void OnUpdateWallpaperForTesting(); On 2014/04/07 17:16:11, oshima wrote: > you may inline void empty method. (see ui/aura/window_observer.h for exmaple) Done. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.h:193: void set_command_line_for_testing(base::CommandLine* command_line); On 2014/04/07 17:16:11, oshima wrote: > SetCommandLineForTesting if it's not inlined Done. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.h:256: gfx::ImageSkia* result) const; On 2014/04/07 17:16:11, oshima wrote: > result_out Done. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.h:506: scoped_ptr<UserImage>* result, On 2014/04/07 17:16:11, oshima wrote: > Looks like |result| output parameter? If so, move this after on_finish and > rename to result_out. Done. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:96: base::MessageLoop::current()->RunUntilIdle(); On 2014/04/07 19:02:28, oshima wrote: > MessageLoop::RunUntilIdle is deprecated. Would you mind changing to use RunLoop? Done. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:104: } On 2014/04/07 19:02:28, oshima wrote: > This is ugly. Isn't there a way to reliably tell when loading is complete? The idea is to wait until "final" wallpaper load request finishes. We can wait for particular load request, or for the next wallpaper change, but here we need to be sure that no more load requests exist. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:176: SkBitmap bitmap; On 2014/04/07 17:16:11, oshima wrote: > optional: CreateTestImage(w,h,c).bitmap(); Done. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:256: wallpaper_manager_command_line_.get()); On 2014/04/07 17:16:11, oshima wrote: > What's the reason why we can't do this in SetUpCommandLine? For the old tests (i.e. not moved from unittests) we do not need wallpaper files. So the idea was not to change the old environment. Do you think we should have the same parameters for all tests? https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:737: LOG(ERROR) << "TestObserver: Created."; On 2014/04/07 17:16:11, oshima wrote: > remove debug log. Done. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:744: virtual void OnWallpaperAnimationFinished(const std::string&) { On 2014/04/07 17:16:11, oshima wrote: > OVERRIDE Done. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:748: LOG(ERROR) << "TestObserver::OnUpdateWallpaperForTesting() called."; On 2014/04/07 17:16:11, oshima wrote: > ditto Done. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:761: }; On 2014/04/07 17:16:11, oshima wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager_unittest.cc (right): https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_unittest.cc:37: #include "ui/gfx/rect.h" On 2014/04/07 15:49:16, bshe wrote: > It looks like you are just removing a test here. Do you need all these new > headers? No. I've removed them.
https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: EXPECT_EQ(1, observer.GetUpdateWallpaperCountAndReset()); On 2014/04/08 13:18:49, alemate wrote: > ... The same debug in a more compact format: UpdateDisplay(800x600) called. UpdateDisplay(800x600) called. GetAppropriateResolution: width=800, height=600 UpdateDisplay(800x600,800x600) called. GetAppropriateResolution: width=800, height=600 UpdateDisplay(1366x800) called. GetAppropriateResolution: width=1366, height=800 GetAppropriateResolution: width=1366, height=800 UpdateDisplay(1367x800) called. GetAppropriateResolution: width=1367, height=800 GetAppropriateResolution: width=1367, height=800 UpdateDisplay(1367x801) called. GetAppropriateResolution: width=1367, height=801 GetAppropriateResolution: width=1367, height=801 UpdateDisplay(2560x1700) called. GetAppropriateResolution: width=2493, height=1547 GetAppropriateResolution: width=2493, height=1547 UpdateDisplay(800x600/r) called. GetAppropriateResolution: width=600, height=800 GetAppropriateResolution: width=600, height=800 UpdateDisplay(800x600/r,800x600) called. GetAppropriateResolution: width=1576, height=2495 GetAppropriateResolution: width=1576, height=2495 ../../chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:835: Failure Value of: WallpaperManager::Get()->GetAppropriateResolution) Actual: 0 Expected: WallpaperManager::WALLPAPER_RESOLUTION_SMALL Which is: 1 UpdateDisplay(1366x800/r) called. GetAppropriateResolution: width=1576, height=2495 GetAppropriateResolution: width=1576, height=2495 UpdateDisplay(900x800/r,400x1366) called. GetAppropriateResolution: width=1576, height=2495
https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:807: On 2014/04/08 13:18:49, alemate wrote: > On 2014/04/07 15:49:16, bshe wrote: > > Should we fallback to resource wallpaper in OnDefaultWallpaperDecoded? If the > > wallpaper decoding is somehow failed (will return a null imageskia in this > > case), we should try to fall back to the resource wallpaper too. > > This is different to the way it works now (I don't know why). I can > implement it if you are sure we should change behavior here. i wouldn't recommend spending any more time on the resource fallback path. i think we should remove it entirely. i'll do it after this change if you don't want to. :-)
On 2014/04/08 13:32:30, alemate wrote: > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: EXPECT_EQ(1, > observer.GetUpdateWallpaperCountAndReset()); > On 2014/04/08 13:18:49, alemate wrote: > > ... > > The same debug in a more compact format: > > UpdateDisplay(800x600) called. > > UpdateDisplay(800x600) called. > > GetAppropriateResolution: width=800, height=600 > > UpdateDisplay(800x600,800x600) called. > > GetAppropriateResolution: width=800, height=600 > > UpdateDisplay(1366x800) called. > > GetAppropriateResolution: width=1366, height=800 > > GetAppropriateResolution: width=1366, height=800 > > UpdateDisplay(1367x800) called. > > GetAppropriateResolution: width=1367, height=800 > > GetAppropriateResolution: width=1367, height=800 > > UpdateDisplay(1367x801) called. > > GetAppropriateResolution: width=1367, height=801 > > GetAppropriateResolution: width=1367, height=801 > > UpdateDisplay(2560x1700) called. > > GetAppropriateResolution: width=2493, height=1547 > > GetAppropriateResolution: width=2493, height=1547 > > UpdateDisplay(800x600/r) called. > > GetAppropriateResolution: width=600, height=800 > > GetAppropriateResolution: width=600, height=800 > > UpdateDisplay(800x600/r,800x600) called. > > GetAppropriateResolution: width=1576, height=2495 > > GetAppropriateResolution: width=1576, height=2495 > > ../../chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:835: > Failure > Value of: WallpaperManager::Get()->GetAppropriateResolution) > > Actual: 0 > > Expected: WallpaperManager::WALLPAPER_RESOLUTION_SMALL > > Which is: 1 > > UpdateDisplay(1366x800/r) called. > > GetAppropriateResolution: width=1576, height=2495 > > GetAppropriateResolution: width=1576, height=2495 > > UpdateDisplay(900x800/r,400x1366) called. > > GetAppropriateResolution: width=1576, height=2495 How did you run your test? If you run it on your Ubuntu desktop, your WM may interfere with the test. Can you run it with Xvfb ?
lgtm https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:807: I believe the original behavior is to fallback to the resource wallpaper if failure, not just when the wallpaper flag is not set. When the default wallpaper (especially we are going to download it from server now) is somehow corrupted, I think it is slightly better to fallback to a resource wallpaper instead of a solid black background. But I dont worry it too much as I believe this case should be really rare and easy to recover. You can keep it as it is. The resource fallback is going to be used by linux_chromeos and chromium build. If we remove it, I worry that these builds will have a solid black background. So we probably want to keep it here? On 2014/04/08 14:11:16, Daniel Erat wrote: > On 2014/04/08 13:18:49, alemate wrote: > > On 2014/04/07 15:49:16, bshe wrote: > > > Should we fallback to resource wallpaper in OnDefaultWallpaperDecoded? If > the > > > wallpaper decoding is somehow failed (will return a null imageskia in this > > > case), we should try to fall back to the resource wallpaper too. > > > > This is different to the way it works now (I don't know why). I can > > implement it if you are sure we should change behavior here. > > i wouldn't recommend spending any more time on the resource fallback path. i > think we should remove it entirely. i'll do it after this change if you don't > want to. :-)
On 2014/04/08 14:26:23, oshima wrote: > On 2014/04/08 13:32:30, alemate wrote: > > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > > File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): > > > > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > > chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: > EXPECT_EQ(1, > > observer.GetUpdateWallpaperCountAndReset()); > > On 2014/04/08 13:18:49, alemate wrote: > > > ... > > > > The same debug in a more compact format: > > > > UpdateDisplay(800x600) called. > > > > > UpdateDisplay(800x600) called. > > > > > GetAppropriateResolution: width=800, height=600 > > > > > UpdateDisplay(800x600,800x600) called. > > > > > GetAppropriateResolution: width=800, height=600 > > > > > UpdateDisplay(1366x800) called. > > > > > GetAppropriateResolution: width=1366, height=800 > > > > > GetAppropriateResolution: width=1366, height=800 > > > > > UpdateDisplay(1367x800) called. > > > > > GetAppropriateResolution: width=1367, height=800 > > > > > GetAppropriateResolution: width=1367, height=800 > > > > > UpdateDisplay(1367x801) called. > > > > > GetAppropriateResolution: width=1367, height=801 > > > > > GetAppropriateResolution: width=1367, height=801 > > > > > UpdateDisplay(2560x1700) called. > > > > > GetAppropriateResolution: width=2493, height=1547 > > > > > GetAppropriateResolution: width=2493, height=1547 > > > > > UpdateDisplay(800x600/r) called. > > > > > GetAppropriateResolution: width=600, height=800 > > > > > GetAppropriateResolution: width=600, height=800 > > > > > UpdateDisplay(800x600/r,800x600) called. > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > ../../chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:835: > > Failure > > Value of: WallpaperManager::Get()->GetAppropriateResolution) > > > > > Actual: 0 > > > > > Expected: WallpaperManager::WALLPAPER_RESOLUTION_SMALL > > > > > Which is: 1 > > > > > UpdateDisplay(1366x800/r) called. > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > UpdateDisplay(900x800/r,400x1366) called. > > > > > GetAppropriateResolution: width=1576, height=2495 > > How did you run your test? If you run it on your Ubuntu desktop, > your WM may interfere with the test. Can you run it with Xvfb ? It works with Xvfb. I was hoping tests do not depend on the environment.
On 2014/04/08 15:58:34, alemate wrote: > On 2014/04/08 14:26:23, oshima wrote: > > On 2014/04/08 13:32:30, alemate wrote: > > > > > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > > > File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): > > > > > > > > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > > > chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: > > EXPECT_EQ(1, > > > observer.GetUpdateWallpaperCountAndReset()); > > > On 2014/04/08 13:18:49, alemate wrote: > > > > ... > > > > > > The same debug in a more compact format: > > > > > > UpdateDisplay(800x600) called. > > > > > > > > > UpdateDisplay(800x600) called. > > > > > > > > > GetAppropriateResolution: width=800, height=600 > > > > > > > > > UpdateDisplay(800x600,800x600) called. > > > > > > > > > GetAppropriateResolution: width=800, height=600 > > > > > > > > > UpdateDisplay(1366x800) called. > > > > > > > > > GetAppropriateResolution: width=1366, height=800 > > > > > > > > > GetAppropriateResolution: width=1366, height=800 > > > > > > > > > UpdateDisplay(1367x800) called. > > > > > > > > > GetAppropriateResolution: width=1367, height=800 > > > > > > > > > GetAppropriateResolution: width=1367, height=800 > > > > > > > > > UpdateDisplay(1367x801) called. > > > > > > > > > GetAppropriateResolution: width=1367, height=801 > > > > > > > > > GetAppropriateResolution: width=1367, height=801 > > > > > > > > > UpdateDisplay(2560x1700) called. > > > > > > > > > GetAppropriateResolution: width=2493, height=1547 > > > > > > > > > GetAppropriateResolution: width=2493, height=1547 > > > > > > > > > UpdateDisplay(800x600/r) called. > > > > > > > > > GetAppropriateResolution: width=600, height=800 > > > > > > > > > GetAppropriateResolution: width=600, height=800 > > > > > > > > > UpdateDisplay(800x600/r,800x600) called. > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > ../../chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:835: > > > Failure > > > Value of: WallpaperManager::Get()->GetAppropriateResolution) > > > > > > > > > Actual: 0 > > > > > > > > > Expected: WallpaperManager::WALLPAPER_RESOLUTION_SMALL > > > > > > > > > Which is: 1 > > > > > > > > > UpdateDisplay(1366x800/r) called. > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > UpdateDisplay(900x800/r,400x1366) called. > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > How did you run your test? If you run it on your Ubuntu desktop, > > your WM may interfere with the test. Can you run it with Xvfb ? > > It works with Xvfb. I was hoping tests do not depend on the environment.
On 2014/04/08 16:14:46, oshima wrote: > On 2014/04/08 15:58:34, alemate wrote: > > On 2014/04/08 14:26:23, oshima wrote: > > > On 2014/04/08 13:32:30, alemate wrote: > > > > > > > > > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > > > > File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > > > > chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: > > > EXPECT_EQ(1, > > > > observer.GetUpdateWallpaperCountAndReset()); > > > > On 2014/04/08 13:18:49, alemate wrote: > > > > > ... > > > > > > > > The same debug in a more compact format: > > > > > > > > UpdateDisplay(800x600) called. > > > > > > > > > > > > > > UpdateDisplay(800x600) called. > > > > > > > > > > > > > > GetAppropriateResolution: width=800, height=600 > > > > > > > > > > > > > > UpdateDisplay(800x600,800x600) called. > > > > > > > > > > > > > > GetAppropriateResolution: width=800, height=600 > > > > > > > > > > > > > > UpdateDisplay(1366x800) called. > > > > > > > > > > > > > > GetAppropriateResolution: width=1366, height=800 > > > > > > > > > > > > > > GetAppropriateResolution: width=1366, height=800 > > > > > > > > > > > > > > UpdateDisplay(1367x800) called. > > > > > > > > > > > > > > GetAppropriateResolution: width=1367, height=800 > > > > > > > > > > > > > > GetAppropriateResolution: width=1367, height=800 > > > > > > > > > > > > > > UpdateDisplay(1367x801) called. > > > > > > > > > > > > > > GetAppropriateResolution: width=1367, height=801 > > > > > > > > > > > > > > GetAppropriateResolution: width=1367, height=801 > > > > > > > > > > > > > > UpdateDisplay(2560x1700) called. > > > > > > > > > > > > > > GetAppropriateResolution: width=2493, height=1547 > > > > > > > > > > > > > > GetAppropriateResolution: width=2493, height=1547 > > > > > > > > > > > > > > UpdateDisplay(800x600/r) called. > > > > > > > > > > > > > > GetAppropriateResolution: width=600, height=800 > > > > > > > > > > > > > > GetAppropriateResolution: width=600, height=800 > > > > > > > > > > > > > > UpdateDisplay(800x600/r,800x600) called. > > > > > > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > > > > > > ../../chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:835: > > > > Failure > > > > Value of: WallpaperManager::Get()->GetAppropriateResolution) > > > > > > > > > > > > > > Actual: 0 > > > > > > > > > > > > > > Expected: WallpaperManager::WALLPAPER_RESOLUTION_SMALL > > > > > > > > > > > > > > Which is: 1 > > > > > > > > > > > > > > UpdateDisplay(1366x800/r) called. > > > > > > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > > > > > > UpdateDisplay(900x800/r,400x1366) called. > > > > > > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > How did you run your test? If you run it on your Ubuntu desktop, > > > your WM may interfere with the test. Can you run it with Xvfb ? > > > > It works with Xvfb. I was hoping tests do not depend on the environment.
On 2014/04/08 16:18:08, oshima wrote: > On 2014/04/08 16:14:46, oshima wrote: > > On 2014/04/08 15:58:34, alemate wrote: > > > On 2014/04/08 14:26:23, oshima wrote: > > > > On 2014/04/08 13:32:30, alemate wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > > > > > File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > > > > > chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: > > > > EXPECT_EQ(1, > > > > > observer.GetUpdateWallpaperCountAndReset()); > > > > > On 2014/04/08 13:18:49, alemate wrote: > > > > > > ... > > > > > > > > > > The same debug in a more compact format: > > > > > > > > > > UpdateDisplay(800x600) called. > > > > > > > > > > > > > > > > > > > > UpdateDisplay(800x600) called. > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=800, height=600 > > > > > > > > > > > > > > > > > > > > UpdateDisplay(800x600,800x600) called. > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=800, height=600 > > > > > > > > > > > > > > > > > > > > UpdateDisplay(1366x800) called. > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=1366, height=800 > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=1366, height=800 > > > > > > > > > > > > > > > > > > > > UpdateDisplay(1367x800) called. > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=1367, height=800 > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=1367, height=800 > > > > > > > > > > > > > > > > > > > > UpdateDisplay(1367x801) called. > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=1367, height=801 > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=1367, height=801 > > > > > > > > > > > > > > > > > > > > UpdateDisplay(2560x1700) called. > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=2493, height=1547 > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=2493, height=1547 > > > > > > > > > > > > > > > > > > > > UpdateDisplay(800x600/r) called. > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=600, height=800 > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=600, height=800 > > > > > > > > > > > > > > > > > > > > UpdateDisplay(800x600/r,800x600) called. > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > > > > > > > > > > > > > ../../chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:835: > > > > > Failure > > > > > Value of: WallpaperManager::Get()->GetAppropriateResolution) > > > > > > > > > > > > > > > > > > > > Actual: 0 > > > > > > > > > > > > > > > > > > > > Expected: WallpaperManager::WALLPAPER_RESOLUTION_SMALL > > > > > > > > > > > > > > > > > > > > Which is: 1 > > > > > > > > > > > > > > > > > > > > UpdateDisplay(1366x800/r) called. > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > > > > > > > > > > > > UpdateDisplay(900x800/r,400x1366) called. > > > > > > > > > > > > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > How did you run your test? If you run it on your Ubuntu desktop, > > > > your WM may interfere with the test. Can you run it with Xvfb ? > > > > > > It works with Xvfb. I was hoping tests do not depend on the environment.
https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:807: On 2014/04/08 15:00:39, bshe wrote: > I believe the original behavior is to fallback to the resource wallpaper if > failure, not just when the wallpaper flag is not set. When the default wallpaper > (especially we are going to download it from server now) is somehow corrupted, I > think it is slightly better to fallback to a resource wallpaper instead of a > solid black background. But I dont worry it too much as I believe this case > should be really rare and easy to recover. You can keep it as it is. > > The resource fallback is going to be used by linux_chromeos and chromium build. > If we remove it, I worry that these builds will have a solid black background. > So we probably want to keep it here? the resource file is just a solid gray color. the code should dynamically generate a gfx::Image with the same color if it needs to fall back (but since this is an unexpected case, it'd also be fine to just use a solid black background here). > On 2014/04/08 14:11:16, Daniel Erat wrote: > > On 2014/04/08 13:18:49, alemate wrote: > > > On 2014/04/07 15:49:16, bshe wrote: > > > > Should we fallback to resource wallpaper in OnDefaultWallpaperDecoded? If > > the > > > > wallpaper decoding is somehow failed (will return a null imageskia in this > > > > case), we should try to fall back to the resource wallpaper too. > > > > > > This is different to the way it works now (I don't know why). I can > > > implement it if you are sure we should change behavior here. > > > > i wouldn't recommend spending any more time on the resource fallback path. i > > think we should remove it entirely. i'll do it after this change if you don't > > want to. :-) >
On 2014/04/08 15:58:34, alemate wrote: > On 2014/04/08 14:26:23, oshima wrote: > > On 2014/04/08 13:32:30, alemate wrote: > > > > > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > > > File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): > > > > > > > > > https://codereview.chromium.org/215293003/diff/40001/chrome/browser/chromeos/... > > > chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:790: > > EXPECT_EQ(1, > > > observer.GetUpdateWallpaperCountAndReset()); > > > On 2014/04/08 13:18:49, alemate wrote: > > > > ... > > > > > > The same debug in a more compact format: > > > > > > UpdateDisplay(800x600) called. > > > > > > > > > UpdateDisplay(800x600) called. > > > > > > > > > GetAppropriateResolution: width=800, height=600 > > > > > > > > > UpdateDisplay(800x600,800x600) called. > > > > > > > > > GetAppropriateResolution: width=800, height=600 > > > > > > > > > UpdateDisplay(1366x800) called. > > > > > > > > > GetAppropriateResolution: width=1366, height=800 > > > > > > > > > GetAppropriateResolution: width=1366, height=800 > > > > > > > > > UpdateDisplay(1367x800) called. > > > > > > > > > GetAppropriateResolution: width=1367, height=800 > > > > > > > > > GetAppropriateResolution: width=1367, height=800 > > > > > > > > > UpdateDisplay(1367x801) called. > > > > > > > > > GetAppropriateResolution: width=1367, height=801 > > > > > > > > > GetAppropriateResolution: width=1367, height=801 > > > > > > > > > UpdateDisplay(2560x1700) called. > > > > > > > > > GetAppropriateResolution: width=2493, height=1547 > > > > > > > > > GetAppropriateResolution: width=2493, height=1547 > > > > > > > > > UpdateDisplay(800x600/r) called. > > > > > > > > > GetAppropriateResolution: width=600, height=800 > > > > > > > > > GetAppropriateResolution: width=600, height=800 > > > > > > > > > UpdateDisplay(800x600/r,800x600) called. > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > ../../chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:835: > > > Failure > > > Value of: WallpaperManager::Get()->GetAppropriateResolution) > > > > > > > > > Actual: 0 > > > > > > > > > Expected: WallpaperManager::WALLPAPER_RESOLUTION_SMALL > > > > > > > > > Which is: 1 > > > > > > > > > UpdateDisplay(1366x800/r) called. > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > > > > > > UpdateDisplay(900x800/r,400x1366) called. > > > > > > > > > GetAppropriateResolution: width=1576, height=2495 > > > > How did you run your test? If you run it on your Ubuntu desktop, > > your WM may interfere with the test. Can you run it with Xvfb ? > > It works with Xvfb. I was hoping tests do not depend on the environment. Sorry, it seems that my touchpad generated multiple clicks for some reason. We probably should set override redirect on host window to mitigate this, although interactive_ui_tests does require isolated environment because the test depends on the shared resources in window system (such as focus/activation/mouse). I'm not sure why this test is in interactive_ui_test though (> bshe?) I'll send the review for the latest patch shortly.
Please take another look at ash/shell/content_client/shell_browser_main_parts.cc https://codereview.chromium.org/215293003/diff/130001/ash/shell/content_clien... File ash/shell/content_client/shell_browser_main_parts.cc (right): https://codereview.chromium.org/215293003/diff/130001/ash/shell/content_clien... ash/shell/content_client/shell_browser_main_parts.cc:140: SetDefaultWallpaperDelayed(chromeos::UserManager::kSignInUser); This breaks dependencies. Should I remove this call completely? It would work for ChromeOS, but will it break some other platform?
On 2014/04/08 16:33:32, oshima wrote: > > > How did you run your test? If you run it on your Ubuntu desktop, > > > your WM may interfere with the test. Can you run it with Xvfb ? > > > > It works with Xvfb. I was hoping tests do not depend on the environment. > > Sorry, it seems that my touchpad generated multiple clicks for some reason. > > We probably should set override redirect on host window to mitigate this, > although interactive_ui_tests does require isolated environment because the test > depends on the shared resources in window system (such as > focus/activation/mouse). > I'm not sure why this test is in interactive_ui_test though (> bshe?) This test used to be unittest, but wallpaper image decoding now happens in sandboxed environment, so I had to move the test to (existing) WallpaperManagerBrowserTest. My comment #4 explains why. > I'll send the review for the latest patch shortly.
On 2014/04/08 16:38:40, alemate wrote: > On 2014/04/08 16:33:32, oshima wrote: > > > > How did you run your test? If you run it on your Ubuntu desktop, > > > > your WM may interfere with the test. Can you run it with Xvfb ? > > > > > > It works with Xvfb. I was hoping tests do not depend on the environment. > > > > Sorry, it seems that my touchpad generated multiple clicks for some reason. > > > > We probably should set override redirect on host window to mitigate this, > > although interactive_ui_tests does require isolated environment because the > test > > depends on the shared resources in window system (such as > > focus/activation/mouse). > > I'm not sure why this test is in interactive_ui_test though (> bshe?) > > This test used to be unittest, but wallpaper image decoding now happens > in sandboxed environment, so I had to move the test to (existing) > WallpaperManagerBrowserTest. My comment #4 explains why. My question was why this is not in browesr_tests, instead of interactive_ui_tests. (Both tests use InProcessBrowserTest but interactive_ui_tests runs serially while browser_tests in parallel) bshe told me that this seems to run fine in browser_tests, so he'll move it in separate CL. > > > > I'll send the review for the latest patch shortly.
On 2014/04/08 16:56:58, oshima wrote: > On 2014/04/08 16:38:40, alemate wrote: > > On 2014/04/08 16:33:32, oshima wrote: > > > > > How did you run your test? If you run it on your Ubuntu desktop, > > > > > your WM may interfere with the test. Can you run it with Xvfb ? > > > > > > > > It works with Xvfb. I was hoping tests do not depend on the environment. > > > > > > Sorry, it seems that my touchpad generated multiple clicks for some reason. > > > > > > We probably should set override redirect on host window to mitigate this, > > > although interactive_ui_tests does require isolated environment because the > > test > > > depends on the shared resources in window system (such as > > > focus/activation/mouse). > > > I'm not sure why this test is in interactive_ui_test though (> bshe?) > > > > This test used to be unittest, but wallpaper image decoding now happens > > in sandboxed environment, so I had to move the test to (existing) > > WallpaperManagerBrowserTest. My comment #4 explains why. > > My question was why this is not in browesr_tests, instead of > interactive_ui_tests. > (Both tests use InProcessBrowserTest but interactive_ui_tests runs serially > while browser_tests in parallel) > > bshe told me that this seems to run fine in browser_tests, so he'll move it in > separate CL. > > > > > > > > I'll send the review for the latest patch shortly. I can't remember why exactly it is interactive_ui_tests in the first place. browser tests probably missed some dependencies when first introduced the tests. Anyway, I just tested on release and debug build locally. It looks like converting these tests from interactive_ui_tests to browser tests is fine. I will do it after your change landed.
On 2014/04/08 16:34:19, alemate wrote: > Please take another look at > ash/shell/content_client/shell_browser_main_parts.cc > > https://codereview.chromium.org/215293003/diff/130001/ash/shell/content_clien... > File ash/shell/content_client/shell_browser_main_parts.cc (right): > > https://codereview.chromium.org/215293003/diff/130001/ash/shell/content_clien... > ash/shell/content_client/shell_browser_main_parts.cc:140: > SetDefaultWallpaperDelayed(chromeos::UserManager::kSignInUser); > This breaks dependencies. > Should I remove this call completely? > It would work for ChromeOS, but will it break some other platform? Or I can copy background image creation from here: https://codereview.chromium.org/215293003/diff/160001/chrome/browser/ui/ash/u...
lgtm if you fixed the run loop issue below. https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/215293003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:256: wallpaper_manager_command_line_.get()); On 2014/04/08 13:18:49, alemate wrote: > On 2014/04/07 17:16:11, oshima wrote: > > What's the reason why we can't do this in SetUpCommandLine? > > For the old tests (i.e. not moved from unittests) we do not need > wallpaper files. So the idea was not to change the old environment. > Do you think we should have the same parameters for all tests? That's because it's using one base test class for two different type of tests. WallpaperManagerBrowserTest is getting quite big now so it's probably make sense to refactor to separate (sub) base class. (all new methods are not necessary for old tests right?) You may do it in separate CL tough. https://codereview.chromium.org/215293003/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/215293003/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:108: while (WallpaperManager::Get()->loading_.size()) { You need the RunLoop here (you can't reuse the instance) Best way is to Run the loop and Quit the loop when the loading is finish but I'm ok as this is outside of the scope of this CL.
https://codereview.chromium.org/215293003/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/215293003/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:108: while (WallpaperManager::Get()->loading_.size()) { On 2014/04/08 21:49:38, oshima wrote: > You need the RunLoop here (you can't reuse the instance) > > Best way is to Run the loop and Quit the loop when the loading is finish but I'm > ok as this is outside of the scope of this CL. Done.
The CQ bit was checked by alemate@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/215293003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
https://codereview.chromium.org/215293003/diff/180001/ash/shell/content_clien... File ash/shell/content_client/shell_browser_main_parts.cc (right): https://codereview.chromium.org/215293003/diff/180001/ash/shell/content_clien... ash/shell/content_client/shell_browser_main_parts.cc:40: #include "chrome/browser/chromeos/login/wallpaper_manager.h" ash/ should not depend on chrome/browser/ You should extend UserWallpaperDelegate API instead.
+sky@ https://codereview.chromium.org/215293003/diff/130001/ash/shell/content_clien... File ash/shell/content_client/shell_browser_main_parts.cc (left): https://codereview.chromium.org/215293003/diff/130001/ash/shell/content_clien... ash/shell/content_client/shell_browser_main_parts.cc:137: Shell::GetInstance()->desktop_background_controller()->SetDefaultWallpaper( Scott, can I completely remove this call? (you've added it back in 2012). DesktopBackgroundController will not have SetDefaultWallpaper() method, so to set default wallpaper we would need to call UserWallpaperDelegate here. It seems that wallpaper (now) will be set in Shell::Init() : https://code.google.com/p/chromium/codesearch#chromium/src/ash/shell.cc&l=983 Can I completely remove SetDefaultWallpaper here?
https://codereview.chromium.org/215293003/diff/130001/ash/shell/content_clien... File ash/shell/content_client/shell_browser_main_parts.cc (right): https://codereview.chromium.org/215293003/diff/130001/ash/shell/content_clien... ash/shell/content_client/shell_browser_main_parts.cc:140: SetDefaultWallpaperDelayed(chromeos::UserManager::kSignInUser); On 2014/04/08 16:34:20, alemate wrote: > This breaks dependencies. > Should I remove this call completely? > It would work for ChromeOS, but will it break some other platform? If unsure, try it out. We need a wallpaper to work when running ash_shell on windows too.
On 2014/04/09 16:49:35, sky wrote: > https://codereview.chromium.org/215293003/diff/130001/ash/shell/content_clien... > File ash/shell/content_client/shell_browser_main_parts.cc (right): > > https://codereview.chromium.org/215293003/diff/130001/ash/shell/content_clien... > ash/shell/content_client/shell_browser_main_parts.cc:140: > SetDefaultWallpaperDelayed(chromeos::UserManager::kSignInUser); > On 2014/04/08 16:34:20, alemate wrote: > > This breaks dependencies. > > Should I remove this call completely? > > It would work for ChromeOS, but will it break some other platform? > > If unsure, try it out. We need a wallpaper to work when running ash_shell on > windows too. For windows Wallpaper should be initialized in chrome/browser/ui/ash/user_wallpaper_delegate_win.cc InitializeWallpaper(). (It's probably incorrect now, as it sets empty wallpaper instead of resource file, but it is a different issue i think.) So I'll drop this.
The CQ bit was checked by alemate@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/215293003/200001
The CQ bit was checked by alemate@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/215293003/240001
Message was sent while issue was closed.
Change committed as 262862 |
