Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2639403002:
[Autofill] Remove direct use of base::Time::Now() in Autofill (Closed)
|
Created:
3 years, 11 months ago by sebsg Modified:
3 years, 9 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Autofill] Remove direct use of base::Time::Now() in Autofill
Adds a standard way to get the current time in Autofill. With the AutofillClock, it is now possible to inject a test clock that can return specific times when queried.
I only made effective use of it in the PersonalDataManagerTest for now.
BUG=682379
Review-Url: https://codereview.chromium.org/2639403002
Cr-Commit-Position: refs/heads/master@{#447572}
Committed: https://chromium.googlesource.com/chromium/src/+/8c21ca772b51f0b9f12879713dd74a6b09d88316
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 20
Patch Set 3 : Addressed comments #
Total comments: 14
Patch Set 4 : Addressed comments #Patch Set 5 : Rebase #Patch Set 6 : Added a scoped TestAutofillClock #
Total comments: 12
Patch Set 7 : Use LazyInstance #Patch Set 8 : Rebase #
Total comments: 2
Patch Set 9 : Added DCHECK #Messages
Total messages: 90 (76 generated)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Autofill] Remove direct use of base::Time::Now() in Autofill BUG=682379 ========== to ========== [Autofill] Remove direct use of base::Time::Now() in Autofill Adds a standard way to get the current time in Autofill. With the AutofillClock, it is now possible to inject a test clock that can return specific times when queried. I only made effective use of it in the PersonalDataManagerTest for now. BUG=682379 ==========
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Hi Math, PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Up to you if you want to change things to AutofillClock in tests without yet using the TestClock. Makes the conversion a bit harder to track :) Some comments! https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_clock.cc (right): https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. +4 https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.cc:5: #include "components/autofill/core/browser/autofill_clock.h" does it make sense to have this in core/common? https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.cc:12: static base::LazyInstance<AutofillClock> instance_ = LAZY_INSTANCE_INITIALIZER; naming style is probably to have g_autofill_clock (check other lazy instances). Otherwise you are adding |instance_| in the autofill namespace... https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.cc:12: static base::LazyInstance<AutofillClock> instance_ = LAZY_INSTANCE_INITIALIZER; can this be in the anonymous namespace? https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.cc:20: clock_ = base::MakeUnique<base::DefaultClock>(); I prefer the style clock_.reset(new base::DefaultClock); https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.cc:26: void AutofillClock::SetClockForTests(base::Clock* clock) { probably better to pass a std::unique_ptr? makes ownership transfer more explicit https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_clock.h (right): https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.h:7: include <memory> https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.h:11: #include "base/memory/ptr_util.h" need? https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.h:26: AutofillClock(AutofillClock const&) = delete; is that DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.h:30: FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerTest, worried this will get cluttered... friend class PersonalDataManagerTest; is good if you can expose the clock functionality through a class method on PersonalDataManagerTest.
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #3 (id:140001) has been deleted
Thanks Math, another look? https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_clock.cc (right): https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2017/01/25 19:57:24, Mathieu Perreault wrote: > +4 Done. https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.cc:5: #include "components/autofill/core/browser/autofill_clock.h" On 2017/01/25 19:57:24, Mathieu Perreault wrote: > does it make sense to have this in core/common? Done. https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.cc:12: static base::LazyInstance<AutofillClock> instance_ = LAZY_INSTANCE_INITIALIZER; On 2017/01/25 19:57:24, Mathieu Perreault wrote: > can this be in the anonymous namespace? Done. https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.cc:12: static base::LazyInstance<AutofillClock> instance_ = LAZY_INSTANCE_INITIALIZER; On 2017/01/25 19:57:24, Mathieu Perreault wrote: > naming style is probably to have g_autofill_clock (check other lazy instances). > Otherwise you are adding |instance_| in the autofill namespace... Done. https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.cc:20: clock_ = base::MakeUnique<base::DefaultClock>(); On 2017/01/25 19:57:24, Mathieu Perreault wrote: > I prefer the style > > clock_.reset(new base::DefaultClock); Done. https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.cc:26: void AutofillClock::SetClockForTests(base::Clock* clock) { On 2017/01/25 19:57:24, Mathieu Perreault wrote: > probably better to pass a std::unique_ptr? makes ownership transfer more > explicit True, thanks for the suggestion. https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_clock.h (right): https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.h:7: On 2017/01/25 19:57:24, Mathieu Perreault wrote: > include <memory> Done. https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.h:11: #include "base/memory/ptr_util.h" On 2017/01/25 19:57:24, Mathieu Perreault wrote: > need? Not anymore! https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.h:26: AutofillClock(AutofillClock const&) = delete; On 2017/01/25 19:57:24, Mathieu Perreault wrote: > is that DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_clock.h:30: FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerTest, On 2017/01/25 19:57:24, Mathieu Perreault wrote: > worried this will get cluttered... > > friend class PersonalDataManagerTest; is good if you can expose the clock > functionality through a class method on PersonalDataManagerTest. It was much easier that I thought, thanks for the suggestion.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm with nits https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager_unittest.cc (right): https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:72: const base::Time kSecondArbitraryTestTime = base::Time::FromDoubleT(1000); nit: tests would read even better with ArbitraryTime, SomeTimeLater, MuchLater :) https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... File components/autofill/core/common/autofill_clock.cc (right): https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... components/autofill/core/common/autofill_clock.cc:6: #include <utility> https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... File components/autofill/core/common/autofill_clock.h (right): https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... components/autofill/core/common/autofill_clock.h:5: #ifndef COMPONENTS_AUTOFILL_CORE_BROWSER_AUTOFILL_CLOCK_H_ fix https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... components/autofill/core/common/autofill_clock.h:10: #include "base/gtest_prod_util.h" need? https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... components/autofill/core/common/autofill_clock.h:11: #include "base/lazy_instance.h" move to cc? https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... components/autofill/core/common/autofill_clock.h:21: class AutofillClock { Please add at least a class comment https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... components/autofill/core/common/autofill_clock.h:25: AutofillClock(); nit: I think I would put ctor and dtor before the static
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #4 (id:180001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #5 (id:220001) has been deleted
Patchset #5 (id:240001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:260001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:280001) has been deleted
Patchset #5 (id:300001) has been deleted
Patchset #5 (id:320001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sebsg@chromium.org changed reviewers: + rogerm@chromium.org
Hi Math, adding Roger for his opinion too. Could you please take a look specifically at autofill_clock test_autofill_clock personal_data_manager_unittest Thanks! https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager_unittest.cc (right): https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:72: const base::Time kSecondArbitraryTestTime = base::Time::FromDoubleT(1000); On 2017/01/25 22:32:14, Mathieu Perreault wrote: > nit: tests would read even better with ArbitraryTime, SomeTimeLater, MuchLater > :) Done. https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... File components/autofill/core/common/autofill_clock.cc (right): https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... components/autofill/core/common/autofill_clock.cc:6: On 2017/01/25 22:32:14, Mathieu Perreault wrote: > #include <utility> Done. https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... File components/autofill/core/common/autofill_clock.h (right): https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... components/autofill/core/common/autofill_clock.h:5: #ifndef COMPONENTS_AUTOFILL_CORE_BROWSER_AUTOFILL_CLOCK_H_ On 2017/01/25 22:32:14, Mathieu Perreault wrote: > fix Done. https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... components/autofill/core/common/autofill_clock.h:10: #include "base/gtest_prod_util.h" On 2017/01/25 22:32:14, Mathieu Perreault wrote: > need? Done. https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... components/autofill/core/common/autofill_clock.h:11: #include "base/lazy_instance.h" On 2017/01/25 22:32:14, Mathieu Perreault wrote: > move to cc? Done. https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... components/autofill/core/common/autofill_clock.h:21: class AutofillClock { On 2017/01/25 22:32:14, Mathieu Perreault wrote: > Please add at least a class comment Done. https://codereview.chromium.org/2639403002/diff/160001/components/autofill/co... components/autofill/core/common/autofill_clock.h:25: AutofillClock(); On 2017/01/25 22:32:14, Mathieu Perreault wrote: > nit: I think I would put ctor and dtor before the static Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... File components/autofill/core/browser/test_autofill_clock.cc (right): https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... components/autofill/core/browser/test_autofill_clock.cc:5: #include "components/autofill/core/browser/test_autofill_clock.h" could this be in common too? https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... File components/autofill/core/common/autofill_clock.h (right): https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... components/autofill/core/common/autofill_clock.h:23: // Returns the current time base on |clock_|. *based on |clock_|
run git cl format https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... File components/autofill/core/browser/test_autofill_clock.cc (right): https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... components/autofill/core/browser/test_autofill_clock.cc:15: // Create a new test clock and set it as the AutofillClock clock and keep a indent -2, or maybe you've got tabs? https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... components/autofill/core/browser/test_autofill_clock.cc:19: test_clock_ = unique_test_clock.get(); this is kind of evil. Document that you're keeping this pointer (making the unique_ptr a lie). https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... File components/autofill/core/browser/test_autofill_clock.h (right): https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... components/autofill/core/browser/test_autofill_clock.h:29: void SetNow(base::Time now); nit: wonky indentation for the rest of the file https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... File components/autofill/core/common/autofill_clock.cc (right): https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... components/autofill/core/common/autofill_clock.cc:14: std::unique_ptr<base::Clock> AutofillClock::clock_; I think the autofill clock might need to be a singleton. We generally avoid any static variables that are not statically initialized or require destructors. You can turn Now() into GetInstance()->Now() internally.
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #7 (id:380001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Roger, PTAL? https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... File components/autofill/core/browser/test_autofill_clock.cc (right): https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... components/autofill/core/browser/test_autofill_clock.cc:5: #include "components/autofill/core/browser/test_autofill_clock.h" On 2017/01/31 18:19:37, Mathieu Perreault wrote: > could this be in common too? I put it with all the other test utils used for tests in .../core/browser I thought it fit better there. https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... components/autofill/core/browser/test_autofill_clock.cc:15: // Create a new test clock and set it as the AutofillClock clock and keep a On 2017/01/31 19:05:54, Roger McFarlane (Chromium) wrote: > indent -2, or maybe you've got tabs? Done. https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... components/autofill/core/browser/test_autofill_clock.cc:19: test_clock_ = unique_test_clock.get(); On 2017/01/31 19:05:54, Roger McFarlane (Chromium) wrote: > this is kind of evil. Document that you're keeping this pointer (making the > unique_ptr a lie). Done. https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... File components/autofill/core/browser/test_autofill_clock.h (right): https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... components/autofill/core/browser/test_autofill_clock.h:29: void SetNow(base::Time now); On 2017/01/31 19:05:54, Roger McFarlane (Chromium) wrote: > nit: wonky indentation for the rest of the file Done. https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... File components/autofill/core/common/autofill_clock.cc (right): https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... components/autofill/core/common/autofill_clock.cc:14: std::unique_ptr<base::Clock> AutofillClock::clock_; On 2017/01/31 19:05:54, Roger McFarlane (Chromium) wrote: > I think the autofill clock might need to be a singleton. We generally avoid any > static variables that are not statically initialized or require destructors. > > You can turn Now() into GetInstance()->Now() internally. Done. https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... File components/autofill/core/common/autofill_clock.h (right): https://codereview.chromium.org/2639403002/diff/360001/components/autofill/co... components/autofill/core/common/autofill_clock.h:23: // Returns the current time base on |clock_|. On 2017/01/31 18:19:37, Mathieu Perreault wrote: > *based on |clock_| Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/25 19:57:24, Mathieu Perreault wrote: > <font><font>Up cho bạn nếu bạn muốn thay đổi mọi thứ để AutofillClock trong các thử nghiệm mà không chưa</font></font><font></font><font><font> > sử dụng TestClock. </font><font>Làm cho việc chuyển đổi một chút khó khăn hơn để theo dõi :)</font></font><font></font> > <font></font><font><font> > Một số ý kiến!</font></font><font></font> > <font></font> > https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... > Tập tin thành phần / tự động điền / core / trình duyệt / autofill_clock.cc (bên phải):</font></font><font></font> > <font></font> > https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... > các thành phần / tự động điền / core / trình duyệt / autofill_clock.cc: 1: // Copyright 2013</font></font><font></font><font><font> > Tác giả Chromium. </font><font>Tất cả quyền được bảo lưu.</font></font><font></font><font><font> > 4</font></font><font></font> > <font></font> > https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... > các thành phần / tự động điền / core / trình duyệt / autofill_clock.cc: 5: #include</font></font><font></font><font><font> > "Các thành phần / tự động điền / core / trình duyệt / autofill_clock.h"</font></font><font></font><font><font> > nó làm cho tinh thần để có điều này trong lõi / chung?</font></font><font></font> > <font></font> > https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... > các thành phần / tự động điền / core / trình duyệt / autofill_clock.cc: 12: static</font></font><font></font><font><font> > cơ sở :: LazyInstance <AutofillClock> instance_ = LAZY_INSTANCE_INITIALIZER;</font></font><font></font><font><font> > phong cách đặt tên có lẽ là để có g_autofill_clock (kiểm tra các trường hợp lười biếng khác).</font></font><font></font><font><font> > Nếu không, bạn đang thêm | instance_ | </font><font>trong không gian tên tự động điền ...</font></font><font></font> > <font></font> > https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... > các thành phần / tự động điền / core / trình duyệt / autofill_clock.cc: 12: static</font></font><font></font><font><font> > cơ sở :: LazyInstance <AutofillClock> instance_ = LAZY_INSTANCE_INITIALIZER;</font></font><font></font><font><font> > này có thể được trong không gian tên vô danh?</font></font><font></font> > <font></font> > https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... > các thành phần / tự động điền / core / trình duyệt / autofill_clock.cc: 20: clock_ =</font></font><font></font><font><font> > cơ sở :: MakeUnique <base :: DefaultClock> ();</font></font><font></font><font><font> > Tôi thích phong cách </font></font><font></font> > <font></font><font><font> > clock_.reset (cơ sở mới :: DefaultClock);</font></font><font></font> > <font></font> > https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... > các thành phần / tự động điền / core / trình duyệt / autofill_clock.cc: 26: void</font></font><font></font><font><font> > AutofillClock :: SetClockForTests (cơ sở :: Clock * đồng hồ) {</font></font><font></font><font><font> > có lẽ tốt hơn để vượt qua một std :: unique_ptr? </font><font>làm chuyển quyền sở hữu hơn</font></font><font></font><font><font> > minh bạch</font></font><font></font> > <font></font> > https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... > Tập tin thành phần / tự động điền / core / trình duyệt / autofill_clock.h (bên phải):</font></font><font></font> > <font></font> > https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... > các thành phần / tự động điền / core / trình duyệt / autofill_clock.h: 7: </font></font><font></font><font><font> > bao gồm <nhớ></font></font><font></font> > <font></font> > https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... > các thành phần / tự động điền / core / trình duyệt / autofill_clock.h: 11: #include</font></font><font></font><font><font> > "Cơ sở / bộ nhớ / ptr_util.h"</font></font><font></font><font><font> > nhu cầu?</font></font><font></font> > <font></font> > https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... > các thành phần / tự động điền / core / trình duyệt / autofill_clock.h: 26:</font></font><font></font><font><font> > AutofillClock (AutofillClock const &) = xóa;</font></font><font></font><font><font> > là DISALLOW_COPY_AND_ASSIGN đó?</font></font><font></font> > <font></font> > https://codereview.chromium.org/2639403002/diff/80001/components/autofill/cor... > các thành phần / tự động điền / core / trình duyệt / autofill_clock.h: 30:</font></font><font></font><font><font> > FRIEND_TEST_ALL_PREFIXES (PersonalDataManagerTest,</font></font><font></font><font><font> > lo lắng rằng điều này sẽ nhận được lộn xộn ... </font></font><font></font> > <font></font><font><font> > lớp người bạn PersonalDataManagerTest; </font><font>là tốt nếu bạn có thể tiếp xúc với các đồng hồ</font></font><font></font><font><font> > chức năng thông qua một phương pháp học trên PersonalDataManagerTest.</font></font>
lgtm https://codereview.chromium.org/2639403002/diff/420001/components/autofill/co... File components/autofill/core/common/autofill_clock.cc (right): https://codereview.chromium.org/2639403002/diff/420001/components/autofill/co... components/autofill/core/common/autofill_clock.cc:38: void AutofillClock::SetTestClock(std::unique_ptr<base::Clock> clock) { DCHECK(clock != nullptr) ?
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, rogerm@chromium.org Link to the patchset: https://codereview.chromium.org/2639403002/#ps440001 (title: "Added DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Roger, sending to CQ https://codereview.chromium.org/2639403002/diff/420001/components/autofill/co... File components/autofill/core/common/autofill_clock.cc (right): https://codereview.chromium.org/2639403002/diff/420001/components/autofill/co... components/autofill/core/common/autofill_clock.cc:38: void AutofillClock::SetTestClock(std::unique_ptr<base::Clock> clock) { On 2017/02/01 15:34:26, Roger McFarlane (Chromium) wrote: > DCHECK(clock != nullptr) > ? Done.
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1485971024727470,
"parent_rev": "b766497105ee42a5460c69d75c80f9a4858285d3", "commit_rev":
"8c21ca772b51f0b9f12879713dd74a6b09d88316"}
Message was sent while issue was closed.
Description was changed from ========== [Autofill] Remove direct use of base::Time::Now() in Autofill Adds a standard way to get the current time in Autofill. With the AutofillClock, it is now possible to inject a test clock that can return specific times when queried. I only made effective use of it in the PersonalDataManagerTest for now. BUG=682379 ========== to ========== [Autofill] Remove direct use of base::Time::Now() in Autofill Adds a standard way to get the current time in Autofill. With the AutofillClock, it is now possible to inject a test clock that can return specific times when queried. I only made effective use of it in the PersonalDataManagerTest for now. BUG=682379 Review-Url: https://codereview.chromium.org/2639403002 Cr-Commit-Position: refs/heads/master@{#447572} Committed: https://chromium.googlesource.com/chromium/src/+/8c21ca772b51f0b9f12879713dd7... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:440001) as https://chromium.googlesource.com/chromium/src/+/8c21ca772b51f0b9f12879713dd7...
Message was sent while issue was closed.
thaovy231011@gmail.com changed reviewers: + Thaovy231011@gmail.com
Message was sent while issue was closed.
|
