Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(127)

Issue 2907493002: ChromeOS: Per-user time zone: refactor tests first. (Closed)

Created:
3 years, 7 months ago by Alexander Alekseev
Modified:
3 years, 6 months ago
CC:
chromium-reviews, dbeam+watch-options_chromium.org, extensions-reviews_chromium.org, msramek+watch_chromium.org, alemate+watch_chromium.org, Peter Beverloo, awdf+watch_chromium.org, Lei Zhang, yamaguchi+watch_chromium.org, oka+watch_chromium.org, achuith+watch_chromium.org, tommycli, rginda+watch_chromium.org, michaelpg+watch-options_chromium.org, oshima+watch_chromium.org, fukino+watch_chromium.org, chromium-apps-reviews_chromium.org, markusheintz_, mlamouri+watch-notifications_chromium.org, davemoore+watch_chromium.org, Bernhard Bauer, Devlin, Reilly Grant (use Gerrit)
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

ChromeOS: Per-user time zone: refactor tests first. This Cl prepares tests to support per-user timezone implementation. BUG=622555, 699472 TBR=bauerb@chromium.org,rdevlin.cronin@chromium.org,reillyg@chromium.org Review-Url: https://codereview.chromium.org/2907493002 Cr-Original-Commit-Position: refs/heads/master@{#476478} Committed: https://chromium.googlesource.com/chromium/src/+/410dbeb8df3d4ab140390d7d1e603163885c8c9c Review-Url: https://codereview.chromium.org/2907493002 Cr-Commit-Position: refs/heads/master@{#476787} Committed: https://chromium.googlesource.com/chromium/src/+/5f96a4f208edf90610e14cfb146b71bbc344da05

Patch Set 1 #

Patch Set 2 : Fix build #

Patch Set 3 : Move more code from dependent CL here. #

Total comments: 9

Patch Set 4 : Update after review. #

Total comments: 15

Patch Set 5 : Update after review. #

Patch Set 6 : Update after review #

Patch Set 7 : Update after review #

Total comments: 2

Patch Set 8 : Rebased. #

Patch Set 9 : Fixed several tests. #

Patch Set 10 : Cleaned up tests. #

Patch Set 11 : Fix debug build #

Unified diffs Side-by-side diffs Delta from patch set Stats (+196 lines, -209 lines) Patch
M chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +0 lines, -4 lines 0 comments Download
M chrome/browser/chromeos/BUILD.gn View 1 2 3 4 5 6 7 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/customization/customization_document_unittest.cc View 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos_unittest.cc View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/chromeos/file_manager/path_util_unittest.cc View 1 2 3 4 5 6 2 chunks +10 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc View 2 chunks +9 lines, -4 lines 0 comments Download
M chrome/browser/chromeos/login/hwid_checker_unittest.cc View 1 2 3 4 5 6 7 8 8 chunks +11 lines, -16 lines 0 comments Download
M chrome/browser/chromeos/login/signin/merge_session_load_page_unittest.cc View 3 chunks +0 lines, -14 lines 0 comments Download
M chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc View 1 2 3 4 5 6 7 2 chunks +7 lines, -2 lines 0 comments Download
M chrome/browser/chromeos/login/users/supervised_user_manager_impl.cc View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc View 1 2 2 chunks +3 lines, -1 line 0 comments Download
A chrome/browser/chromeos/scoped_set_running_on_chromeos_for_testing.h View 1 2 3 4 5 6 7 8 1 chunk +31 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/scoped_set_running_on_chromeos_for_testing.cc View 1 2 3 4 5 6 1 chunk +21 lines, -0 lines 0 comments Download
M chrome/browser/extensions/active_tab_unittest.cc View 1 2 3 4 5 6 7 8 9 3 chunks +1 line, -5 lines 0 comments Download
M chrome/browser/extensions/activity_log/activity_database_unittest.cc View 1 2 3 4 5 6 7 8 9 3 chunks +0 lines, -20 lines 0 comments Download
M chrome/browser/extensions/activity_log/activity_log_enabled_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +0 lines, -18 lines 0 comments Download
M chrome/browser/extensions/activity_log/activity_log_unittest.cc View 1 2 3 4 5 6 7 8 9 4 chunks +0 lines, -18 lines 0 comments Download
M chrome/browser/extensions/api/image_writer_private/operation_manager_unittest.cc View 2 chunks +0 lines, -12 lines 0 comments Download
M chrome/browser/extensions/extension_service_unittest.cc View 1 2 3 4 5 6 7 3 chunks +0 lines, -15 lines 0 comments Download
M chrome/browser/extensions/test_extension_system.h View 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/browser/extensions/test_extension_system.cc View 2 chunks +9 lines, -1 line 0 comments Download
M chrome/browser/media_galleries/media_file_system_registry_unittest.cc View 4 chunks +0 lines, -10 lines 0 comments Download
M chrome/browser/notifications/extension_welcome_notification_unittest.cc View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/notifications/platform_notification_service_unittest.cc View 2 chunks +0 lines, -13 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 3 chunks +4 lines, -4 lines 0 comments Download
M chrome/test/base/testing_profile.h View 1 2 3 4 5 6 7 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/test/base/testing_profile.cc View 1 2 3 4 5 6 7 8 2 chunks +8 lines, -0 lines 0 comments Download
M components/user_manager/user_manager_base.cc View 1 2 20 chunks +49 lines, -49 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 78 (59 generated)
stevenjb
Sorry if this isn't quite ready, but some early feedback (I'm trying to get through ...
3 years, 7 months ago (2017-05-25 21:20:24 UTC) #12
Alexander Alekseev
https://codereview.chromium.org/2907493002/diff/40001/chrome/browser/chromeos/file_manager/path_util_unittest.cc File chrome/browser/chromeos/file_manager/path_util_unittest.cc (right): https://codereview.chromium.org/2907493002/diff/40001/chrome/browser/chromeos/file_manager/path_util_unittest.cc#newcode28 chrome/browser/chromeos/file_manager/path_util_unittest.cc:28: // Sets a valid Chrome OS version info so ...
3 years, 7 months ago (2017-05-26 02:18:01 UTC) #15
Alexander Alekseev
Please review: phajdan.jr@ chrome/test/* xiyuan@ all This is first half of longer CL https://codereview.chromium.org/2849823003 . ...
3 years, 7 months ago (2017-05-26 02:21:12 UTC) #17
xiyuan
https://codereview.chromium.org/2907493002/diff/60001/chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc File chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc (right): https://codereview.chromium.org/2907493002/diff/60001/chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc#newcode14 chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc:14: base::SysInfo::SetChromeOSVersionInfoForTest(lsb_release, lsb_release_time); ScopedSetRunningOnChromeOSForTesting is necessary because SetChromeOSVersionInfoForTest uses env ...
3 years, 7 months ago (2017-05-26 17:14:53 UTC) #22
Alexander Alekseev
https://codereview.chromium.org/2907493002/diff/60001/chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc File chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc (right): https://codereview.chromium.org/2907493002/diff/60001/chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc#newcode14 chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc:14: base::SysInfo::SetChromeOSVersionInfoForTest(lsb_release, lsb_release_time); On 2017/05/26 17:14:53, xiyuan wrote: > ScopedSetRunningOnChromeOSForTesting ...
3 years, 7 months ago (2017-05-26 18:06:21 UTC) #23
xiyuan
https://codereview.chromium.org/2907493002/diff/60001/chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc File chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc (right): https://codereview.chromium.org/2907493002/diff/60001/chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc#newcode11 chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc:11: ScopedSetRunningOnChromeOSForTesting::ScopedSetRunningOnChromeOSForTesting( ScopedSetRunningOnChromeOSForTesting and the file name does not match. ...
3 years, 7 months ago (2017-05-26 19:14:36 UTC) #26
Alexander Alekseev
https://codereview.chromium.org/2907493002/diff/60001/chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc File chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc (right): https://codereview.chromium.org/2907493002/diff/60001/chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc#newcode11 chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc:11: ScopedSetRunningOnChromeOSForTesting::ScopedSetRunningOnChromeOSForTesting( On 2017/05/26 19:14:35, xiyuan wrote: > ScopedSetRunningOnChromeOSForTesting and ...
3 years, 6 months ago (2017-05-29 17:06:04 UTC) #29
xiyuan
lgtm https://codereview.chromium.org/2907493002/diff/60001/chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc File chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc (right): https://codereview.chromium.org/2907493002/diff/60001/chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc#newcode14 chrome/browser/chromeos/scoped_sys_info_chromeos_version_for_testing.cc:14: base::SysInfo::SetChromeOSVersionInfoForTest(lsb_release, lsb_release_time); On 2017/05/29 17:06:04, Alexander Alekseev wrote: ...
3 years, 6 months ago (2017-05-30 15:39:39 UTC) #38
stevenjb
owner lgtm (if needed)
3 years, 6 months ago (2017-05-30 23:26:33 UTC) #43
Paweł Hajdan Jr.
LGTM w/nit https://codereview.chromium.org/2907493002/diff/120001/chrome/test/base/testing_profile.cc File chrome/test/base/testing_profile.cc (right): https://codereview.chromium.org/2907493002/diff/120001/chrome/test/base/testing_profile.cc#newcode433 chrome/test/base/testing_profile.cc:433: if (!chromeos::CrosSettings::IsInitialized()) nit: Use {}.
3 years, 6 months ago (2017-05-31 13:07:22 UTC) #48
Alexander Alekseev
https://codereview.chromium.org/2907493002/diff/120001/chrome/test/base/testing_profile.cc File chrome/test/base/testing_profile.cc (right): https://codereview.chromium.org/2907493002/diff/120001/chrome/test/base/testing_profile.cc#newcode433 chrome/test/base/testing_profile.cc:433: if (!chromeos::CrosSettings::IsInitialized()) On 2017/05/31 13:07:21, Paweł Hajdan Jr. wrote: ...
3 years, 6 months ago (2017-06-01 18:44:06 UTC) #53
Alexander Alekseev
TBRing individual test owners: bauerb@chromium.org: c/b/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc rdevlin.cronin@chromium.org: chrome/browser/extensions/* reillyg@chromium.org: c/b/media_galleries/media_file_system_registry_unittest.cc
3 years, 6 months ago (2017-06-01 20:39:48 UTC) #59
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2907493002/180001
3 years, 6 months ago (2017-06-01 21:22:21 UTC) #62
commit-bot: I haz the power
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/410dbeb8df3d4ab140390d7d1e603163885c8c9c
3 years, 6 months ago (2017-06-01 23:17:29 UTC) #65
Peter Kasting
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/2919933002/ by pkasting@chromium.org. ...
3 years, 6 months ago (2017-06-02 00:23:57 UTC) #66
Peter Kasting
More detail: I think the problem here is TestingProfile pulling in ScopedCrosSettingsTestHelper, which pulls in ...
3 years, 6 months ago (2017-06-02 00:28:10 UTC) #67
Alexander Alekseev
On 2017/06/02 00:28:10, Peter Kasting wrote: > More detail: I think the problem here is ...
3 years, 6 months ago (2017-06-02 19:06:15 UTC) #72
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2907493002/200001
3 years, 6 months ago (2017-06-02 19:07:01 UTC) #75
commit-bot: I haz the power
3 years, 6 months ago (2017-06-02 21:09:56 UTC) #78
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/5f96a4f208edf90610e14cfb146b...

Powered by Google App Engine
This is Rietveld 408576698