Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
236013002:
Apply default wallpaper from customization manifest. (Closed)
|
Created:
6 years, 8 months ago by Alexander Alekseev Modified:
6 years, 8 months ago Reviewers:
Nikita (slow), Daniel Erat, Mattias Nissler (ping if slow), Dmitry Polukhin, asargent_no_longer_on_chrome, bshe CC:
chromium-reviews, sadrul, nkostylev+watch_chromium.org, ben+ash_chromium.org, oshima+watch_chromium.org, kalyank, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionApply default wallpaper from customization manifest.
This CL also fixes bug with default wallpaper cache.
BUG=348136, 363134
TEST=manual
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265636
Patch Set 1 #
Total comments: 16
Patch Set 2 : After-review. #
Total comments: 57
Patch Set 3 : After-review. #
Total comments: 68
Patch Set 4 : Update after review. #Patch Set 5 : Update comments. #
Total comments: 14
Patch Set 6 : Implemented retry apply customization on restart. #Patch Set 7 : Comments updated. #
Total comments: 6
Patch Set 8 : Moved call to EnsureCustomizationAppliedClosure() to a better place. #
Total comments: 10
Patch Set 9 : Update after review. #Patch Set 10 : Rebased. #Patch Set 11 : Fix windows build. #Patch Set 12 : Allow customization without wallpaper. #
Total comments: 10
Patch Set 13 : Fix unittest. #Patch Set 14 : Update after review. #Patch Set 15 : Fix ExternalProviderImplTest. #Patch Set 16 : Remove wallpaper URL from ServicesCustomizationDocumentTest. #
Total comments: 1
Messages
Total messages: 37 (0 generated)
This is a second part of CL: https://codereview.chromium.org/208273005/ (A follow up to the first part: https://codereview.chromium.org/215293003 ) Please review: mnissler@: chrome/common/pref_names.* derat@,bshe@: ash/desktop_background/* dpolukhin@, nkostylev@: all
https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document.cc:76: // THis is subdirectory relative to PathService(DIR_CHROMEOS_CUSTOM_WALLPAPERS), nit, s/THis/This/ https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document.cc:629: WallpaperManager::Get()->SetCustomizedDefaultWallpaper( Instead of using WallpaperManager::Get() you can pass callback with url parameter to CustomizationWallpaperDownloader that should be called when download completed. It will also allow to make OnCustomizedWallpaperDownloaded private. https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document.cc:753: if (wallpaper_downloader_.get()) { It is the only use of wallpaper_downloader_ and bool flag should be enough. https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right): https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader.cc:64: url_fetcher_->Start(); Perhaps you need to check network condition to don't retry when there is no connectivity or under captive portal. https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader.cc:67: void CustomizationWallpaperDownloader::Retry() { We need to give up after several retries to don't overload server. Also perhaps we should remember that file doesn't exist on server to don't retry forever. https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader.cc:154: ServicesCustomizationDocument::GetInstance()->DestroyWallpaperDownloader(); I think you can just delete self if ServicesCustomizationDocument will keep WeakPtr to CustomizationWallpaperDownloader. Alternative that ServicesCustomizationDocument may not keep pointer toCustomizationWallpaperDownloader (it is not really used anyway).
Please review again. We also need to add a call to ServicesCustomizationDocument::CheckAndApplyWallpaper() somewhere to each boot, to continue wallpaper download if previously failed. Where should I add it? https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document.cc:76: // THis is subdirectory relative to PathService(DIR_CHROMEOS_CUSTOM_WALLPAPERS), On 2014/04/11 23:17:17, Dmitry Polukhin wrote: > nit, s/THis/This/ Done. https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document.cc:629: WallpaperManager::Get()->SetCustomizedDefaultWallpaper( On 2014/04/11 23:17:17, Dmitry Polukhin wrote: > Instead of using WallpaperManager::Get() you can pass callback with url > parameter to CustomizationWallpaperDownloader that should be called when > download completed. It will also allow to make OnCustomizedWallpaperDownloaded > private. Done. https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_document.cc:753: if (wallpaper_downloader_.get()) { On 2014/04/11 23:17:17, Dmitry Polukhin wrote: > It is the only use of wallpaper_downloader_ and bool flag should be enough. Now CustomizationWallpaperDownloader will be destroyed when ServicesCustomizationDocument is destroyed. This is usually important for tests (because tests destroy singletons). https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right): https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader.cc:64: url_fetcher_->Start(); On 2014/04/11 23:17:17, Dmitry Polukhin wrote: > Perhaps you need to check network condition to don't retry when there is no > connectivity or under captive portal. I can use DelayNetworkCall here, but I think it adds extra complexity here. Wallpaper download is started after successful manifest fetch, so why add another portal check? https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader.cc:67: void CustomizationWallpaperDownloader::Retry() { On 2014/04/11 23:17:17, Dmitry Polukhin wrote: > We need to give up after several retries to don't overload server. Also perhaps > we should remember that file doesn't exist on server to don't retry forever. If file doesn't exist, it usually means "temporary server error". To make the retries less expensive, the timeout is increasing here. Let's stop after 200 requests. (It is actually 2 days due to increasing timeout.). https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader.cc:154: ServicesCustomizationDocument::GetInstance()->DestroyWallpaperDownloader(); On 2014/04/11 23:17:17, Dmitry Polukhin wrote: > I think you can just delete self if ServicesCustomizationDocument will keep > WeakPtr to CustomizationWallpaperDownloader. Alternative that > ServicesCustomizationDocument may not keep pointer > toCustomizationWallpaperDownloader (it is not really used anyway). I think this would lead to problems in tests. Our tests work better with hierarchical objects, because even singletons are destroyed in tests (and in unit tests destruction happens on arbitrary threads).
pref_names.{cc,h} LGTM
please add tests for the new code https://codereview.chromium.org/236013002/diff/20001/ash/desktop_background/d... File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/236013002/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.cc:204: if (WALLPAPER_LAYOUT_UNKNOWN != layout && please put the variable first and the constant second in comparisons so the code reads like english https://codereview.chromium.org/236013002/diff/20001/ash/desktop_background/d... File ash/desktop_background/desktop_background_controller.h (right): https://codereview.chromium.org/236013002/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.h:28: // -1 either turn this comment into a sentence or delete it https://codereview.chromium.org/236013002/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.h:29: extern const int ASH_EXPORT kInvalidResourceID; this name is too generic to be at the top level of the ash namespace. make it be a public static member of DesktopBackgroundController instead. https://codereview.chromium.org/236013002/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.h:45: WALLPAPER_LAYOUT_UNKNOWN, please don't add a special-purpose value like this to the enum; it means that all of the code that uses this enum needs to handle it. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:730: if (!dir.empty() && !file.empty()) { nit: just do this so the rest of this method doesn't need to be in a block: if (dir.empty() || file.empty()) return; ... https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:751: DCHECK(wallpaper_url.is_valid() || wallpaper_url.is_empty()); do you need these is_empty checks? url/gurl.h says "Note that empty URLs are also invalid, and is_valid() will return false for them." https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_document.h (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:46: class CustomizationWallpaperDownloader; nit: alphabetize these https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:249: // Start download of wallpaper image if need. nit: s/need/needed/ https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:256: void OnCheckedWallpaperCacheExists(scoped_ptr<bool>); name this parameter https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:287: scoped_ptr<CustomizationWallpaperDownloader> wallpaper_downloader_; weak_ptr_factory_ should come last in the class definition so weak pointers will be invalidated before any other members are destroyed https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:21: const unsigned kMaxRetries = 200; where did these numbers come from? why is it meaningful to give up after 200 retries, especially if we're sleeping ~2000 seconds between retries at that point? https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:51: ++retries_; should |retries_| actually be named |attempts_|? "retry" means that the previous try failed. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:74: ServicesCustomizationDocument::GetInstance()->DestroyWallpaperDownloader(); having CustomizationWallpaperDownloader call into ServicesCustomizationDocument like this creates a circular dependency between the two classes. why not instead have SCD observe CWD and get notified when the download completes successfully or fails, and then keep all of the logic about what to do with the CWD inside of SCD? https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:104: void CustomizationWallpaperDownloader::CreateWallpaperDirectory( private static method -> should probably be function in anonymous namespace instead https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:129: !status.is_success() || (response_code >= 500 && response_code < 600); are there constants anywhere for HTTP response codes that you can use instead? https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:176: void CustomizationWallpaperDownloader::RenameTemporaryFile( private static method -> should probably be function in anonymous namespace instead https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader.h (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.h:23: // ServicesCustomizationDocument::OnCustomizedWallpaperDownloaded() in called on nit: s/in/is/ https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.h:65: scoped_refptr<net::URLRequestContextGetter> url_context_getter_; add comments to all members describing what they're used for https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.h:81: unsigned retries_; just make this be an int https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/user_image.h (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user_image.h:65: // If image was loaded from local file, file path is stored here. move this comment down to the member definition instead https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/user_image_loader.h (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user_image_loader.h:62: const int size; s/int/size_t/ (num_bytes might be a better name) https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:475: // eApply device nustomization. nit: Apply device customization. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1669: LOG(WARNING) << "Failed to save Resized Customized Default Wallpaper"; nit: "resized customized default wallpaper" (capitalizing these seems strange) https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1713: DCHECK(wallpaper_url.is_valid() || wallpaper_url.is_empty()); same comment as before about is_empty https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:93: struct CustomizedWallpaperRescaledFiles { does this really need to be public? https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:101: struct CustomizedWallpaperFilesExist { this sounds more like a method name than a struct. this should probably be a method on CustomizedWallpaperRescaledFiles, which should then be a class instead of a struct. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:361: // resized_directory - directory where resized versions are stored match the style of other comments in this file: "|resized_directory| is the directory where resized versions are stored and must be writable." https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:562: static base::FilePath GetCustomizedWallpaperDefaultRescaledFileName( please make this and ShouldUseCustomizedDefaultWallpaper() be functions in an anonymous namespace too
https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right): https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader.cc:67: void CustomizationWallpaperDownloader::Retry() { On 2014/04/12 01:42:30, alemate wrote: > On 2014/04/11 23:17:17, Dmitry Polukhin wrote: > > We need to give up after several retries to don't overload server. Also > perhaps > > we should remember that file doesn't exist on server to don't retry forever. > > If file doesn't exist, it usually means "temporary server error". > To make the retries less expensive, the timeout is increasing here. > Let's stop after 200 requests. (It is actually 2 days due to increasing > timeout.). Because you don't remember throttling info between sessions. There is very good chance that you may never have 2 days of working time to give up. We need to think about some throttling very carefully so Chrome won't keep pinging server for every user on the machine separately. Perhaps we should preserver throttling info in Local State. https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader.cc:154: ServicesCustomizationDocument::GetInstance()->DestroyWallpaperDownloader(); On 2014/04/12 01:42:30, alemate wrote: > On 2014/04/11 23:17:17, Dmitry Polukhin wrote: > > I think you can just delete self if ServicesCustomizationDocument will keep > > WeakPtr to CustomizationWallpaperDownloader. Alternative that > > ServicesCustomizationDocument may not keep pointer > > toCustomizationWallpaperDownloader (it is not really used anyway). > > I think this would lead to problems in tests. Our tests work better > with hierarchical objects, because even singletons are destroyed in tests (and > in unit tests destruction happens on arbitrary threads). Singletons shouldn't be a problem in test if you use ShadowingAtExitManager to destroy all singletons after running test. Design decision with DestroyWallpaperDownloader looks very ugly and we should fine better solution, for example WeakPointers, RefCounting or something else. DestroyWallpaperDownloader shouldn't be part of public interface of ServicesCustomizationDocument, friendship is also not a solution. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:74: ServicesCustomizationDocument::GetInstance()->DestroyWallpaperDownloader(); On 2014/04/14 15:26:15, Daniel Erat wrote: > having CustomizationWallpaperDownloader call into ServicesCustomizationDocument > like this creates a circular dependency between the two classes. why not instead > have SCD observe CWD and get notified when the download completes successfully > or fails, and then keep all of the logic about what to do with the CWD inside of > SCD? Good suggestion how to eliminate DestroyWallpaperDownloader.
Please review. https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right): https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader.cc:67: void CustomizationWallpaperDownloader::Retry() { On 2014/04/14 20:26:37, Dmitry Polukhin wrote: > On 2014/04/12 01:42:30, alemate wrote: > > On 2014/04/11 23:17:17, Dmitry Polukhin wrote: > > > We need to give up after several retries to don't overload server. Also > > perhaps > > > we should remember that file doesn't exist on server to don't retry forever. > > > > If file doesn't exist, it usually means "temporary server error". > > To make the retries less expensive, the timeout is increasing here. > > Let's stop after 200 requests. (It is actually 2 days due to increasing > > timeout.). > > Because you don't remember throttling info between sessions. There is very good > chance that you may never have 2 days of working time to give up. We need to > think about some throttling very carefully so Chrome won't keep pinging server > for every user on the machine separately. Perhaps we should preserver throttling > info in Local State. The idea was to increase delay fast enough to make remembering throttling useless. Probably I should implement even faster delay like power series. https://codereview.chromium.org/236013002/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader.cc:154: ServicesCustomizationDocument::GetInstance()->DestroyWallpaperDownloader(); On 2014/04/14 20:26:37, Dmitry Polukhin wrote: > On 2014/04/12 01:42:30, alemate wrote: > > On 2014/04/11 23:17:17, Dmitry Polukhin wrote: > > > I think you can just delete self if ServicesCustomizationDocument will keep > > > WeakPtr to CustomizationWallpaperDownloader. Alternative that > > > ServicesCustomizationDocument may not keep pointer > > > toCustomizationWallpaperDownloader (it is not really used anyway). > > > > I think this would lead to problems in tests. Our tests work better > > with hierarchical objects, because even singletons are destroyed in tests (and > > in unit tests destruction happens on arbitrary threads). > > Singletons shouldn't be a problem in test if you use ShadowingAtExitManager to > destroy all singletons after running test. Design decision with > DestroyWallpaperDownloader looks very ugly and we should fine better solution, > for example WeakPointers, RefCounting or something else. > DestroyWallpaperDownloader shouldn't be part of public interface of > ServicesCustomizationDocument, friendship is also not a solution. I've dropped DestroyWallpaperDownloader method. https://codereview.chromium.org/236013002/diff/20001/ash/desktop_background/d... File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/236013002/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.cc:204: if (WALLPAPER_LAYOUT_UNKNOWN != layout && On 2014/04/14 15:26:15, Daniel Erat wrote: > please put the variable first and the constant second in comparisons so the code > reads like english Done. https://codereview.chromium.org/236013002/diff/20001/ash/desktop_background/d... File ash/desktop_background/desktop_background_controller.h (right): https://codereview.chromium.org/236013002/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.h:28: // -1 On 2014/04/14 15:26:15, Daniel Erat wrote: > either turn this comment into a sentence or delete it Done. https://codereview.chromium.org/236013002/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.h:29: extern const int ASH_EXPORT kInvalidResourceID; On 2014/04/14 15:26:15, Daniel Erat wrote: > this name is too generic to be at the top level of the ash namespace. make it be > a public static member of DesktopBackgroundController instead. Done. https://codereview.chromium.org/236013002/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.h:45: WALLPAPER_LAYOUT_UNKNOWN, On 2014/04/14 15:26:15, Daniel Erat wrote: > please don't add a special-purpose value like this to the enum; it means that > all of the code that uses this enum needs to handle it. Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:730: if (!dir.empty() && !file.empty()) { On 2014/04/14 15:26:15, Daniel Erat wrote: > nit: just do this so the rest of this method doesn't need to be in a block: > > if (dir.empty() || file.empty()) > return; > > ... Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:751: DCHECK(wallpaper_url.is_valid() || wallpaper_url.is_empty()); On 2014/04/14 15:26:15, Daniel Erat wrote: > do you need these is_empty checks? url/gurl.h says "Note that empty URLs are > also invalid, and is_valid() will return false for them." Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_document.h (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:46: class CustomizationWallpaperDownloader; On 2014/04/14 15:26:15, Daniel Erat wrote: > nit: alphabetize these Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:249: // Start download of wallpaper image if need. On 2014/04/14 15:26:15, Daniel Erat wrote: > nit: s/need/needed/ Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:256: void OnCheckedWallpaperCacheExists(scoped_ptr<bool>); On 2014/04/14 15:26:15, Daniel Erat wrote: > name this parameter Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:287: scoped_ptr<CustomizationWallpaperDownloader> wallpaper_downloader_; On 2014/04/14 15:26:15, Daniel Erat wrote: > weak_ptr_factory_ should come last in the class definition so weak pointers will > be invalidated before any other members are destroyed Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:21: const unsigned kMaxRetries = 200; On 2014/04/14 15:26:15, Daniel Erat wrote: > where did these numbers come from? why is it meaningful to give up after 200 > retries, especially if we're sleeping ~2000 seconds between retries at that > point? The original idea was to "never give up". 200 retries means 2 days. I don't have better ideas though. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:51: ++retries_; On 2014/04/14 15:26:15, Daniel Erat wrote: > should |retries_| actually be named |attempts_|? "retry" means that the previous > try failed. Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:74: ServicesCustomizationDocument::GetInstance()->DestroyWallpaperDownloader(); On 2014/04/14 20:26:37, Dmitry Polukhin wrote: > On 2014/04/14 15:26:15, Daniel Erat wrote: > > having CustomizationWallpaperDownloader call into > ServicesCustomizationDocument > > like this creates a circular dependency between the two classes. why not > instead > > have SCD observe CWD and get notified when the download completes successfully > > or fails, and then keep all of the logic about what to do with the CWD inside > of > > SCD? > > Good suggestion how to eliminate DestroyWallpaperDownloader. Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:104: void CustomizationWallpaperDownloader::CreateWallpaperDirectory( On 2014/04/14 15:26:15, Daniel Erat wrote: > private static method -> should probably be function in anonymous namespace > instead Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:129: !status.is_success() || (response_code >= 500 && response_code < 600); On 2014/04/14 15:26:15, Daniel Erat wrote: > are there constants anywhere for HTTP response codes that you can use instead? I've replaced this with constants, but I don't know if it actually became more readable. Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:176: void CustomizationWallpaperDownloader::RenameTemporaryFile( On 2014/04/14 15:26:15, Daniel Erat wrote: > private static method -> should probably be function in anonymous namespace > instead Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader.h (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.h:23: // ServicesCustomizationDocument::OnCustomizedWallpaperDownloaded() in called on On 2014/04/14 15:26:15, Daniel Erat wrote: > nit: s/in/is/ Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.h:65: scoped_refptr<net::URLRequestContextGetter> url_context_getter_; On 2014/04/14 15:26:15, Daniel Erat wrote: > add comments to all members describing what they're used for Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.h:81: unsigned retries_; On 2014/04/14 15:26:15, Daniel Erat wrote: > just make this be an int Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/user_image.h (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user_image.h:65: // If image was loaded from local file, file path is stored here. On 2014/04/14 15:26:15, Daniel Erat wrote: > move this comment down to the member definition instead Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/user_image_loader.h (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user_image_loader.h:62: const int size; On 2014/04/14 15:26:15, Daniel Erat wrote: > s/int/size_t/ > > (num_bytes might be a better name) |size| is image size in pixels and may be negative (see comment above). https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:475: // eApply device nustomization. On 2014/04/14 15:26:15, Daniel Erat wrote: > nit: Apply device customization. Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1669: LOG(WARNING) << "Failed to save Resized Customized Default Wallpaper"; On 2014/04/14 15:26:15, Daniel Erat wrote: > nit: "resized customized default wallpaper" (capitalizing these seems strange) Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1713: DCHECK(wallpaper_url.is_valid() || wallpaper_url.is_empty()); On 2014/04/14 15:26:15, Daniel Erat wrote: > same comment as before about is_empty Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:93: struct CustomizedWallpaperRescaledFiles { On 2014/04/14 15:26:15, Daniel Erat wrote: > does this really need to be public? Probably yes. Otherwize functions in anonymous namespace cannot use it. I've moved it to .cc file. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:101: struct CustomizedWallpaperFilesExist { On 2014/04/14 15:26:15, Daniel Erat wrote: > this sounds more like a method name than a struct. this should probably be a > method on CustomizedWallpaperRescaledFiles, which should then be a class instead > of a struct. Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:361: // resized_directory - directory where resized versions are stored On 2014/04/14 15:26:15, Daniel Erat wrote: > match the style of other comments in this file: "|resized_directory| is the > directory where resized versions are stored and must be writable." Done. https://codereview.chromium.org/236013002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:562: static base::FilePath GetCustomizedWallpaperDefaultRescaledFileName( On 2014/04/14 15:26:15, Daniel Erat wrote: > please make this and ShouldUseCustomizedDefaultWallpaper() be functions in an > anonymous namespace too Done.
as mentioned before, please write tests for CustomizationWallpaperDownloader https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:81: const char kCustomizationDefaultWallpaperDownloadedFile[] = nit: put "Pref" on the end of this constant's name, assuming it's a pref (a shorter name like kDownloadedWallpaperFilePref would also be fine, since this constant looks like it's local to the file) https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:604: DCHECK(success); nit: this could be simplified a bit as: if (!PathService::Get(...)) { LOG(DFATAL) << "Unable to get custom wallpaper dir"; return base::FilePath(); } return custom_wallpaper_dir.Append(...); https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:750: if (!wallpaper_url.is_empty()) { nit: omit curly brackets here since the statement fits on a single line https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:751: LOG(WARNING) << "Invalid Customized Wallpaper URL."; include wallpaper_url.spec() in this message to aid in debugging https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:814: DCHECK(wallpaper_url.is_valid()); everything in this block needs to be indented two more spaces https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:23: const int kMaxRetries = 200; why don't you make it retry forever and add a constant to limit the maximum amount of time to sleep between retries? const int kMaxRetryDelay = 3600; https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/user_image_loader.h (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user_image_loader.h:57: int size, how about renaming this to something that's clearer like |dimension| or |pixels_per_side|? https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:63: const base::FilePath path_downloaded; these should all be private members with underscores on the ends of their names and accessors now that this is a class instead of a struct https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:150: const char* suffix) { any reason this can't take const std::string& ? https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1742: if (!wallpaper_url.is_empty()) { nit: omit curly brackets here https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:411: // wallpaper to the loaded wallpaper. you accidentally deleted the line above this one, i think
https://codereview.chromium.org/236013002/diff/40001/ash/desktop_background/d... File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/236013002/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.cc:103: if (WallpaperIsAlreadyLoaded(&image, kInvalidResourceID, true, layout)) { nit: /* compare layouts */ https://codereview.chromium.org/236013002/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.cc:124: if (WallpaperIsAlreadyLoaded(NULL, resource_id, true, layout)) { nit: /* compare layouts */ https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:82: "default.downloaded"; nit: should file name include extension for clarity? What does document say - is it always a JPG? https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:392: registry->RegisterStringPref(prefs::kCustomizationDefaultWallpaperURL, ""); std::string() https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:600: base::FilePath ServicesCustomizationDocument::GetCustomizedWallpaperCacheDir() { nit: Please group static methods together, i.e. move to line 415 https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:617: return dir; nit: NOTREACHED() https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:727: return; NOTREACHED() https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:747: // Should fail if this ever happens in tests. nit: Insert empty line before comment. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_document.h (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:183: // This is also used by WallpaperManager to set customized wallpaper path Comments needs to be updated. It says that these methods are used to set while these methods are Get*() https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:249: // Check downloaded wallpaper and start download if needed. nit: >> and start download if needed start applying? or it does redownload as well? https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:250: void CheckAndApplyWallpaper(); nit: What does "check" mean in this context? https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:261: void OnCustomizedWallpaperDownloaded(bool success, const GURL& wallpaper_url); nit: Rename to OnOEMWallpaperDownloaded() to match StartOEMWallpaperDownload() https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:17: const char kTemporarySuffix[] = ".temp"; nit: .tmp? // To reflect what comments are saying. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:30: *success = CreateDirectoryAndGetError(wallpaper_dir, NULL); NOTREACHED() if unable to create directory. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:83: url_fetcher_->SetLoadFlags(net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE | nit: One flag per line. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/user_image.h (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user_image.h:75: // If image was loaded from local file, file path is stored here. nit: Insert empty line before comment. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:655: ServicesCustomizationDocument::GetInstance()->StartFetching(); Is this the only place where StartFetching() is called? If OEM wallpaper download fails here for network connectivity reasons, when it will retry?
https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:61: bool AllRescaledExist() const; nit: Rename to AllSizesExist() https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:161: // If DesktopBackgroundController should start with customized default nit: if > whether https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1611: // Need rescale nit: dot at the end otherwise comment looks like it is unfinished. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1829: const bool need_reload = nit: Insert empty line before comment. Can you please expand comment and mention why need_reload will be false if wallpaper is not already loaded? Does it mean that this call will eventually happen?
https://codereview.chromium.org/236013002/diff/40001/ash/desktop_background/d... File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/236013002/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.cc:103: if (WallpaperIsAlreadyLoaded(&image, kInvalidResourceID, true, layout)) { On 2014/04/15 12:39:24, Nikita Kostylev wrote: > nit: /* compare layouts */ Done. https://codereview.chromium.org/236013002/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_controller.cc:124: if (WallpaperIsAlreadyLoaded(NULL, resource_id, true, layout)) { On 2014/04/15 12:39:24, Nikita Kostylev wrote: > nit: /* compare layouts */ Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:81: const char kCustomizationDefaultWallpaperDownloadedFile[] = On 2014/04/15 03:06:30, Daniel Erat wrote: > nit: put "Pref" on the end of this constant's name, assuming it's a pref > > (a shorter name like kDownloadedWallpaperFilePref would also be fine, since this > constant looks like it's local to the file) This is not a preference. This is exactly file name: localhost ~ # ls -l /home/chronos/custom_wallpapers/customization/ total 2788 -rw-r--r-- 1 chronos chronos 1404517 Apr 15 05:38 default.downloaded -rw-r--r-- 1 chronos chronos 1129849 Apr 15 05:38 default.downloaded_large -rw-r--r-- 1 chronos chronos 318315 Apr 15 05:38 default.downloaded_small localhost ~ # I've dropped file type extensions because customization desgn document says nothing about file format. So any supported image format would work. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:82: "default.downloaded"; On 2014/04/15 12:39:24, Nikita Kostylev wrote: > nit: should file name include extension for clarity? > What does document say - is it always a JPG? It says nothing. Current implementation will allow any supported image format. That is why I've dropped extensions here. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:392: registry->RegisterStringPref(prefs::kCustomizationDefaultWallpaperURL, ""); On 2014/04/15 12:39:24, Nikita Kostylev wrote: > std::string() Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:600: base::FilePath ServicesCustomizationDocument::GetCustomizedWallpaperCacheDir() { On 2014/04/15 12:39:24, Nikita Kostylev wrote: > nit: Please group static methods together, i.e. move to line 415 Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:604: DCHECK(success); On 2014/04/15 03:06:30, Daniel Erat wrote: > nit: this could be simplified a bit as: > > if (!PathService::Get(...)) { > LOG(DFATAL) << "Unable to get custom wallpaper dir"; > return base::FilePath(); > } > return custom_wallpaper_dir.Append(...); Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:617: return dir; On 2014/04/15 12:39:24, Nikita Kostylev wrote: > nit: NOTREACHED() Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:727: return; On 2014/04/15 12:39:24, Nikita Kostylev wrote: > NOTREACHED() Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:747: // Should fail if this ever happens in tests. On 2014/04/15 12:39:24, Nikita Kostylev wrote: > nit: Insert empty line before comment. Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:750: if (!wallpaper_url.is_empty()) { On 2014/04/15 03:06:30, Daniel Erat wrote: > nit: omit curly brackets here since the statement fits on a single line Now it occupies two lines ;-) https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:751: LOG(WARNING) << "Invalid Customized Wallpaper URL."; On 2014/04/15 03:06:30, Daniel Erat wrote: > include wallpaper_url.spec() in this message to aid in debugging Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:814: DCHECK(wallpaper_url.is_valid()); On 2014/04/15 03:06:30, Daniel Erat wrote: > everything in this block needs to be indented two more spaces Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_document.h (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:183: // This is also used by WallpaperManager to set customized wallpaper path On 2014/04/15 12:39:24, Nikita Kostylev wrote: > Comments needs to be updated. It says that these methods are used to set while > these methods are Get*() Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:249: // Check downloaded wallpaper and start download if needed. On 2014/04/15 12:39:24, Nikita Kostylev wrote: > nit: > >> and start download if needed > > start applying? or it does redownload as well? If we do not have final two rescaled wallpaper files matching wallpaper URL from customization manifest, wallpaper will be downloaded again. There is no restart "in the middle of the process". https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:250: void CheckAndApplyWallpaper(); On 2014/04/15 12:39:24, Nikita Kostylev wrote: > nit: What does "check" mean in this context? Check that current customized wallpaper cache exists and matches manifest. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:261: void OnCustomizedWallpaperDownloaded(bool success, const GURL& wallpaper_url); On 2014/04/15 12:39:24, Nikita Kostylev wrote: > nit: Rename to OnOEMWallpaperDownloaded() to match > StartOEMWallpaperDownload() Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:17: const char kTemporarySuffix[] = ".temp"; On 2014/04/15 12:39:24, Nikita Kostylev wrote: > nit: .tmp? // To reflect what comments are saying. Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:23: const int kMaxRetries = 200; On 2014/04/15 03:06:30, Daniel Erat wrote: > why don't you make it retry forever and add a constant to limit the maximum > amount of time to sleep between retries? > > const int kMaxRetryDelay = 3600; Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:30: *success = CreateDirectoryAndGetError(wallpaper_dir, NULL); On 2014/04/15 12:39:24, Nikita Kostylev wrote: > NOTREACHED() if unable to create directory. Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:83: url_fetcher_->SetLoadFlags(net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE | On 2014/04/15 12:39:24, Nikita Kostylev wrote: > nit: One flag per line. Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/user_image.h (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user_image.h:75: // If image was loaded from local file, file path is stored here. On 2014/04/15 12:39:24, Nikita Kostylev wrote: > nit: Insert empty line before comment. Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/user_image_loader.h (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user_image_loader.h:57: int size, On 2014/04/15 03:06:30, Daniel Erat wrote: > how about renaming this to something that's clearer like |dimension| or > |pixels_per_side|? Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:61: bool AllRescaledExist() const; On 2014/04/15 13:40:00, Nikita Kostylev wrote: > nit: Rename to AllSizesExist() Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:63: const base::FilePath path_downloaded; On 2014/04/15 03:06:30, Daniel Erat wrote: > these should all be private members with underscores on the ends of their names > and accessors now that this is a class instead of a struct Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:150: const char* suffix) { On 2014/04/15 03:06:30, Daniel Erat wrote: > any reason this can't take const std::string& ? Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:161: // If DesktopBackgroundController should start with customized default On 2014/04/15 13:40:00, Nikita Kostylev wrote: > nit: if > whether Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1611: // Need rescale On 2014/04/15 13:40:00, Nikita Kostylev wrote: > nit: dot at the end otherwise comment looks like it is unfinished. Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1742: if (!wallpaper_url.is_empty()) { On 2014/04/15 03:06:30, Daniel Erat wrote: > nit: omit curly brackets here Now it occupies 2 lines. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1829: const bool need_reload = On 2014/04/15 13:40:00, Nikita Kostylev wrote: > nit: Insert empty line before comment. > > Can you please expand comment and mention why need_reload will be false if > wallpaper is not already loaded? Does it mean that this call will eventually > happen? Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.h:411: // wallpaper to the loaded wallpaper. On 2014/04/15 03:06:30, Daniel Erat wrote: > you accidentally deleted the line above this one, i think Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:655: ServicesCustomizationDocument::GetInstance()->StartFetching(); On 2014/04/15 12:39:24, Nikita Kostylev wrote: > Is this the only place where StartFetching() is called? > > If OEM wallpaper download fails here for network connectivity reasons, when it > will retry? As far as I understand, customization will be additionally fetched for each new user on device, so customization will be fetched this of other way. The problem is that ServicesCustomizationDocument::GetInstance()->CheckAndApplyWallpaper() should be called on each boot and I didn't find a proper place to insert this call.
https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:655: ServicesCustomizationDocument::GetInstance()->StartFetching(); On 2014/04/15 22:47:31, alemate wrote: > On 2014/04/15 12:39:24, Nikita Kostylev wrote: > > Is this the only place where StartFetching() is called? > > > > If OEM wallpaper download fails here for network connectivity reasons, when it > > will retry? > > As far as I understand, customization will be additionally fetched for each new > user on device, so customization will be fetched this of other way. > > The problem is that > ServicesCustomizationDocument::GetInstance()->CheckAndApplyWallpaper() > should be called on each boot and I didn't find a proper place to > insert this call. I think it should be somewhere near ShowLoginWizard. But please make sure that manifest is not fetched before ELUA accepted.
https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:82: "default.downloaded"; having a period in the middle of the filename seems strange, and it'd also be good to have a name that explains that this is wallpaper and that it's a binary file. how about something like "default_downloaded_wallpaper.bin"? https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right): https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:23: // longer than maximum (kMaxRetryDelaySeconds) it is set to the maximim. nit: s/maximim/maximum/ https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:24: const int kMaxRetryDelaySeconds = 6 * 3600; // 6 hours nit: be consistent wrt "Sleep" and "Delay" between this constant and the above one https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:33: NOTREACHED() << "Failed to creare directory '" << wallpaper_dir.value() nit: s/creare/create/ https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:105: retries_ * double(retries_) * kRetrySleepSeconds >= kMaxRetryDelaySeconds; few things: - use static_cast instead - i think i've convinced myself that this is safe (i.e. it'll become infinity instead of overflowing), but please add a comment explaining this - something like this is simpler and equivalent, i think (you'll need to change the constant to a double): const double delay_seconds = std::min(kMaxRetryDelaySeconds, static_cast<double>(retries_) * retries_ * kRetrySleepSeconds); const base::TimeDelta delay = base::TimeDelta::FromSeconds( lround(delay_seconds)); https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:108: ignore_retries ? base::TimeDelta::FromSeconds(kRetrySleepSeconds) you'd need kMaxRetryDelaySeconds here, but see above https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader.h (right): https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.h:25: class CustomizationWallpaperDownloader : public net::URLFetcherDelegate { i haven't seen a reply to this yet; maybe you missed it: please add tests for this class.
Please add tests, I'll do another look then. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:655: ServicesCustomizationDocument::GetInstance()->StartFetching(); On 2014/04/15 22:47:31, alemate wrote: > On 2014/04/15 12:39:24, Nikita Kostylev wrote: > > Is this the only place where StartFetching() is called? > > > > If OEM wallpaper download fails here for network connectivity reasons, when it > > will retry? > > As far as I understand, customization will be additionally fetched for each new > user on device, so customization will be fetched this of other way. > > The problem is that > ServicesCustomizationDocument::GetInstance()->CheckAndApplyWallpaper() > should be called on each boot and I didn't find a proper place to > insert this call. Please make sure that customization document header comment includes this details i.e. when oem wallpaper fetch happens and when it will be retried. Same goes about comparing existing URL with new one if it is changed. Do we actually support now refetching manifest or not?
https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:655: ServicesCustomizationDocument::GetInstance()->StartFetching(); On 2014/04/15 22:57:06, Dmitry Polukhin wrote: > On 2014/04/15 22:47:31, alemate wrote: > > On 2014/04/15 12:39:24, Nikita Kostylev wrote: > > > Is this the only place where StartFetching() is called? > > > > > > If OEM wallpaper download fails here for network connectivity reasons, when > it > > > will retry? > > > > As far as I understand, customization will be additionally fetched for each > new > > user on device, so customization will be fetched this of other way. > > > > The problem is that > > ServicesCustomizationDocument::GetInstance()->CheckAndApplyWallpaper() > > should be called on each boot and I didn't find a proper place to > > insert this call. > > I think it should be somewhere near ShowLoginWizard. But please make sure that > manifest is not fetched before ELUA accepted. Done. https://codereview.chromium.org/236013002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:655: ServicesCustomizationDocument::GetInstance()->StartFetching(); On 2014/04/16 18:16:21, Nikita Kostylev wrote: > On 2014/04/15 22:47:31, alemate wrote: > > On 2014/04/15 12:39:24, Nikita Kostylev wrote: > > > Is this the only place where StartFetching() is called? > > > > > > If OEM wallpaper download fails here for network connectivity reasons, when > it > > > will retry? > > > > As far as I understand, customization will be additionally fetched for each > new > > user on device, so customization will be fetched this of other way. > > > > The problem is that > > ServicesCustomizationDocument::GetInstance()->CheckAndApplyWallpaper() > > should be called on each boot and I didn't find a proper place to > > insert this call. > > Please make sure that customization document header comment includes this > details i.e. when oem wallpaper fetch happens and when it will be retried. > > Same goes about comparing existing URL with new one if it is changed. > Do we actually support now refetching manifest or not? There is a comment on no-refetch policy here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... I've added another comment on this to the header file. https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.cc:82: "default.downloaded"; On 2014/04/16 15:35:41, Daniel Erat wrote: > having a period in the middle of the filename seems strange, and it'd also be > good to have a name that explains that this is wallpaper and that it's a binary > file. how about something like "default_downloaded_wallpaper.bin"? Done. https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right): https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:23: // longer than maximum (kMaxRetryDelaySeconds) it is set to the maximim. On 2014/04/16 15:35:41, Daniel Erat wrote: > nit: s/maximim/maximum/ Done. https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:24: const int kMaxRetryDelaySeconds = 6 * 3600; // 6 hours On 2014/04/16 15:35:41, Daniel Erat wrote: > nit: be consistent wrt "Sleep" and "Delay" between this constant and the above > one Done. https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:33: NOTREACHED() << "Failed to creare directory '" << wallpaper_dir.value() On 2014/04/16 15:35:41, Daniel Erat wrote: > nit: s/creare/create/ Done. https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:105: retries_ * double(retries_) * kRetrySleepSeconds >= kMaxRetryDelaySeconds; On 2014/04/16 15:35:41, Daniel Erat wrote: > few things: > - use static_cast instead > - i think i've convinced myself that this is safe (i.e. it'll become infinity > instead of overflowing), but please add a comment explaining this > - something like this is simpler and equivalent, i think (you'll need to change > the constant to a double): > > const double delay_seconds = std::min(kMaxRetryDelaySeconds, > static_cast<double>(retries_) * retries_ * kRetrySleepSeconds); > const base::TimeDelta delay = base::TimeDelta::FromSeconds( > lround(delay_seconds)); Done. https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.cc:108: ignore_retries ? base::TimeDelta::FromSeconds(kRetrySleepSeconds) On 2014/04/16 15:35:41, Daniel Erat wrote: > you'd need kMaxRetryDelaySeconds here, but see above Done. https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader.h (right): https://codereview.chromium.org/236013002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader.h:25: class CustomizationWallpaperDownloader : public net::URLFetcherDelegate { On 2014/04/16 15:35:41, Daniel Erat wrote: > i haven't seen a reply to this yet; maybe you missed it: please add tests for > this class. I'll add tests in another CL. This one is already large.
lgtm https://codereview.chromium.org/236013002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/236013002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.cc:372: bool engaged_; nit: Add comment for engaged_. https://codereview.chromium.org/236013002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document.h (right): https://codereview.chromium.org/236013002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.h:262: // downloaded, it's never updated (even if manifest is re-fetched). nit: Even if wallpaper URL from the downloaded manifest is different that was in a previous version of manifest? https://codereview.chromium.org/236013002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/login_display_host_impl.cc (right): https://codereview.chromium.org/236013002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/login_display_host_impl.cc:1214: if (StartupUtils::IsEulaAccepted()) Just FYI, this call will only be called in one case: 1. user goes through out-of-box 2. there's a blocking update 3. restart after update happens 4. since out-of-box is not ended, this branch will be executed.
On 2014/04/16 23:19:58, alemate wrote: > I'll add tests in another CL. This one is already large. Ok, let's do that in a separate CL. Otherwise it is hard to follow (and review) in this one.
https://codereview.chromium.org/236013002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/236013002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.cc:372: bool engaged_; On 2014/04/17 05:23:34, Nikita Kostylev wrote: > nit: Add comment for engaged_. Done. https://codereview.chromium.org/236013002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document.h (right): https://codereview.chromium.org/236013002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.h:262: // downloaded, it's never updated (even if manifest is re-fetched). On 2014/04/17 05:23:34, Nikita Kostylev wrote: > nit: Even if wallpaper URL from the downloaded manifest is different that was in > a previous version of manifest? Yes. There is a warning message on that. https://codereview.chromium.org/236013002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/login_display_host_impl.cc (right): https://codereview.chromium.org/236013002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/login_display_host_impl.cc:1214: if (StartupUtils::IsEulaAccepted()) On 2014/04/17 05:23:34, Nikita Kostylev wrote: > Just FYI, this call will only be called in one case: > > 1. user goes through out-of-box > 2. there's a blocking update > 3. restart after update happens > 4. since out-of-box is not ended, this branch will be executed. Thank you. I should move this call before if (show_login_screen) {...} .
LGTM but please rebase and run trybots. Also please add tests in next CL.
lgtm after some more comments are addressed, although i'd strongly prefer that you added tests as part of this change, to make sure both that this code works correctly and that the new class is structured in such a way that it's testable. https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.cc:362: ApplyingTask(ServicesCustomizationDocument* document); add explicit here https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.cc:912: DCHECK(apply_tasks_started_ > apply_tasks_finished_); nit: use DCHECK_GT instead so both values will be included in the log message https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader.h (right): https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader.h:16: #include "chrome/browser/chromeos/customization_document.h" remove this include https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader.h:23: // ServicesCustomizationDocument::OnCustomizedWallpaperDownloaded() is called on update this comment https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:202: }; add DISALLOW_COPY_AND_ASSIGN
Ok. I'll rebase it now, check and commit, as it probably stops other CLs to WM / Customization. I'll create tests in the other CL. https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.cc:362: ApplyingTask(ServicesCustomizationDocument* document); On 2014/04/17 16:24:22, Daniel Erat wrote: > add explicit here Done. https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.cc:912: DCHECK(apply_tasks_started_ > apply_tasks_finished_); On 2014/04/17 16:24:22, Daniel Erat wrote: > nit: use DCHECK_GT instead so both values will be included in the log message Done. https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader.h (right): https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader.h:16: #include "chrome/browser/chromeos/customization_document.h" On 2014/04/17 16:24:22, Daniel Erat wrote: > remove this include Done. https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader.h:23: // ServicesCustomizationDocument::OnCustomizedWallpaperDownloaded() is called on On 2014/04/17 16:24:22, Daniel Erat wrote: > update this comment Done. https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/236013002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:202: }; On 2014/04/17 16:24:22, Daniel Erat wrote: > add DISALLOW_COPY_AND_ASSIGN Done.
bshe@, do you have any more comments on this CL?
On 2014/04/17 15:44:30, Dmitry Polukhin wrote: > LGTM but please rebase and run trybots. Also please add tests in next CL. PTAL at Patch Set 12. (It adds support for customization manifests without wallpaper). BTW: http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds... shows that OnManifestLoaded() is called when server returned 404 : @@@STEP_LOG_LINE@CustomizationManifestNotFound@ServicesCustomizationDocumentT... (run #3):@@@ Is it expected behavior?
On 2014/04/19 02:51:47, alemate wrote: > On 2014/04/17 15:44:30, Dmitry Polukhin wrote: > > LGTM but please rebase and run trybots. Also please add tests in next CL. > > PTAL at Patch Set 12. (It adds support for customization manifests without > wallpaper). > > BTW: > http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds... > shows that OnManifestLoaded() is called when server returned 404 : > > mailto:@@@STEP_LOG_LINE@CustomizationManifestNotFound@ServicesCustomizationDocumentTest.CustomizationManifestNotFound > (run #3):@@@ > > Is it expected behavior? Yes, OnManifestLoaded is called with empty customization manifest that is used when no customization available. See what ServicesCustomizationDocument::OnCustomizationNotFound does.
lgtm https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.cc:182: // recursively so we need to return to don't call LoadFinished twice. nit: s/don't call/avoid/calling/ (not from this change) https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.cc:522: // There is no customization ID in VPD remember that. nit: turn this into a sentence ("remember that"?) (not from this change) https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document.h (right): https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.h:171: // "out_url" is set to attribute value. nit: s/"out_url"/|out_url|/ https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document_unittest.cc (right): https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document_unittest.cc:328: EXPECT_EQ(wallpaper_url.spec(), "http://somedomain.com/image.png"); EXPECT_EQ()'s arguments should be (expected, actual), not (actual, expected). otherwise the failure messages that it prints are backwards. please update this call (and also the rest of the file, although that can happen in another change). https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:1095: extensions::api::wallpaper_private::OnRequestEnableSurpriseMe::kEventName, i don't think that this is from your change (i'm just reviewing the between-patch-sets diffs), but this indenting is weird...
+asargent@ : please review chrome/browser/extensions/external_provider_impl_chromeos_unittest.cc https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.cc:182: // recursively so we need to return to don't call LoadFinished twice. On 2014/04/19 14:24:08, Daniel Erat wrote: > nit: s/don't call/avoid/calling/ > > (not from this change) Done. https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.cc:522: // There is no customization ID in VPD remember that. On 2014/04/19 14:24:08, Daniel Erat wrote: > nit: turn this into a sentence ("remember that"?) > > (not from this change) Done. https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document.h (right): https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.h:171: // "out_url" is set to attribute value. On 2014/04/19 14:24:08, Daniel Erat wrote: > nit: s/"out_url"/|out_url|/ Done. https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document_unittest.cc (right): https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document_unittest.cc:328: EXPECT_EQ(wallpaper_url.spec(), "http://somedomain.com/image.png"); On 2014/04/19 14:24:08, Daniel Erat wrote: > EXPECT_EQ()'s arguments should be (expected, actual), not (actual, expected). > otherwise the failure messages that it prints are backwards. > > please update this call (and also the rest of the file, although that can happen > in another change). Done. https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/236013002/diff/170001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:1095: extensions::api::wallpaper_private::OnRequestEnableSurpriseMe::kEventName, On 2014/04/19 14:24:08, Daniel Erat wrote: > i don't think that this is from your change (i'm just reviewing the > between-patch-sets diffs), but this indenting is weird... Done.
chrome/browser/extensions lgtm
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/236013002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
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/236013002/240001
Message was sent while issue was closed.
Change committed as 265636
Message was sent while issue was closed.
https://codereview.chromium.org/236013002/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document_unittest.cc (left): https://codereview.chromium.org/236013002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document_unittest.cc:79: " \"default_wallpaper\": \"http://somedomain.com/image.png\",\n" Why did you do this instead of adding test? Moreover it happened after I gave LGTM.
Message was sent while issue was closed.
On 2014/04/23 20:24:04, Dmitry Polukhin wrote: > https://codereview.chromium.org/236013002/diff/240001/chrome/browser/chromeos... > File chrome/browser/chromeos/customization_document_unittest.cc (left): > > https://codereview.chromium.org/236013002/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/customization_document_unittest.cc:79: " > \"default_wallpaper\": \"http://somedomain.com/image.png\",\n" > Why did you do this instead of adding test? Moreover it happened after I gave > LGTM. The idea was to add tests in another CL. So here I have only fixed failures in existing tests. Yes, It seems that it was a bad idea to modify this test after your note, but I'll add test on that in another CL.
Message was sent while issue was closed.
On 2014/04/24 14:47:25, alemate wrote: > The idea was to add tests in another CL. So here I have only fixed failures in > existing tests. > > Yes, It seems that it was a bad idea to modify this test after your note, but > I'll add test on that in another CL. OK, please add new test. But you closed Issue 348136. In future please keep issue open until you add required test or open new issue for the test only. |
