Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
253833006:
Add browser test for CustomizationWallpaperDownloader. (Closed)
|
Created:
6 years, 7 months ago by Alexander Alekseev Modified:
6 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd browser test for CustomizationWallpaperDownloader.
Also implements TestURLFetcher::SaveResponseToTemporaryFile(),
and explicitly allows ScopedIO in TestURLFetcher, as it doesn't support
asynchronous IO by design.
Note to tree sheriffs: this CL can break tests using FakeURLFetcherFactory.
BUG=366614
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269907
Patch Set 1 #
Total comments: 36
Patch Set 2 : Rebased. #
Total comments: 4
Patch Set 3 : Update after review. #
Total comments: 5
Patch Set 4 : Move utils to namespace. #
Total comments: 33
Patch Set 5 : Update after review. #Patch Set 6 : Update after review. #Patch Set 7 : Implemented set_retry_delay_for_testing. #
Total comments: 10
Patch Set 8 : Update after review. #
Total comments: 8
Patch Set 9 : Rebased. #Patch Set 10 : Fixed WallpaperManagerBrowserTest. #Patch Set 11 : Update after review. #Patch Set 12 : Add get_retry_current_delay_for_testing. #
Total comments: 4
Patch Set 13 : Rebased. #Patch Set 14 : Update after review. #
Total comments: 2
Messages
Total messages: 37 (0 generated)
Please review: maruel@ : PRESUBMIT.py mmenke@ : net/url_request/test_url_fetcher_factory.cc PRESUBMIT.py derat@, dpolukhin@, nkostylev@ : all
Very first question before reviewing the code itself do you really need browsertest? Why unittest is not enough?
On 2014/04/29 00:03:34, Dmitry Polukhin wrote: > Very first question before reviewing the code itself do you really need > browsertest? Why unittest is not enough? Because wallpaper decoding is done in sandboxed environment.
presubmit.py rubberstamp lgtm
https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... File chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:5: #include "chrome/browser/chromeos/customization_wallpaper_downloader.h" nit: This include should go with the others. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:92: class TestObserver : public WallpaperManager::Observer { Please move to the unnamed namespace. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:92: class TestObserver : public WallpaperManager::Observer { nit: TestWallpaperObserver. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:110: while (!finished_) { nit: drop {} https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:146: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100)); Potential source of flakiness. Please eliminate sleep approach. This seems like a workaround that will break on some platform/configuration etc. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:181: // Creates a test image of given size nit: dot at the end. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:182: gfx::ImageSkia CreateTestImage(int width, int height, SkColor color) { Can be moved to unnamed namespace? https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:192: bool WriteJPEGFile(const base::FilePath& path, Can be moved to unnamed namespace? https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:270: bool ImageIsNearColor(gfx::ImageSkia image, SkColor expected_color) { Can be moved to unnamed namespace? https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:303: bool LoadManifestFromString( Eliminate this method. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:321: class WallpaperImageURLFetcherCallback { Please move to the unnamed namespace and add short comment for this class. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:342: EXPECT_GE(passed, I don't think you should check for specific timings and timeouts in tests. Our overall approach is to set such timeouts to zero i.e. add new test method like SetZeroWallpaperFetchTimeoutForTesting() https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:356: new net::HttpResponseHeaders(""); nit: std::string https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:383: class WallpaperImageFetcherFactory { Please move to the unnamed namespace and add short comment for this class.
https://codereview.chromium.org/253833006/diff/1/net/url_request/test_url_fet... File net/url_request/test_url_fetcher_factory.cc (right): https://codereview.chromium.org/253833006/diff/1/net/url_request/test_url_fet... net/url_request/test_url_fetcher_factory.cc:176: // File saving is done in SaveResponseToFileAtPath() I think this comment is confusing. Suggest something more along the lines of "SaveResponseToFileAtPath should be called instead in this case" (In URLFetcherImpl, neither of these functions do any writing, which I think makes the comment confusing)
just looked at the beginning of the browsertest file https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... File chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:27: #include "testing/gmock/include/gmock/gmock.h" are you actually using this? it doesn't look like it https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:37: #define OEM_WALLPAPER_URL "http://somedomain.com/image.png" i think it'd be preferable to just hardcode this in both the constant and the string literal instead of using a macro here https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:66: bool CreateJPEGImage(int width, please don't just copy-and-paste all of this code from wallpaper_manager_browsertest.cc. move it to a shared location. the same goes for all the other stuff that's being duplicated (CreateCmdlineWallpapers looks like a copy of WriteWallpapers, WriteJPEGFile, etc.)
https://codereview.chromium.org/253833006/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_document.h (right): https://codereview.chromium.org/253833006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:199: friend class CustomizationWallpaperDownloaderBrowserTest; Please use FRIEND_TEST_ALL_PREFIXES. https://codereview.chromium.org/253833006/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:146: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100)); We need to fins better approach when sleep i.e. wait for some event.
+bshe@ as reviewer because I've modified wallpaper_manager.h. Please review again. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... File chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:5: #include "chrome/browser/chromeos/customization_wallpaper_downloader.h" On 2014/04/29 08:56:03, Nikita Kostylev wrote: > nit: This include should go with the others. Done. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:27: #include "testing/gmock/include/gmock/gmock.h" On 2014/04/29 16:41:16, Daniel Erat wrote: > are you actually using this? it doesn't look like it Done. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:37: #define OEM_WALLPAPER_URL "http://somedomain.com/image.png" On 2014/04/29 16:41:16, Daniel Erat wrote: > i think it'd be preferable to just hardcode this in both the constant and the > string literal instead of using a macro here Done. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:66: bool CreateJPEGImage(int width, On 2014/04/29 16:41:16, Daniel Erat wrote: > please don't just copy-and-paste all of this code from > wallpaper_manager_browsertest.cc. move it to a shared location. > > the same goes for all the other stuff that's being duplicated > (CreateCmdlineWallpapers looks like a copy of WriteWallpapers, WriteJPEGFile, > etc.) Done. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:92: class TestObserver : public WallpaperManager::Observer { On 2014/04/29 08:56:03, Nikita Kostylev wrote: > Please move to the unnamed namespace. Done. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:92: class TestObserver : public WallpaperManager::Observer { On 2014/04/29 08:56:03, Nikita Kostylev wrote: > nit: TestWallpaperObserver. Done. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:110: while (!finished_) { On 2014/04/29 08:56:03, Nikita Kostylev wrote: > nit: drop {} Done. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:146: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100)); On 2014/04/29 08:56:03, Nikita Kostylev wrote: > Potential source of flakiness. > > Please eliminate sleep approach. This seems like a workaround that will break on > some platform/configuration etc. Done. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:181: // Creates a test image of given size On 2014/04/29 08:56:03, Nikita Kostylev wrote: > nit: dot at the end. Done. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:182: gfx::ImageSkia CreateTestImage(int width, int height, SkColor color) { On 2014/04/29 08:56:03, Nikita Kostylev wrote: > Can be moved to unnamed namespace? Done. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:192: bool WriteJPEGFile(const base::FilePath& path, On 2014/04/29 08:56:03, Nikita Kostylev wrote: > Can be moved to unnamed namespace? Done. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:270: bool ImageIsNearColor(gfx::ImageSkia image, SkColor expected_color) { On 2014/04/29 08:56:03, Nikita Kostylev wrote: > Can be moved to unnamed namespace? Done. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:303: bool LoadManifestFromString( On 2014/04/29 08:56:03, Nikita Kostylev wrote: > Eliminate this method. Done. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:321: class WallpaperImageURLFetcherCallback { On 2014/04/29 08:56:03, Nikita Kostylev wrote: > Please move to the unnamed namespace and add short comment for this class. Done. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:342: EXPECT_GE(passed, On 2014/04/29 08:56:03, Nikita Kostylev wrote: > I don't think you should check for specific timings and timeouts in tests. > > Our overall approach is to set such timeouts to zero i.e. add new test method > like SetZeroWallpaperFetchTimeoutForTesting() This checks that retry is not sent too early to prevent server from overloading. So this is an intended check. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:356: new net::HttpResponseHeaders(""); On 2014/04/29 08:56:03, Nikita Kostylev wrote: > nit: std::string Done. https://codereview.chromium.org/253833006/diff/1/chrome/browser/chromeos/cust... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:383: class WallpaperImageFetcherFactory { On 2014/04/29 08:56:03, Nikita Kostylev wrote: > Please move to the unnamed namespace and add short comment for this class. Done. https://codereview.chromium.org/253833006/diff/1/net/url_request/test_url_fet... File net/url_request/test_url_fetcher_factory.cc (right): https://codereview.chromium.org/253833006/diff/1/net/url_request/test_url_fet... net/url_request/test_url_fetcher_factory.cc:176: // File saving is done in SaveResponseToFileAtPath() On 2014/04/29 14:11:40, mmenke wrote: > I think this comment is confusing. Suggest something more along the lines of > "SaveResponseToFileAtPath should be called instead in this case" (In > URLFetcherImpl, neither of these functions do any writing, which I think makes > the comment confusing) Done. https://codereview.chromium.org/253833006/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_document.h (right): https://codereview.chromium.org/253833006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_document.h:199: friend class CustomizationWallpaperDownloaderBrowserTest; On 2014/04/29 18:39:42, Dmitry Polukhin wrote: > Please use FRIEND_TEST_ALL_PREFIXES. Done. https://codereview.chromium.org/253833006/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:146: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100)); On 2014/04/29 18:39:42, Dmitry Polukhin wrote: > We need to fins better approach when sleep i.e. wait for some event. Done.
https://codereview.chromium.org/253833006/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager_test_utils.cc (right): https://codereview.chromium.org/253833006/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_test_utils.cc:25: class TestWallpaperObserverPendingListEmpty It doesn't look like you accessed private methods/members in WallpaperManager or maybe I missed something. You may not need to make it a friend of WallpaperManager. https://codereview.chromium.org/253833006/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager_test_utils.h (right): https://codereview.chromium.org/253833006/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_test_utils.h:20: // It should not have non-static methods. You can probably just use namespace test_utils instead of a class. This way you dont need to make all methods static
https://codereview.chromium.org/253833006/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager_test_utils.cc (right): https://codereview.chromium.org/253833006/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_test_utils.cc:25: class TestWallpaperObserverPendingListEmpty On 2014/04/29 23:24:06, bshe wrote: > It doesn't look like you accessed private methods/members in WallpaperManager or > maybe I missed something. You may not need to make it a friend of > WallpaperManager. This one is not a friend. It is a descendant of WallpaperManager::Observer. https://codereview.chromium.org/253833006/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager_test_utils.h (right): https://codereview.chromium.org/253833006/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_test_utils.h:20: // It should not have non-static methods. On 2014/04/29 23:24:06, bshe wrote: > You can probably just use namespace test_utils instead of a class. This way you > dont need to make all methods static Done. It used 'list_' member directly in the previous version.
https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:56: const size_t kMinOEMWallpaperRetryIntervalSec = 10; size_t isn't the right type for this; just use int https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:61: : finished_(false), wallpaper_manager_(wallpaper_manager) { nit: one member per line https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:94: : url_(url), require_retries_(require_retries), factory_(NULL) { nit: one member per line https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:101: net::URLFetcherDelegate* d, s/d/delegate/ https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:110: base::TimeDelta::FromSeconds(kMinOEMWallpaperRetryIntervalSec)) tests shouldn't sleep. please use something like base/time/tick_clock.h in both the implementation and this test so that it can run without blocking here. https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:137: size_t attempts() const { return attempts_.size(); } please name this num_attempts() instead; attempts() would be an accessor for |attempts_| https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:144: std::vector<base::Time> attempts_; use base::TimeTicks, not base::Time. the former isn't affected by the system clock being updated https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:170: size_t attempts() const { return url_callback_->attempts(); } nit: num_attempts() https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:209: : controller_(NULL), local_state_(NULL) { nit: one per line https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:227: nit: delete extra blank line here https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:273: EXPECT_EQ(size_t(1), url_factory.attempts()); either use 1U here or c++-style casts, e.g. static_cast<size_t>(1) https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:303: EXPECT_EQ(size_t(2), url_factory.attempts()); update cast here too https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1753: ++i) you need curly brackets here https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1759: if (loading_.size() == 0) nit: if (loading_.empty()) https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager_test_utils.h (right): https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_test_utils.h:27: // A custom color, pecifically chosen to not s/pecifically/specifically/ https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_test_utils.h:36: bool CreateJPEGImage(int width, add a comment https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_test_utils.h:68: delete blank line
https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:56: const size_t kMinOEMWallpaperRetryIntervalSec = 10; On 2014/04/30 00:24:22, Daniel Erat wrote: > size_t isn't the right type for this; just use int Done. https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:61: : finished_(false), wallpaper_manager_(wallpaper_manager) { On 2014/04/30 00:24:22, Daniel Erat wrote: > nit: one member per line Done. https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:94: : url_(url), require_retries_(require_retries), factory_(NULL) { On 2014/04/30 00:24:22, Daniel Erat wrote: > nit: one member per line Done. https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:101: net::URLFetcherDelegate* d, On 2014/04/30 00:24:22, Daniel Erat wrote: > s/d/delegate/ Done. https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:110: base::TimeDelta::FromSeconds(kMinOEMWallpaperRetryIntervalSec)) On 2014/04/30 00:24:22, Daniel Erat wrote: > tests shouldn't sleep. please use something like base/time/tick_clock.h in both > the implementation and this test so that it can run without blocking here. CustomizationWallpaperDownloader uses base::OneShotTimer<CustomizationWallpaperDownloader> request_scheduled_; to delay retry call. How can I adjust TimeTicks counter in MessageLoop ? https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:137: size_t attempts() const { return attempts_.size(); } On 2014/04/30 00:24:22, Daniel Erat wrote: > please name this num_attempts() instead; attempts() would be an accessor for > |attempts_| Done. https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:170: size_t attempts() const { return url_callback_->attempts(); } On 2014/04/30 00:24:22, Daniel Erat wrote: > nit: num_attempts() Done. https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:209: : controller_(NULL), local_state_(NULL) { On 2014/04/30 00:24:22, Daniel Erat wrote: > nit: one per line Done. https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:227: On 2014/04/30 00:24:22, Daniel Erat wrote: > nit: delete extra blank line here Done. https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:273: EXPECT_EQ(size_t(1), url_factory.attempts()); On 2014/04/30 00:24:22, Daniel Erat wrote: > either use 1U here or c++-style casts, e.g. static_cast<size_t>(1) Done. https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:303: EXPECT_EQ(size_t(2), url_factory.attempts()); On 2014/04/30 00:24:22, Daniel Erat wrote: > update cast here too Done. https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1753: ++i) On 2014/04/30 00:24:22, Daniel Erat wrote: > you need curly brackets here Done. https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:1759: if (loading_.size() == 0) On 2014/04/30 00:24:22, Daniel Erat wrote: > nit: if (loading_.empty()) Done. https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager_test_utils.h (right): https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_test_utils.h:27: // A custom color, pecifically chosen to not On 2014/04/30 00:24:22, Daniel Erat wrote: > s/pecifically/specifically/ Done. https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_test_utils.h:36: bool CreateJPEGImage(int width, On 2014/04/30 00:24:22, Daniel Erat wrote: > add a comment Done. https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_test_utils.h:68: On 2014/04/30 00:24:22, Daniel Erat wrote: > delete blank line Done.
On Apr 29, 2014 6:10 PM, <alemate@chromium.org> wrote: > > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > File > chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc > (right): > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:56: > const size_t kMinOEMWallpaperRetryIntervalSec = 10; > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> size_t isn't the right type for this; just use int > > > Done. > > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:61: > : finished_(false), wallpaper_manager_(wallpaper_manager) { > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> nit: one member per line > > > Done. > > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:94: > : url_(url), require_retries_(require_retries), factory_(NULL) { > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> nit: one member per line > > > Done. > > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:101: > net::URLFetcherDelegate* d, > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> s/d/delegate/ > > > Done. > > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:110: > base::TimeDelta::FromSeconds(kMinOEMWallpaperRetryIntervalSec)) > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> tests shouldn't sleep. please use something like > > base/time/tick_clock.h in both >> >> the implementation and this test so that it can run without blocking > > here. > > > CustomizationWallpaperDownloader uses > > base::OneShotTimer<CustomizationWallpaperDownloader> request_scheduled_; > > > to delay retry call. How can I adjust TimeTicks counter in MessageLoop ? how about adding a set_retry_delay_for_testing() setter to the downloader that the test can call? > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:137: > size_t attempts() const { return attempts_.size(); } > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> please name this num_attempts() instead; attempts() would be an > > accessor for >> >> |attempts_| > > > Done. > > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:170: > size_t attempts() const { return url_callback_->attempts(); } > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> nit: num_attempts() > > > Done. > > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:209: > : controller_(NULL), local_state_(NULL) { > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> nit: one per line > > > Done. > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:227: > > > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> nit: delete extra blank line here > > > Done. > > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:273: > EXPECT_EQ(size_t(1), url_factory.attempts()); > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> either use 1U here or c++-style casts, e.g. static_cast<size_t>(1) > > > Done. > > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:303: > EXPECT_EQ(size_t(2), url_factory.attempts()); > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> update cast here too > > > Done. > > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > File chrome/browser/chromeos/login/wallpaper_manager.cc (right): > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/login/wallpaper_manager.cc:1753: ++i) > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> you need curly brackets here > > > Done. > > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/login/wallpaper_manager.cc:1759: if > (loading_.size() == 0) > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> nit: if (loading_.empty()) > > > Done. > > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > File chrome/browser/chromeos/login/wallpaper_manager_test_utils.h > (right): > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/login/wallpaper_manager_test_utils.h:27: // A > custom color, pecifically chosen to not > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> s/pecifically/specifically/ > > > Done. > > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/login/wallpaper_manager_test_utils.h:36: bool > CreateJPEGImage(int width, > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> add a comment > > > Done. > > https://codereview.chromium.org/253833006/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/login/wallpaper_manager_test_utils.h:68: > On 2014/04/30 00:24:22, Daniel Erat wrote: >> >> delete blank line > > > Done. > > https://codereview.chromium.org/253833006/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
net/url_request/ LGTM
Test adjusts retry delay to reduce test time. Please review.
https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document.h (right): https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.h:205: friend class TestWallpaperImageURLFetcherCallback; can you remove the friends if you just add a public accessor for the downloader? https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader.h (right): https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader.h:60: void set_retry_delay_for_testing(double value) { leave this as public; there's a presubmit check that _for_testing methods aren't called from outside of tests. (you probably don't need to list the friend then, right?) also use base::TimeDelta instead of double. "double value" doesn't explain anything about the units that are being used -- everyone calling this would need to read the implementation to see what to pass. https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader.h:102: double retry_delay_seconds_; base::TimeDelta https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:56: const double kMinOEMWallpaperRetryIntervalSec = 2; s/double/int/ can this be smaller?
lgtm after you address derat's comments https://codereview.chromium.org/253833006/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager_test_utils.cc (right): https://codereview.chromium.org/253833006/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager_test_utils.cc:25: class TestWallpaperObserverPendingListEmpty Hah, Right. Sorry I miss read WallpaperManagerTestUtils as this class somehow. On 2014/04/30 00:04:44, alemate wrote: > On 2014/04/29 23:24:06, bshe wrote: > > It doesn't look like you accessed private methods/members in WallpaperManager > or > > maybe I missed something. You may not need to make it a friend of > > WallpaperManager. > > This one is not a friend. It is a descendant of WallpaperManager::Observer. https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:162: // Directory created by CreateCmdlineWallpapersAndSetFlags() to store default There is no function named CreateCmdlineWallpapersAndSetFlags, you probably means: CreateCmdlineWallpapers instead.
https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document.h (right): https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.h:205: friend class TestWallpaperImageURLFetcherCallback; On 2014/04/30 21:23:06, Daniel Erat wrote: > can you remove the friends if you just add a public accessor for the downloader? Done. https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader.h (right): https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader.h:60: void set_retry_delay_for_testing(double value) { On 2014/04/30 21:23:06, Daniel Erat wrote: > leave this as public; there's a presubmit check that _for_testing methods aren't > called from outside of tests. (you probably don't need to list the friend then, > right?) > > also use base::TimeDelta instead of double. "double value" doesn't explain > anything about the units that are being used -- everyone calling this would need > to read the implementation to see what to pass. Done. https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader.h:102: double retry_delay_seconds_; On 2014/04/30 21:23:06, Daniel Erat wrote: > base::TimeDelta Done. https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:56: const double kMinOEMWallpaperRetryIntervalSec = 2; On 2014/04/30 21:23:06, Daniel Erat wrote: > s/double/int/ > > can this be smaller? Done. https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/110001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:162: // Directory created by CreateCmdlineWallpapersAndSetFlags() to store default On 2014/05/02 14:58:18, bshe wrote: > There is no function named CreateCmdlineWallpapersAndSetFlags, you probably > means: > CreateCmdlineWallpapers instead. Done.
https://codereview.chromium.org/253833006/diff/130001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document.h (right): https://codereview.chromium.org/253833006/diff/130001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.h:197: // For testing delete unnecessary comment https://codereview.chromium.org/253833006/diff/130001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.h:198: CustomizationWallpaperDownloader* GetWallpaperDownloaderForTesting() const; i think it's fine to inline this and make it into an accessor: CustomizationWallpaperDownloader* wallpaper_downloader_for_testing() { return wallpaper_downloader_.get(); } also, make it non-const since it returns a non-const pointer to a member. https://codereview.chromium.org/253833006/diff/130001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/130001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:56: const int kDownloadRetryIntervalSec = 2; you didn't answer my question about whether this can be smaller than two seconds. why not e.g. 100 milliseconds?
https://codereview.chromium.org/253833006/diff/130001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_document.h (right): https://codereview.chromium.org/253833006/diff/130001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.h:197: // For testing On 2014/05/05 14:17:29, Daniel Erat wrote: > delete unnecessary comment Done. https://codereview.chromium.org/253833006/diff/130001/chrome/browser/chromeos... chrome/browser/chromeos/customization_document.h:198: CustomizationWallpaperDownloader* GetWallpaperDownloaderForTesting() const; On 2014/05/05 14:17:29, Daniel Erat wrote: > i think it's fine to inline this and make it into an accessor: > > CustomizationWallpaperDownloader* wallpaper_downloader_for_testing() { > return wallpaper_downloader_.get(); > } > > also, make it non-const since it returns a non-const pointer to a member. Done. https://codereview.chromium.org/253833006/diff/130001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/130001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:56: const int kDownloadRetryIntervalSec = 2; On 2014/05/05 14:17:29, Daniel Erat wrote: > you didn't answer my question about whether this can be smaller than two > seconds. why not e.g. 100 milliseconds? Sorry, I misunderstood you. The idea was to make the time between retry attempts to depend mostly on this value. Debug builds are rather slowly, so setting this to 100ms can result in checking not retry timer delay, but debug checks. But I can set it to 100ms is you think it would still make test stable enough.
https://codereview.chromium.org/253833006/diff/130001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/130001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:56: const int kDownloadRetryIntervalSec = 2; On 2014/05/06 19:04:17, alemate wrote: > On 2014/05/05 14:17:29, Daniel Erat wrote: > > you didn't answer my question about whether this can be smaller than two > > seconds. why not e.g. 100 milliseconds? > > Sorry, I misunderstood you. > > The idea was to make the time between retry attempts to depend mostly on this > value. Debug builds are rather slowly, so setting > this to 100ms can result in checking not retry timer delay, but > debug checks. > > But I can set it to 100ms is you think it would still make test > stable enough. making tests depend on time in any way is usually a bad idea. is there a reason you can't just make the downloader track the amount of time it _would've_ slept and expose that to tests via an accessor?
https://codereview.chromium.org/253833006/diff/130001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/130001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:56: const int kDownloadRetryIntervalSec = 2; On 2014/05/06 20:23:32, Daniel Erat wrote: > On 2014/05/06 19:04:17, alemate wrote: > > On 2014/05/05 14:17:29, Daniel Erat wrote: > > > you didn't answer my question about whether this can be smaller than two > > > seconds. why not e.g. 100 milliseconds? > > > > Sorry, I misunderstood you. > > > > The idea was to make the time between retry attempts to depend mostly on this > > value. Debug builds are rather slowly, so setting > > this to 100ms can result in checking not retry timer delay, but > > debug checks. > > > > But I can set it to 100ms is you think it would still make test > > stable enough. > > making tests depend on time in any way is usually a bad idea. is there a reason > you can't just make the downloader track the amount of time it _would've_ slept > and expose that to tests via an accessor? Done.
lgtm with a few more comments https://codereview.chromium.org/253833006/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right): https://codereview.chromium.org/253833006/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader.cc:76: retry_current_delay_(base::TimeDelta::FromSeconds(0)), don't need to initialize this; a 0-second delay is the default https://codereview.chromium.org/253833006/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader.h (right): https://codereview.chromium.org/253833006/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader.h:60: base::TimeDelta get_retry_current_delay_for_testing() const { nit: rename to retry_current_delay_for_testing() (no "get" on inline accessors)
https://codereview.chromium.org/253833006/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right): https://codereview.chromium.org/253833006/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader.cc:76: retry_current_delay_(base::TimeDelta::FromSeconds(0)), On 2014/05/07 00:10:33, Daniel Erat wrote: > don't need to initialize this; a 0-second delay is the default Done. https://codereview.chromium.org/253833006/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader.h (right): https://codereview.chromium.org/253833006/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader.h:60: base::TimeDelta get_retry_current_delay_for_testing() const { On 2014/05/07 00:10:33, Daniel Erat wrote: > nit: rename to retry_current_delay_for_testing() (no "get" on inline accessors) Done.
dpolukhin@, nkostylev@, please review.
lgtm
lgtm https://codereview.chromium.org/253833006/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:234: virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE { Please check that this test works fine with --multi-profiles switch passed to it so that this new test won't block enabling MP by default.
The CQ bit was checked by nkostylev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/253833006/240001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
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/253833006/240001
Message was sent while issue was closed.
Change committed as 269907
Message was sent while issue was closed.
https://codereview.chromium.org/253833006/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc (right): https://codereview.chromium.org/253833006/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/customization_wallpaper_downloader_browsertest.cc:234: virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE { On 2014/05/08 08:30:47, Nikita Kostylev wrote: > Please check that this test works fine with --multi-profiles switch passed to it > so that this new test won't block enabling MP by default. https://codereview.chromium.org/283183002/ |
