Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2585253002:
Integration of PasswordReuseDetector into PasswordStore. (Closed)
|
Created:
4 years ago by dvadym Modified:
3 years, 11 months ago Reviewers:
vabr (Chromium) CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntegration of PasswordReuseDetector into PasswordStore.
This CL includes:
1.Creating, initializing and updating PasswordReuseDetector instance as a member of PasswordStore.
2.Introducing all flows for checking password reuse with CheckReuse() of ChromePasswordManagerClient and receiving response.
ChromePasswordManagerClient.CheckReuse() is not called now, it will be implemented in next CLs.
BUG=657041
Committed: https://crrev.com/ab0dd7668e48e6c0b84396b1bdbd80435aba1f6d
Cr-Commit-Position: refs/heads/master@{#440865}
Patch Set 1 #Patch Set 2 : Init reusedetector #Patch Set 3 : Tests #Patch Set 4 : pretifying #Patch Set 5 : rebase #Patch Set 6 : Tests fix #Patch Set 7 : more tests #
Total comments: 44
Patch Set 8 : Addressing reviewer's comments #Patch Set 9 : Reverted changes in PasswordManagerClinet #Patch Set 10 : Added missing files #Patch Set 11 : Fixed thread issues #Patch Set 12 : tiny fix #
Total comments: 6
Patch Set 13 : Addressed comments #Patch Set 14 : Tests fix #Patch Set 15 : Thread fixes #
Total comments: 6
Patch Set 16 : rebase #Patch Set 17 : Update comment #Patch Set 18 : Update comment #Patch Set 19 : attempt mac fix #Patch Set 20 : fix mac test and disable Init of ReuseDetector on Mac #Patch Set 21 : Disable test for mac #Patch Set 22 : Fix mac test #
Total comments: 8
Patch Set 23 : comments fixes #Patch Set 24 : Rebase #Messages
Total messages: 65 (35 generated)
Description was changed from ========== Integration of ... BUG=657041 ========== to ========== Integration of PasswordReuseDetector into PasswordStore. BUG=657041 ==========
Description was changed from ========== Integration of PasswordReuseDetector into PasswordStore. BUG=657041 ========== to ========== Integration of PasswordReuseDetector into PasswordStore. This CL includes: 1.Creating, initializing and updating PasswordReuseDetector instance as member of PasswordStore. 2.Introducing all flows for checking password reuse with CheckReuse() of ChromePasswordManagerClient and receiving response. Checking reuse is not called now, it will be implemented in next CLs. BUG=657041 ==========
Description was changed from ========== Integration of PasswordReuseDetector into PasswordStore. This CL includes: 1.Creating, initializing and updating PasswordReuseDetector instance as member of PasswordStore. 2.Introducing all flows for checking password reuse with CheckReuse() of ChromePasswordManagerClient and receiving response. Checking reuse is not called now, it will be implemented in next CLs. BUG=657041 ========== to ========== Integration of PasswordReuseDetector into PasswordStore. This CL includes: 1.Creating, initializing and updating PasswordReuseDetector instance as a member of PasswordStore. 2.Introducing all flows for checking password reuse with CheckReuse() of ChromePasswordManagerClient and receiving response. Checking reuse is not called now, it will be implemented in next CLs. BUG=657041 ==========
dvadym@chromium.org changed reviewers: + vabr@chromium.org
Hi Vaclav, Could you please review this CL? Regards, Vadym
Description was changed from ========== Integration of PasswordReuseDetector into PasswordStore. This CL includes: 1.Creating, initializing and updating PasswordReuseDetector instance as a member of PasswordStore. 2.Introducing all flows for checking password reuse with CheckReuse() of ChromePasswordManagerClient and receiving response. Checking reuse is not called now, it will be implemented in next CLs. BUG=657041 ========== to ========== Integration of PasswordReuseDetector into PasswordStore. This CL includes: 1.Creating, initializing and updating PasswordReuseDetector instance as a member of PasswordStore. 2.Introducing all flows for checking password reuse with CheckReuse() of ChromePasswordManagerClient and receiving response. ChromePasswordManagerClient.CheckReuse() is not called now, it will be implemented in next CLs. BUG=657041 ==========
Hi Vadym, Looks good in general, some comments below. Cheers, Vaclav https://codereview.chromium.org/2585253002/diff/120001/chrome/browser/passwor... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2585253002/diff/120001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.cc:384: GetPasswordStore()->CheckReuse(input, domain, consumer); What is the advantage of having PasswordManagerClient::CheckReuse over all the callsites just using the client's already existing GetPasswordStore() to call CheckReuse directly on the store? https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_manager_client.h (right): https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager_client.h:203: // |consumer| should not be null. optional nit: should not -> may not https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_reuse_detector.cc (right): https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_reuse_detector.cc:32: if (change.type() == PasswordStoreChange::ADD || Why are we not removing passwords when they get deleted? https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_reuse_detector.h (right): https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_reuse_detector.h:25: // Called when a password reuse is found. nit: Add a blank line between lines 24 and 25. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_store.cc (right): https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.cc:65: origin_task_runner_ = base::ThreadTaskRunnerHandle::Get(); Why not add , origin_task_runner_(base::ThreadTaskRunnerHandle::Get()) to the line above instead? https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.cc:66: } DCHECK(consumer) ? https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.cc:76: base::Unretained(consumer_), password, saved_domain)); PasswordStoreConsumer is called with a weak pointer (see line 51 above). Why is it OK to use Unretained here? https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.cc:309: std::unique_ptr<CheckReuseRequest> check_reuse_request( nit: auto check_reuse_request = base::MakeUnique<CheckReuseRequest>(consumer); to avoid type name duplication. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_store.h (right): https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.h:229: // |consumer| should not be null. nit: Let's be clear about |consumer|: it must not be null. Writing "should" makes the consequences unclear. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.h:274: explicit CheckReuseRequest(PasswordReuseDetectorConsumer* consumer); nit: Please mention that |consumer| must not be null. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.h:282: scoped_refptr<base::SingleThreadTaskRunner> origin_task_runner_; const scoped_refptr<base::SingleThreadTaskRunner> origin_task_runner_ to indicate that it is not changed during the lifetime of the request? https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.h:283: PasswordReuseDetectorConsumer* consumer_; PasswordReuseDetectorConsumer* const consumer_ to indicate that it is not changed during the lifetime of the request? https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.h:284: }; DISALLOW_COPY_AND_ASSIGN ? https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.h:529: std::unique_ptr<PasswordReuseDetector> reuse_detector_; Why is this a unique_ptr? Could it be just aggregated into PasswordStore? PasswordReuseDetector reuse_detector_; https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_store_unittest.cc (right): https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:844: static const PasswordFormData kTestCredentials[] = { Would constexpr instead of const work? But +1 to static, because of https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/-2PSZ... https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:855: for (size_t i = 0; i < arraysize(kTestCredentials); ++i) { nit: for (const auto& testcase : kTestCredentials) { ... https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:862: static const struct { static constexpr? https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:865: const wchar_t* reused_password; // set to nullptr if no reuse is expected. nit: set -> Set https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:873: for (size_t i = 0; i < arraysize(kReuseTestData); ++i) { for (const auto& test_data : kReuseTestData) { and drop line 875 https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:874: MockPasswordReuseDetectorConsumer mockConsumer; nit: mockConsumer -> mock_consumer https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:877: if (is_reuse_expected) { nit: Just inline test_data.reused_password here.
Thanks a lot Vaclav for comments. I've addressed them. PTAL https://codereview.chromium.org/2585253002/diff/120001/chrome/browser/passwor... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2585253002/diff/120001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.cc:384: GetPasswordStore()->CheckReuse(input, domain, consumer); On 2016/12/20 18:11:44, vabr (Chromium) wrote: > What is the advantage of having PasswordManagerClient::CheckReuse over all the > callsites just using the client's already existing GetPasswordStore() to call > CheckReuse directly on the store? Mainly for easier testing. In the next CL I'm going to use mock for this method for checking that it's fired. But if you think that it's better to call PasswordStore.CheckReuse. But you are right, probably these methods in the client could be unnessary. I've removed client changes from this CL and if needed I'll add them later. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_manager_client.h (right): https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager_client.h:203: // |consumer| should not be null. On 2016/12/20 18:11:44, vabr (Chromium) wrote: > optional nit: should not -> may not Done. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_reuse_detector.cc (right): https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_reuse_detector.cc:32: if (change.type() == PasswordStoreChange::ADD || On 2016/12/20 18:11:44, vabr (Chromium) wrote: > Why are we not removing passwords when they get deleted? That's hard to do 100% correct: if the user removed duplicating credentials from store, in |passwords_| we don't have information how many times specific password occurred and we can delete still valid password. But it looks like not deleting passwords are not the big problem: 1.It's rare. 2.False positive is better than false negative. 3.After Chrome restarts passwords will be deleted. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_reuse_detector.h (right): https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_reuse_detector.h:25: // Called when a password reuse is found. On 2016/12/20 18:11:44, vabr (Chromium) wrote: > nit: Add a blank line between lines 24 and 25. Done. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_store.cc (right): https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.cc:65: origin_task_runner_ = base::ThreadTaskRunnerHandle::Get(); On 2016/12/20 18:11:44, vabr (Chromium) wrote: > Why not add > , origin_task_runner_(base::ThreadTaskRunnerHandle::Get()) > to the line above instead? Done. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.cc:66: } On 2016/12/20 18:11:44, vabr (Chromium) wrote: > DCHECK(consumer) ? Done. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.cc:76: base::Unretained(consumer_), password, saved_domain)); On 2016/12/20 18:11:44, vabr (Chromium) wrote: > PasswordStoreConsumer is called with a weak pointer (see line 51 above). Why is > it OK to use Unretained here? Thanks, that's a very good point. I've done with weak pointer. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.cc:309: std::unique_ptr<CheckReuseRequest> check_reuse_request( On 2016/12/20 18:11:44, vabr (Chromium) wrote: > nit: auto check_reuse_request = base::MakeUnique<CheckReuseRequest>(consumer); > to avoid type name duplication. Thanks https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_store.h (right): https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.h:229: // |consumer| should not be null. On 2016/12/20 18:11:44, vabr (Chromium) wrote: > nit: Let's be clear about |consumer|: it must not be null. Writing "should" > makes the consequences unclear. Done. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.h:274: explicit CheckReuseRequest(PasswordReuseDetectorConsumer* consumer); On 2016/12/20 18:11:44, vabr (Chromium) wrote: > nit: Please mention that |consumer| must not be null. Done. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.h:282: scoped_refptr<base::SingleThreadTaskRunner> origin_task_runner_; On 2016/12/20 18:11:44, vabr (Chromium) wrote: > const scoped_refptr<base::SingleThreadTaskRunner> origin_task_runner_ > to indicate that it is not changed during the lifetime of the request? Done. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.h:283: PasswordReuseDetectorConsumer* consumer_; On 2016/12/20 18:11:44, vabr (Chromium) wrote: > PasswordReuseDetectorConsumer* const consumer_ > to indicate that it is not changed during the lifetime of the request? Done. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.h:284: }; On 2016/12/20 18:11:44, vabr (Chromium) wrote: > DISALLOW_COPY_AND_ASSIGN ? Yeah, thanks https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.h:529: std::unique_ptr<PasswordReuseDetector> reuse_detector_; On 2016/12/20 18:11:44, vabr (Chromium) wrote: > Why is this a unique_ptr? Could it be just aggregated into PasswordStore? > PasswordReuseDetector reuse_detector_; Sure, it could be. Done https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_store_unittest.cc (right): https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:844: static const PasswordFormData kTestCredentials[] = { On 2016/12/20 18:11:45, vabr (Chromium) wrote: > Would constexpr instead of const work? > > But +1 to static, because of > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/-2PSZ... Done. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:855: for (size_t i = 0; i < arraysize(kTestCredentials); ++i) { On 2016/12/20 18:11:45, vabr (Chromium) wrote: > nit: for (const auto& testcase : kTestCredentials) { ... Done. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:862: static const struct { On 2016/12/20 18:11:45, vabr (Chromium) wrote: > static constexpr? Done. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:865: const wchar_t* reused_password; // set to nullptr if no reuse is expected. On 2016/12/20 18:11:45, vabr (Chromium) wrote: > nit: set -> Set Done. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:873: for (size_t i = 0; i < arraysize(kReuseTestData); ++i) { On 2016/12/20 18:11:45, vabr (Chromium) wrote: > for (const auto& test_data : kReuseTestData) { > > and drop line 875 Done. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:874: MockPasswordReuseDetectorConsumer mockConsumer; On 2016/12/20 18:11:45, vabr (Chromium) wrote: > nit: mockConsumer -> mock_consumer Done. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:877: if (is_reuse_expected) { On 2016/12/20 18:11:45, vabr (Chromium) wrote: > nit: Just inline test_data.reused_password here. Done.
Now it's ready for review. I've fixed some threading issues. PTAL. https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_store.h (right): https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_store.h:529: std::unique_ptr<PasswordReuseDetector> reuse_detector_; On 2016/12/21 12:15:35, dvadym wrote: > On 2016/12/20 18:11:44, vabr (Chromium) wrote: > > Why is this a unique_ptr? Could it be just aggregated into PasswordStore? > > PasswordReuseDetector reuse_detector_; > > Sure, it could be. Done I've returned back to unique_ptr. It should be destroyed on UI thread.
Thanks, Vadym! LGTM with comments. Cheers, Vaclav https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_reuse_detector.cc (right): https://codereview.chromium.org/2585253002/diff/120001/components/password_ma... components/password_manager/core/browser/password_reuse_detector.cc:32: if (change.type() == PasswordStoreChange::ADD || On 2016/12/21 12:15:35, dvadym wrote: > On 2016/12/20 18:11:44, vabr (Chromium) wrote: > > Why are we not removing passwords when they get deleted? > > That's hard to do 100% correct: if the user removed duplicating credentials from > store, in |passwords_| we don't have information how many times specific > password occurred and we can delete still valid password. But it looks like not > deleting passwords are not the big problem: > 1.It's rare. > 2.False positive is better than false negative. > 3.After Chrome restarts passwords will be deleted. Acknowledged, this makes sense. https://codereview.chromium.org/2585253002/diff/220001/components/password_ma... File components/password_manager/core/browser/password_reuse_detector.cc (right): https://codereview.chromium.org/2585253002/diff/220001/components/password_ma... components/password_manager/core/browser/password_reuse_detector.cc:27: AddPassword(*form.get()); Please use just *form appending the ".get()" is unnecessary in this case. (In other words, unique_ptr has its own operator *, no need to use that of a raw pointer.) https://codereview.chromium.org/2585253002/diff/220001/components/password_ma... File components/password_manager/core/browser/password_reuse_detector_consumer.h (right): https://codereview.chromium.org/2585253002/diff/220001/components/password_ma... components/password_manager/core/browser/password_reuse_detector_consumer.h:27: base::WeakPtr<PasswordReuseDetectorConsumer> GetWeakPtr() { Please cut down this bolierplate method and just inherit from SupportsWeakPtr (which provides AsWeakPtr() method for you). That's the preferred way of creating weak pointers outside of the class they point to. WeakPtrFactory is meant for cases when the weak pointer is used internally (then there is no need for the accessor method). The only advantage of the factory is that you can control when it is destroyed, so if you put it after all data members, it will invalidate the weak pointers before the data members are deleted. That does not apply here, because the actual classes will derive from the consumer interface, so the factory will always get destroyed after all the data members. https://codereview.chromium.org/2585253002/diff/220001/components/password_ma... File components/password_manager/core/browser/password_reuse_detector_unittest.cc (right): https://codereview.chromium.org/2585253002/diff/220001/components/password_ma... components/password_manager/core/browser/password_reuse_detector_unittest.cc:47: for (auto& form : forms) nit: const auto& This will result in the same code as currently, because |forms| is const, but it will make the constness more visible.
Thanks Vaclav for review! https://codereview.chromium.org/2585253002/diff/220001/components/password_ma... File components/password_manager/core/browser/password_reuse_detector.cc (right): https://codereview.chromium.org/2585253002/diff/220001/components/password_ma... components/password_manager/core/browser/password_reuse_detector.cc:27: AddPassword(*form.get()); On 2016/12/21 14:38:19, vabr (Chromium) wrote: > Please use just > *form > appending the ".get()" is unnecessary in this case. (In other words, unique_ptr > has its own operator *, no need to use that of a raw pointer.) Thanks, that's left from removing of unique_ptr and vice versa. https://codereview.chromium.org/2585253002/diff/220001/components/password_ma... File components/password_manager/core/browser/password_reuse_detector_consumer.h (right): https://codereview.chromium.org/2585253002/diff/220001/components/password_ma... components/password_manager/core/browser/password_reuse_detector_consumer.h:27: base::WeakPtr<PasswordReuseDetectorConsumer> GetWeakPtr() { On 2016/12/21 14:38:19, vabr (Chromium) wrote: > Please cut down this bolierplate method and just inherit from SupportsWeakPtr > (which provides AsWeakPtr() method for you). That's the preferred way of > creating weak pointers outside of the class they point to. > > WeakPtrFactory is meant for cases when the weak pointer is used internally (then > there is no need for the accessor method). The only advantage of the factory is > that you can control when it is destroyed, so if you put it after all data > members, it will invalidate the weak pointers before the data members are > deleted. That does not apply here, because the actual classes will derive from > the consumer interface, so the factory will always get destroyed after all the > data members. Thanks a lot for explanation. I had no idea about SupportsWeakPtr. I've changed this class to be inherited from SupportsWeakPtr. https://codereview.chromium.org/2585253002/diff/220001/components/password_ma... File components/password_manager/core/browser/password_reuse_detector_unittest.cc (right): https://codereview.chromium.org/2585253002/diff/220001/components/password_ma... components/password_manager/core/browser/password_reuse_detector_unittest.cc:47: for (auto& form : forms) On 2016/12/21 14:38:20, vabr (Chromium) wrote: > nit: const auto& > This will result in the same code as currently, because |forms| is const, but it > will make the constness more visible. Done.
LGTM! :)
The CQ bit was checked by dvadym@chromium.org
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
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 dvadym@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_ozone_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 dvadym@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...
Vaclav could please have a look again? You can check difference with PatchSet 13. The main fix is that PasswordReuseDetector is created and destroyed on the background thread, not on UI. Now all operations with PasswordReuseDetector are on the background thread.
LGTM with one comment. Thanks! Vaclav https://codereview.chromium.org/2585253002/diff/280001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc (right): https://codereview.chromium.org/2585253002/diff/280001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:113: EXPECT_CALL(*GetStore(), FillAutofillableLogins(_)); It is a bit strange to expect a call and then return the function. Normally the code which will result in the expected call should be below the EXPECT macro. What is causing the call to FillAutofillableLogins? Could you put the EXPECT_CALL before that?
https://codereview.chromium.org/2585253002/diff/280001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc (right): https://codereview.chromium.org/2585253002/diff/280001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:113: EXPECT_CALL(*GetStore(), FillAutofillableLogins(_)); On 2016/12/22 16:15:12, vabr (Chromium) wrote: > It is a bit strange to expect a call and then return the function. Normally the > code which will result in the expected call should be below the EXPECT macro. > What is causing the call to FillAutofillableLogins? Could you put the > EXPECT_CALL before that? PasswordStore initializes PasswordReuseDetector, and it causes call of FillAutofillableLogins. So a call of BuildPasswordStore itself causes a call of FillAutofillableLogins, but since it's on a posted task it happens after returning from BuildPasswordStore. Note that there is no thread race, since unit tests are running on 1 thread and any posted task is always running after current function execution is finished. I can explain all this in comments? Would it be ok?
https://codereview.chromium.org/2585253002/diff/280001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc (right): https://codereview.chromium.org/2585253002/diff/280001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:113: EXPECT_CALL(*GetStore(), FillAutofillableLogins(_)); On 2016/12/22 16:23:12, dvadym wrote: > On 2016/12/22 16:15:12, vabr (Chromium) wrote: > > It is a bit strange to expect a call and then return the function. Normally > the > > code which will result in the expected call should be below the EXPECT macro. > > What is causing the call to FillAutofillableLogins? Could you put the > > EXPECT_CALL before that? > > PasswordStore initializes PasswordReuseDetector, and it causes call of > FillAutofillableLogins. So a call of BuildPasswordStore itself causes a call of > FillAutofillableLogins, but since it's on a posted task it happens after > returning from BuildPasswordStore. > Note that there is no thread race, since unit tests are running on 1 thread and > any posted task is always running after current function execution is finished. > I can explain all this in comments? Would it be ok? Thanks for the explanation. In that case please move the EXPECT_CALL before line 108. You might comment on the expectation being fulfilled in a task posted as a result of the call below.
https://codereview.chromium.org/2585253002/diff/280001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc (right): https://codereview.chromium.org/2585253002/diff/280001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:113: EXPECT_CALL(*GetStore(), FillAutofillableLogins(_)); On 2016/12/22 16:33:30, vabr (Chromium) wrote: > On 2016/12/22 16:23:12, dvadym wrote: > > On 2016/12/22 16:15:12, vabr (Chromium) wrote: > > > It is a bit strange to expect a call and then return the function. Normally > > the > > > code which will result in the expected call should be below the EXPECT > macro. > > > What is causing the call to FillAutofillableLogins? Could you put the > > > EXPECT_CALL before that? > > > > PasswordStore initializes PasswordReuseDetector, and it causes call of > > FillAutofillableLogins. So a call of BuildPasswordStore itself causes a call > of > > FillAutofillableLogins, but since it's on a posted task it happens after > > returning from BuildPasswordStore. > > Note that there is no thread race, since unit tests are running on 1 thread > and > > any posted task is always running after current function execution is > finished. > > I can explain all this in comments? Would it be ok? > > Thanks for the explanation. In that case please move the EXPECT_CALL before line > 108. You might comment on the expectation being fulfilled in a task posted as a > result of the call below. If I correctly understand, line 108 creates password store, so I can't use EXPECT_CALL on store object before it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Silly me! :) Still LGTM. Cheers, Vaclav https://codereview.chromium.org/2585253002/diff/280001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc (right): https://codereview.chromium.org/2585253002/diff/280001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:113: EXPECT_CALL(*GetStore(), FillAutofillableLogins(_)); On 2016/12/22 16:57:57, dvadym wrote: > On 2016/12/22 16:33:30, vabr (Chromium) wrote: > > On 2016/12/22 16:23:12, dvadym wrote: > > > On 2016/12/22 16:15:12, vabr (Chromium) wrote: > > > > It is a bit strange to expect a call and then return the function. > Normally > > > the > > > > code which will result in the expected call should be below the EXPECT > > macro. > > > > What is causing the call to FillAutofillableLogins? Could you put the > > > > EXPECT_CALL before that? > > > > > > PasswordStore initializes PasswordReuseDetector, and it causes call of > > > FillAutofillableLogins. So a call of BuildPasswordStore itself causes a call > > of > > > FillAutofillableLogins, but since it's on a posted task it happens after > > > returning from BuildPasswordStore. > > > Note that there is no thread race, since unit tests are running on 1 thread > > and > > > any posted task is always running after current function execution is > > finished. > > > I can explain all this in comments? Would it be ok? > > > > Thanks for the explanation. In that case please move the EXPECT_CALL before > line > > 108. You might comment on the expectation being fulfilled in a task posted as > a > > result of the call below. > > If I correctly understand, line 108 creates password store, so I can't use > EXPECT_CALL on store object before it. You are right, sorry for missing that! Then please do comment that the expectation is met in a task posted by the above construction.
Thanks for review! https://codereview.chromium.org/2585253002/diff/280001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc (right): https://codereview.chromium.org/2585253002/diff/280001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:113: EXPECT_CALL(*GetStore(), FillAutofillableLogins(_)); On 2016/12/22 17:14:31, vabr (Chromium) wrote: > On 2016/12/22 16:57:57, dvadym wrote: > > On 2016/12/22 16:33:30, vabr (Chromium) wrote: > > > On 2016/12/22 16:23:12, dvadym wrote: > > > > On 2016/12/22 16:15:12, vabr (Chromium) wrote: > > > > > It is a bit strange to expect a call and then return the function. > > Normally > > > > the > > > > > code which will result in the expected call should be below the EXPECT > > > macro. > > > > > What is causing the call to FillAutofillableLogins? Could you put the > > > > > EXPECT_CALL before that? > > > > > > > > PasswordStore initializes PasswordReuseDetector, and it causes call of > > > > FillAutofillableLogins. So a call of BuildPasswordStore itself causes a > call > > > of > > > > FillAutofillableLogins, but since it's on a posted task it happens after > > > > returning from BuildPasswordStore. > > > > Note that there is no thread race, since unit tests are running on 1 > thread > > > and > > > > any posted task is always running after current function execution is > > > finished. > > > > I can explain all this in comments? Would it be ok? > > > > > > Thanks for the explanation. In that case please move the EXPECT_CALL before > > line > > > 108. You might comment on the expectation being fulfilled in a task posted > as > > a > > > result of the call below. > > > > If I correctly understand, line 108 creates password store, so I can't use > > EXPECT_CALL on store object before it. > > You are right, sorry for missing that! > Then please do comment that the expectation is met in a task posted by the above > construction. Thanks, I've updated comment.
The CQ bit was checked by dvadym@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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by dvadym@chromium.org
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Hi Vaclav, I've fixed Mac issues with tests and disabled for now PasswordReuse detector init on Mac, could you please have a loot at PatchSet 19?
I've decided that for now it's easier to disable test on Mac. So please have a look at both PatchSet 20 and 21.
The CQ bit was checked by dvadym@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.
LGTM with nits. Thanks! Vaclav https://codereview.chromium.org/2585253002/diff/420001/components/password_ma... File components/password_manager/core/browser/password_store.cc (right): https://codereview.chromium.org/2585253002/diff/420001/components/password_ma... components/password_manager/core/browser/password_store.cc:695: // TODO(crbug.com/668155): For non-migrated keychain users it can lead for nit: lead for -> lead to https://codereview.chromium.org/2585253002/diff/420001/components/password_ma... components/password_manager/core/browser/password_store.cc:696: // hundreds requests to unlock keychain. To find a way to fix it. nit: hundreds requests -> hundreds of requests https://codereview.chromium.org/2585253002/diff/420001/components/password_ma... components/password_manager/core/browser/password_store.cc:696: // hundreds requests to unlock keychain. To find a way to fix it. nit: The last sentence ("To find a way to fix it") misses a verb and seems to contribute no additional information. I suggest dropping it. https://codereview.chromium.org/2585253002/diff/420001/components/password_ma... File components/password_manager/core/browser/password_store_unittest.cc (right): https://codereview.chromium.org/2585253002/diff/420001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:844: // TODO(crbug.com/668155): Enable this test for Mac. optional nit: Perhaps instead of saying "enable this test", comment on what is the issue on Mac and what needs to be done to enable it.
Thanks for review Vaclav! https://codereview.chromium.org/2585253002/diff/420001/components/password_ma... File components/password_manager/core/browser/password_store.cc (right): https://codereview.chromium.org/2585253002/diff/420001/components/password_ma... components/password_manager/core/browser/password_store.cc:695: // TODO(crbug.com/668155): For non-migrated keychain users it can lead for On 2016/12/28 15:48:01, vabr (Chromium) wrote: > nit: lead for -> lead to Done. https://codereview.chromium.org/2585253002/diff/420001/components/password_ma... components/password_manager/core/browser/password_store.cc:696: // hundreds requests to unlock keychain. To find a way to fix it. On 2016/12/28 15:48:01, vabr (Chromium) wrote: > nit: The last sentence ("To find a way to fix it") misses a verb and seems to > contribute no additional information. I suggest dropping it. Done. https://codereview.chromium.org/2585253002/diff/420001/components/password_ma... components/password_manager/core/browser/password_store.cc:696: // hundreds requests to unlock keychain. To find a way to fix it. On 2016/12/28 15:48:02, vabr (Chromium) wrote: > nit: hundreds requests -> hundreds of requests Done. https://codereview.chromium.org/2585253002/diff/420001/components/password_ma... File components/password_manager/core/browser/password_store_unittest.cc (right): https://codereview.chromium.org/2585253002/diff/420001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:844: // TODO(crbug.com/668155): Enable this test for Mac. On 2016/12/28 15:48:02, vabr (Chromium) wrote: > optional nit: Perhaps instead of saying "enable this test", comment on what is > the issue on Mac and what needs to be done to enable it. I've added more text. But anyway, I'm going to fix it in 2 weeks or so.
The CQ bit was checked by dvadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org Link to the patchset: https://codereview.chromium.org/2585253002/#ps440001 (title: "comments fixes")
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
Failed to apply patch for components/password_manager/core/browser/BUILD.gn:
While running git apply --index -p1;
error: patch failed: components/password_manager/core/browser/BUILD.gn:97
error: components/password_manager/core/browser/BUILD.gn: patch does not apply
Patch: components/password_manager/core/browser/BUILD.gn
Index: components/password_manager/core/browser/BUILD.gn
diff --git a/components/password_manager/core/browser/BUILD.gn
b/components/password_manager/core/browser/BUILD.gn
index
b1acc8970e4c87f6ded13f472d652b227eb9aa71..ddec88e8c444a4e43c61e5d0bf587e1d8591b6b4
100644
--- a/components/password_manager/core/browser/BUILD.gn
+++ b/components/password_manager/core/browser/BUILD.gn
@@ -97,6 +97,8 @@ static_library("browser") {
"password_manager_util.h",
"password_reuse_detector.cc",
"password_reuse_detector.h",
+ "password_reuse_detector_consumer.cc",
+ "password_reuse_detector_consumer.h",
"password_store.cc",
"password_store.h",
"password_store_change.cc",
The CQ bit was checked by dvadym@chromium.org
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
Failed to apply patch for components/password_manager/core/browser/BUILD.gn:
While running git apply --index -p1;
error: patch failed: components/password_manager/core/browser/BUILD.gn:97
error: components/password_manager/core/browser/BUILD.gn: patch does not apply
Patch: components/password_manager/core/browser/BUILD.gn
Index: components/password_manager/core/browser/BUILD.gn
diff --git a/components/password_manager/core/browser/BUILD.gn
b/components/password_manager/core/browser/BUILD.gn
index
b1acc8970e4c87f6ded13f472d652b227eb9aa71..ddec88e8c444a4e43c61e5d0bf587e1d8591b6b4
100644
--- a/components/password_manager/core/browser/BUILD.gn
+++ b/components/password_manager/core/browser/BUILD.gn
@@ -97,6 +97,8 @@ static_library("browser") {
"password_manager_util.h",
"password_reuse_detector.cc",
"password_reuse_detector.h",
+ "password_reuse_detector_consumer.cc",
+ "password_reuse_detector_consumer.h",
"password_store.cc",
"password_store.h",
"password_store_change.cc",
The CQ bit was checked by dvadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org Link to the patchset: https://codereview.chromium.org/2585253002/#ps460001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 460001, "attempt_start_ts": 1482944211981250,
"parent_rev": "151ced0e7df1b1b1461c94381c1fa104c088fbe7", "commit_rev":
"476d854e45e632e15a66590c5cdb74e3301aac96"}
Message was sent while issue was closed.
Description was changed from ========== Integration of PasswordReuseDetector into PasswordStore. This CL includes: 1.Creating, initializing and updating PasswordReuseDetector instance as a member of PasswordStore. 2.Introducing all flows for checking password reuse with CheckReuse() of ChromePasswordManagerClient and receiving response. ChromePasswordManagerClient.CheckReuse() is not called now, it will be implemented in next CLs. BUG=657041 ========== to ========== Integration of PasswordReuseDetector into PasswordStore. This CL includes: 1.Creating, initializing and updating PasswordReuseDetector instance as a member of PasswordStore. 2.Introducing all flows for checking password reuse with CheckReuse() of ChromePasswordManagerClient and receiving response. ChromePasswordManagerClient.CheckReuse() is not called now, it will be implemented in next CLs. BUG=657041 Review-Url: https://codereview.chromium.org/2585253002 ==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== Integration of PasswordReuseDetector into PasswordStore. This CL includes: 1.Creating, initializing and updating PasswordReuseDetector instance as a member of PasswordStore. 2.Introducing all flows for checking password reuse with CheckReuse() of ChromePasswordManagerClient and receiving response. ChromePasswordManagerClient.CheckReuse() is not called now, it will be implemented in next CLs. BUG=657041 Review-Url: https://codereview.chromium.org/2585253002 ========== to ========== Integration of PasswordReuseDetector into PasswordStore. This CL includes: 1.Creating, initializing and updating PasswordReuseDetector instance as a member of PasswordStore. 2.Introducing all flows for checking password reuse with CheckReuse() of ChromePasswordManagerClient and receiving response. ChromePasswordManagerClient.CheckReuse() is not called now, it will be implemented in next CLs. BUG=657041 Committed: https://crrev.com/ab0dd7668e48e6c0b84396b1bdbd80435aba1f6d Cr-Commit-Position: refs/heads/master@{#440865} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/ab0dd7668e48e6c0b84396b1bdbd80435aba1f6d Cr-Commit-Position: refs/heads/master@{#440865} |
