Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1414463004:
Implement origin-based deletion for passwords in PasswordDefaultMac. (Closed)
|
Created:
5 years, 2 months ago by Timo Reimann Modified:
5 years ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org, vabr+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement origin-based deletion for passwords in PasswordDefaultMac.
This change adds the ability to remove passwords by origin (and time) for the password store under Mac.
While the implementation is fairly straight forward, the major work has been invested into moving the existing origin-based tests from PasswordStoreDefaultTest into a new type-parameterized test suite and refactoring PasswordStore{Default,Mac}Test so that the tests can be shared by both classes. We enabled the latter by means of introducing test delegates which encapsulate parts of the test setup and test helper logic.
Finally, we do some refactorings to extract common mock implementations into helper classes and improve test print output for PasswordStoreChanges.
BUG=113973
Committed: https://crrev.com/b4f01c0a90dcba304b45b280f18c66b3c65822f2
Cr-Commit-Position: refs/heads/master@{#365852}
Patch Set 1 #
Total comments: 57
Patch Set 2 : Rebase from upstream #Patch Set 3 : Address first round of comments. #
Total comments: 35
Patch Set 4 : Address second round of comments #Patch Set 5 : Rebase from upstream #
Total comments: 12
Patch Set 6 : Address additional comments. #Patch Set 7 : Run PasswordStoreMacTest DB operations on IO main loop. #
Total comments: 12
Patch Set 8 : Addressed all remaining comments. #
Total comments: 15
Patch Set 9 : Build simplified, single-threaded PasswordStoreMacTestDelegate and untouch PasswordStoreMacTest. #Patch Set 10 : Make FinishAsyncProcessing static. #
Total comments: 13
Patch Set 11 : Another round of comments addressed. #
Total comments: 2
Patch Set 12 : Move Google Test expectations just before usage. #Patch Set 13 : Rebase from upstream #Patch Set 14 : Remove extra base::Time namespace qualifier causing compiler problems. #Messages
Total messages: 74 (20 generated)
Description was changed from
==========
Implement origin-based deletion for passwords in PasswordDefaultMac.
This change adds the ability to remove passwords by origin (and time) for the
password store under Mac.
While the implementation is fairly straight forward, the major effort has been
invested into moving the existing origin-based tests from
PasswordStoreDefaultTest into a new type-parameterized test suite and
refactoring PasswordStore{Default,Mac}Test so that the tests can be shared by
both classes.
We enabled the latter by means of introducing test delegates which encapsulate
parts of the test setup and test helper logic.
BUG=113973
==========
to
==========
Implement origin-based deletion for passwords in PasswordDefaultMac.
This change adds the ability to remove passwords by origin (and time) for the
password store under Mac.
While the implementation is fairly straight forward, the major effort has been
invested into moving the existing origin-based tests from
PasswordStoreDefaultTest into a new type-parameterized test suite and
refactoring PasswordStore{Default,Mac}Test so that the tests can be shared by
both classes. We enabled the latter by means of introducing test delegates which
encapsulate parts of the test setup and test helper logic.
Finally, we do some refactorings to extract common mock implementations into
helper classes and improve test print output for PasswordStoreChanges.
BUG=113973
==========
Description was changed from
==========
Implement origin-based deletion for passwords in PasswordDefaultMac.
This change adds the ability to remove passwords by origin (and time) for the
password store under Mac.
While the implementation is fairly straight forward, the major effort has been
invested into moving the existing origin-based tests from
PasswordStoreDefaultTest into a new type-parameterized test suite and
refactoring PasswordStore{Default,Mac}Test so that the tests can be shared by
both classes. We enabled the latter by means of introducing test delegates which
encapsulate parts of the test setup and test helper logic.
Finally, we do some refactorings to extract common mock implementations into
helper classes and improve test print output for PasswordStoreChanges.
BUG=113973
==========
to
==========
Implement origin-based deletion for passwords in PasswordDefaultMac.
This change adds the ability to remove passwords by origin (and time) for the
password store under Mac.
While the implementation is fairly straight forward, the major effort has been
invested into moving the existing origin-based tests from
PasswordStoreDefaultTest into a new type-parameterized test suite and
refactoring PasswordStore{Default,Mac}Test so that the tests can be shared by
both classes. We enabled the latter by means of introducing test delegates which
encapsulate parts of the test setup and test helper logic.
Finally, we do some refactorings to extract common mock implementations into
helper classes and improve test print output for PasswordStoreChanges.
BUG=113973
==========
Description was changed from
==========
Implement origin-based deletion for passwords in PasswordDefaultMac.
This change adds the ability to remove passwords by origin (and time) for the
password store under Mac.
While the implementation is fairly straight forward, the major effort has been
invested into moving the existing origin-based tests from
PasswordStoreDefaultTest into a new type-parameterized test suite and
refactoring PasswordStore{Default,Mac}Test so that the tests can be shared by
both classes. We enabled the latter by means of introducing test delegates which
encapsulate parts of the test setup and test helper logic.
Finally, we do some refactorings to extract common mock implementations into
helper classes and improve test print output for PasswordStoreChanges.
BUG=113973
==========
to
==========
Implement origin-based deletion for passwords in PasswordDefaultMac.
This change adds the ability to remove passwords by origin (and time) for the
password store under Mac.
While the implementation is fairly straight forward, the major work has been
invested into moving the existing origin-based tests from
PasswordStoreDefaultTest into a new type-parameterized test suite and
refactoring PasswordStore{Default,Mac}Test so that the tests can be shared by
both classes. We enabled the latter by means of introducing test delegates which
encapsulate parts of the test setup and test helper logic.
Finally, we do some refactorings to extract common mock implementations into
helper classes and improve test print output for PasswordStoreChanges.
BUG=113973
==========
ttr314@googlemail.com changed reviewers: + vasilii@chromium.org
Taking on the next password store implementation, tackling the Mac version this time. Vasilii, would you mind reviewing again? I left a few comments already. Thanks! https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac.cc:1151: CleanOrphanedForms(&forms_to_remove); // Add the orphaned forms. Note that I did not implement tests to verify that keychain forms are removed / orphaned forms cleaned during the execution of this method as I wasn't completely sure about the right approach. Here are the available options I see: 1. Extend PasswordStoreMacTest.CheckRemoveLoginsBetween to cover origin-based deletion too. The downside is that this would duplicate some functionality (though not all) that PasswordStoreCommonTest.* (the new type-parameterized tests) already cares about in a more specific way. Consequently, in the worst case this could lead to two tests failing for the same regression, which is bad from a test design perspective. 2. Extract the keychain and orphaned forms parts from PasswordStoreMacTest.CheckRemoveLoginsBetween into a new test and share it by all methods dealing with the two form types. 3. Make the keychain and orphaned forms injectable so that we can use mocks to verify the behavior on. 4. Having read crbug.com/466638, I understand that we are dropping Mac keychain support soon. This might turn it acceptable to skip writing tests for the functionality at hand altogether and wait for the test gap to close naturally. Happy to hear your thoughts. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:212: void initialize() { The initialization of PasswordStoreMac for testing purposes seems more complicated that the equivalent for PasswordStoreDefault. For example, the Mac version manages at least one more thread. I am wondering if we can (and should) try to align the two approaches, either by making one more sophisticated (and thus supposedly more correct) or streamlining the other (and thus remove supposedly extra complexity). https://codereview.chromium.org/1414463004/diff/1/components/password_manager... File components/password_manager/core/browser/password_manager_test_utils.h (right): https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_manager_test_utils.h:65: class MockPasswordStoreObserver : public PasswordStore::Observer { Completely equal (but distinct) implementations of MockPasswordStoreObserver are shared among all store tests. Since password_manager_test_utils.h is also included by all of them, it seemed natural to have one central mock implementations and drop the private ones. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_change.cc (right): https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_change.cc:12: << "password form: " << password_store_change.form(); Let me know if you think this is a good enough format to display the two components of a password store change (i.e., the type and the form).
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414463004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414463004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2015/10/21 at 12:29:10, commit-bot wrote: > Dry run: Try jobs failed on following builders: > ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) > mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) Looks like I might need to rebase...
https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac.cc:1151: CleanOrphanedForms(&forms_to_remove); // Add the orphaned forms. On 2015/10/20 20:47:09, Timo Reimann wrote: > Note that I did not implement tests to verify that keychain forms are removed / > orphaned forms cleaned during the execution of this method as I wasn't > completely sure about the right approach. Here are the available options I see: > > 1. Extend PasswordStoreMacTest.CheckRemoveLoginsBetween to cover origin-based > deletion too. The downside is that this would duplicate some functionality > (though not all) that PasswordStoreCommonTest.* (the new type-parameterized > tests) already cares about in a more specific way. Consequently, in the worst > case this could lead to two tests failing for the same regression, which is bad > from a test design perspective. > 2. Extract the keychain and orphaned forms parts from > PasswordStoreMacTest.CheckRemoveLoginsBetween into a new test and share it by > all methods dealing with the two form types. > 3. Make the keychain and orphaned forms injectable so that we can use mocks to > verify the behavior on. > 4. Having read crbug.com/466638, I understand that we are dropping Mac keychain > support soon. This might turn it acceptable to skip writing tests for the > functionality at hand altogether and wait for the test gap to close naturally. > > Happy to hear your thoughts. You can proceed with the option 4. I don't think we ship your feature soon enough. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:172: namespace password_manager { You don't need this namespace. Use the anonymous one. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:178: initialize(); Method's name should start with a capital letter. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:196: base::Thread* thread() { return thread_.get(); } Why is it needed? https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:199: base::MessageLoopForUI message_loop_; Methods should be above. They are complex enough so don't make them inline. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:212: void initialize() { On 2015/10/20 20:47:09, Timo Reimann wrote: > The initialization of PasswordStoreMac for testing purposes seems more > complicated that the equivalent for PasswordStoreDefault. For example, the Mac > version manages at least one more thread. > > I am wondering if we can (and should) try to align the two approaches, either by > making one more sophisticated (and thus supposedly more correct) or streamlining > the other (and thus remove supposedly extra complexity). For your tests you can definitely simplify PasswordStoreMac creation by using just one thread as for PasswordStoreDefault. However, it should not change PasswordStoreMacTest logic. So choose one approach. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:217: OSCrypt::UseMockKeychain(true); You just pass the Keychain instance into constructor. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:234: void CreateAndInitPasswordStore(LoginDatabase* login_db) { merge the method into initialize() https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:248: if (!store_) It can't be true. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:1263: store_(delegate_.store()) {} I'm generally against changing this class. Instead you can simplify the delegate code a lot. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... File components/password_manager/core/browser/password_manager_test_utils.h (right): https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_manager_test_utils.h:65: class MockPasswordStoreObserver : public PasswordStore::Observer { On 2015/10/20 20:47:09, Timo Reimann wrote: > Completely equal (but distinct) implementations of MockPasswordStoreObserver are > shared among all store tests. Since password_manager_test_utils.h is also > included by all of them, it seemed natural to have one central mock > implementations and drop the private ones. Acknowledged. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_change.cc (right): https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_change.cc:12: << "password form: " << password_store_change.form(); On 2015/10/20 20:47:10, Timo Reimann wrote: > Let me know if you think this is a good enough format to display the two > components of a password store change (i.e., the type and the form). Don't put the line break. It should be one line. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_common_unittest.h (right): https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_common_unittest.h:32: nullptr, true, false, base::Time::Now().ToDoubleT()}; I'd prefer a const number here so the tests are reproducible. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_common_unittest.h:47: class PasswordStoreCommonTest : public testing::Test { If it's only about deletion by origin then I'd name the class accordingly. Or you want to move more common tests into PasswordStoreCommonTest? https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_common_unittest.h:49: PasswordStoreCommonTest() : store_(delegate_.store()) {} I'd move the init/deinit to SetUp()/TearDown(). https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_common_unittest.h:52: this->store_->Shutdown(); I'd separate the logic as following. The delegate is responsible for creating/destroying/initializing the password store. PasswordStoreCommonTest is responsible for the tests only. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_common_unittest.h:57: PasswordStore* store_; I think you don't need it as you can retrieve from the delegate. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_common_unittest.h:64: const char origin_url[] = "http://foo.example.com"; Declare the string as a constant in the beginning of the file. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:80: class PasswordStoreDefaultTestDelegateBase { Why do you need a base class if it's not used without PasswordStoreDefaultTestDelegate? https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:87: base::ScopedTempDir temp_dir_; Why is it protected? https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:98: }; Add noncopyable macros here and to the delegate. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:115: scoped_refptr<PasswordStoreDefault> store_; Member declaration is after methods declaration. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:125: base::ThreadTaskRunnerHandle::Get(), Should you use |message_loop_|? Otherwise why do you need |message_loop_|. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:141: class PasswordStoreDefaultTest : public testing::Test {}; You don't need an empty class. Just change TEST_F to TEST.
Sorry for the awfully long delay; pesky real life has been keeping me busy. I'll try my best to react more timely now. I addressed all comments that were clear to me. There are a few though where I needed to get back to you. Happy to follow up on them once we've got everything sorted out. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:172: namespace password_manager { On 2015/10/21 at 16:47:56, vasilii wrote: > You don't need this namespace. Use the anonymous one. Done. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:178: initialize(); On 2015/10/21 at 16:47:56, vasilii wrote: > Method's name should start with a capital letter. Done. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:196: base::Thread* thread() { return thread_.get(); } On 2015/10/21 at 16:47:56, vasilii wrote: > Why is it needed? PasswordStoreMacTest.ImportFromKeychain, PasswordStoreMacTest.ImportFederatedFromLockedKeychain, and PasswordStoreMacTest.ImportFromLockedKeychainError all post PasswordStoreMac::ImportFromKeychain to the thread's task runner during the respective tests. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:199: base::MessageLoopForUI message_loop_; On 2015/10/21 at 16:47:56, vasilii wrote: > Methods should be above. They are complex enough so don't make them inline. Done (for PasswordStoreDefaultTest too). https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:217: OSCrypt::UseMockKeychain(true); On 2015/10/21 at 16:47:56, vasilii wrote: > You just pass the Keychain instance into constructor. Which constructor do you mean? The test already passes an instance of MockAppleKeychain into PasswordStoreMac's constructor. However, that does not seem to suffice: Commenting out the call to OSCrypt::UseMockKeychain(true) leads to failing tests. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:234: void CreateAndInitPasswordStore(LoginDatabase* login_db) { On 2015/10/21 at 16:47:56, vasilii wrote: > merge the method into initialize() Done. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:248: if (!store_) On 2015/10/21 at 16:47:56, vasilii wrote: > It can't be true. I agree, unless there's a bug where store is attempted to be deleted twice. Should we completely drop the if-clause or replace it by a DCHECK, just to be safe and catch potential bugs early on? https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:1263: store_(delegate_.store()) {} On 2015/10/21 at 16:47:56, vasilii wrote: > I'm generally against changing this class. Instead you can simplify the delegate code a lot. I'm not sure I understand: The delegate was created by moving parts of PasswordStoreMacTest into it in order to provide a self-contained class to the new type-parameterized test template. So it seems we need to do at least a certain amount of modifications to PasswordStoreMacTest. Could you please elaborate on how the delegate should be simplified from your perspective? https://codereview.chromium.org/1414463004/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_change.cc (right): https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_change.cc:12: << "password form: " << password_store_change.form(); On 2015/10/21 at 16:47:56, vasilii wrote: > On 2015/10/20 20:47:10, Timo Reimann wrote: > > Let me know if you think this is a good enough format to display the two > > components of a password store change (i.e., the type and the form). > > Don't put the line break. It should be one line. Done. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_common_unittest.h (right): https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_common_unittest.h:32: nullptr, true, false, base::Time::Now().ToDoubleT()}; On 2015/10/21 at 16:47:57, vasilii wrote: > I'd prefer a const number here so the tests are reproducible. Good point, done. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_common_unittest.h:47: class PasswordStoreCommonTest : public testing::Test { On 2015/10/21 at 16:47:56, vasilii wrote: > If it's only about deletion by origin then I'd name the class accordingly. Or you want to move more common tests into PasswordStoreCommonTest? Generally, I consider the class to be a suitable for holding generic tests. I currently don't have any tests planned other than origin-based ones, however, so I followed your advice and renamed it to PasswordStoreOriginTest. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_common_unittest.h:49: PasswordStoreCommonTest() : store_(delegate_.store()) {} On 2015/10/21 at 16:47:56, vasilii wrote: > I'd move the init/deinit to SetUp()/TearDown(). May I ask for your rationale? The Google Test documentation[1] generally speaks in favor of fixture constructors/destructors, and we don't seem to hit any of the exceptional cases. [1]https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/destructor_of_the_test_fixture_or_t https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_common_unittest.h:52: this->store_->Shutdown(); On 2015/10/21 at 16:47:57, vasilii wrote: > I'd separate the logic as following. The delegate is responsible for creating/destroying/initializing the password store. PasswordStoreCommonTest is responsible for the tests only. Moved store destruction into the delegates. Looks like this was the only bit missing in order to follow your suggestion. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_common_unittest.h:57: PasswordStore* store_; On 2015/10/21 at 16:47:57, vasilii wrote: > I think you don't need it as you can retrieve from the delegate. Removed and re-routed through the delegate. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_common_unittest.h:64: const char origin_url[] = "http://foo.example.com"; On 2015/10/21 at 16:47:57, vasilii wrote: > Declare the string as a constant in the beginning of the file. IMHO, it'd make the RemoveLoginsByOriginAndTimeImpl_NonMatchingOrigin test (which requires two differing origins) harder to read because the required test data would be split across the file header (shared origin) and the test implementation (distinct origin). With such few tests at the moment, I'm not sure it's worth it. WDYT? https://codereview.chromium.org/1414463004/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:80: class PasswordStoreDefaultTestDelegateBase { On 2015/10/21 at 16:47:57, vasilii wrote: > Why do you need a base class if it's not used without PasswordStoreDefaultTestDelegate? To have initialize() be called automatically through the base class constructor once any of the two PasswordStoreDefaultTestDelegate constructors is invoked. Otherwise, there would be a chance we'd miss triggering initialization for the existing and any future overloaded constructors. I did remove ~PasswordStoreDefaultTestDelegateBase() though by moving terminate() into PasswordStoreDefaultTestDelegate and calling it from the child class destructor. Also missed to use public inheritance. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:87: base::ScopedTempDir temp_dir_; On 2015/10/21 at 16:47:57, vasilii wrote: > Why is it protected? Because PasswordStoreDefaultTestDelegate::test_login_db_file_path() is accessing it. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:98: }; On 2015/10/21 at 16:47:57, vasilii wrote: > Add noncopyable macros here and to the delegate. Done. Added to the Mac delegate as well. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:115: scoped_refptr<PasswordStoreDefault> store_; On 2015/10/21 at 16:47:57, vasilii wrote: > Member declaration is after methods declaration. Fixed. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:125: base::ThreadTaskRunnerHandle::Get(), On 2015/10/21 at 16:47:57, vasilii wrote: > Should you use |message_loop_|? Otherwise why do you need |message_loop_|. Another case of copying code over from the existing implementation as-is. Is it possibly referenced through base::MessageLoop::current()? TODO: Check if removing makes a difference. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:141: class PasswordStoreDefaultTest : public testing::Test {}; On 2015/10/21 at 16:47:57, vasilii wrote: > You don't need an empty class. Just change TEST_F to TEST. Of course! Done.
I missed to mention one thing: Removing |message_loop_| from PasswordStoreDefaultTestDelegateBase in password_store_default_unittest.cc causes PasswordStoreDefaultTest.NonASCIIData to fail a check: FATAL:thread_task_runner_handle.cc(23)] Check failed: current.
https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:196: base::Thread* thread() { return thread_.get(); } On 2015/11/17 23:10:55, Timo Reimann wrote: > On 2015/10/21 at 16:47:56, vasilii wrote: > > Why is it needed? > > PasswordStoreMacTest.ImportFromKeychain, > PasswordStoreMacTest.ImportFederatedFromLockedKeychain, and > PasswordStoreMacTest.ImportFromLockedKeychainError all post > PasswordStoreMac::ImportFromKeychain to the thread's task runner during the > respective tests. You can expose a task runner, not a thread. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:248: if (!store_) On 2015/11/17 23:10:55, Timo Reimann wrote: > On 2015/10/21 at 16:47:56, vasilii wrote: > > It can't be true. > > I agree, unless there's a bug where store is attempted to be deleted twice. > > Should we completely drop the if-clause or replace it by a DCHECK, just to be > safe and catch potential bugs early on? Just drop. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_common_unittest.h (right): https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_common_unittest.h:49: PasswordStoreCommonTest() : store_(delegate_.store()) {} On 2015/11/17 23:10:55, Timo Reimann wrote: > On 2015/10/21 at 16:47:56, vasilii wrote: > > I'd move the init/deinit to SetUp()/TearDown(). > > May I ask for your rationale? The Google Test documentation[1] generally speaks > in favor of fixture constructors/destructors, and we don't seem to hit any of > the exceptional cases. > > [1]https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/destructor_of_the_test_fixture_or_t Acknowledged. https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_common_unittest.h:64: const char origin_url[] = "http://foo.example.com"; On 2015/11/17 23:10:55, Timo Reimann wrote: > On 2015/10/21 at 16:47:57, vasilii wrote: > > Declare the string as a constant in the beginning of the file. > > IMHO, it'd make the RemoveLoginsByOriginAndTimeImpl_NonMatchingOrigin test > (which requires two differing origins) harder to read because the required test > data would be split across the file header (shared origin) and the test > implementation (distinct origin). With such few tests at the moment, I'm not > sure it's worth it. WDYT? Acknowledged. https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac.h (right): https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.h:26: class Origin; It's already present in PasswordStore. https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:189: content::TestBrowserThread ui_thread_; It's deprecated. Use TestBrowserThreadBundle. It can also be used to get rid of |thread_ and |message_loop_| https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:227: make_scoped_ptr<AppleKeychain>(new MockAppleKeychain)); I think it should work without the type specifier. https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:248: // static https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:1354: PasswordStoreMac* store_; Remove https://codereview.chromium.org/1414463004/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager_test_utils.cc (right): https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_test_utils.cc:97: // Explicit out-of-line constructor/destructor required. It's a style rule, no need for clarifications. Add a blank line between the definitions. https://codereview.chromium.org/1414463004/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager_test_utils.h (right): https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_test_utils.h:83: ~MockPasswordStoreObserver(); override https://codereview.chromium.org/1414463004/diff/40001/components/password_man... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:80: class PasswordStoreDefaultTestDelegateBase { Why do you need a base class if it's unused. https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:85: base::ScopedTempDir temp_dir_; Should be private. https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:88: void initialize(); Get rid of the method by inlining. https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:110: void terminate(); Also inline https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:143: store_->Shutdown(); First Shutdown, then everything else.
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414463004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414463004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2015/11/18 at 16:34:24, commit-bot wrote: > Dry run: Try jobs failed on following builders: > android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) > android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) > android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) > android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) > android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) > cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) > chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) > chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) > chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) > mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) My branch is in desperate need of update-rebase; will do after I have addressed your latest comments.
Addressed the majority of remaining comments. Only a few more left. I also fixed all cpplint errors and warnings mentioned for the files in question. Will rebase and push during the day so that we can try-build. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:196: base::Thread* thread() { return thread_.get(); } On 2015/11/18 at 16:27:42, vasilii wrote: > On 2015/11/17 23:10:55, Timo Reimann wrote: > > On 2015/10/21 at 16:47:56, vasilii wrote: > > > Why is it needed? > > > > PasswordStoreMacTest.ImportFromKeychain, > > PasswordStoreMacTest.ImportFederatedFromLockedKeychain, and > > PasswordStoreMacTest.ImportFromLockedKeychainError all post > > PasswordStoreMac::ImportFromKeychain to the thread's task runner during the > > respective tests. > > You can expose a task runner, not a thread. I use ThreadTaskRunnerHandle now. Hope that's the right approach. https://codereview.chromium.org/1414463004/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac_unittest.cc:248: if (!store_) On 2015/11/18 at 16:27:42, vasilii wrote: > On 2015/11/17 23:10:55, Timo Reimann wrote: > > On 2015/10/21 at 16:47:56, vasilii wrote: > > > It can't be true. > > > > I agree, unless there's a bug where store is attempted to be deleted twice. > > > > Should we completely drop the if-clause or replace it by a DCHECK, just to be > > safe and catch potential bugs early on? > > Just drop. Done. https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac.h (right): https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.h:26: class Origin; On 2015/11/18 at 16:27:42, vasilii wrote: > It's already present in PasswordStore. Removed. https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:189: content::TestBrowserThread ui_thread_; On 2015/11/18 at 16:27:42, vasilii wrote: > It's deprecated. Use TestBrowserThreadBundle. It can also be used to get rid of |thread_ and |message_loop_| Okay, done. Apart from adding TestBrowserThreadBundle, I also replaced all references to threads by ThreadTaskRunnerHandle. Please have a look and let me know if the approach is correct. https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:227: make_scoped_ptr<AppleKeychain>(new MockAppleKeychain)); On 2015/11/18 at 16:27:42, vasilii wrote: > I think it should work without the type specifier. It does. Removed. https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:248: On 2015/11/18 at 16:27:42, vasilii wrote: > // static Done. https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:1354: PasswordStoreMac* store_; On 2015/11/18 at 16:27:42, vasilii wrote: > Remove Done (and replaced all references to store_ by delegate_.store()). I also removed |store_| and store() from PasswordStoreMacTest and redirected calls through delegate_. However, I had to add a const overload for PasswordStoreMacTestDelegate.store() in order to satisfy login_db()'s constness. https://codereview.chromium.org/1414463004/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager_test_utils.cc (right): https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_test_utils.cc:97: // Explicit out-of-line constructor/destructor required. On 2015/11/18 at 16:27:42, vasilii wrote: > It's a style rule, no need for clarifications. Add a blank line between the definitions. Done. https://codereview.chromium.org/1414463004/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager_test_utils.h (right): https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_test_utils.h:83: ~MockPasswordStoreObserver(); On 2015/11/18 at 16:27:42, vasilii wrote: > override Done. https://codereview.chromium.org/1414463004/diff/40001/components/password_man... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:80: class PasswordStoreDefaultTestDelegateBase { On 2015/11/18 at 16:27:42, vasilii wrote: > Why do you need a base class if it's unused. In order to guarantee invocation of the initialization method. See also my comment in patch set 1: https://codereview.chromium.org/1414463004/diff/1/components/password_manager... https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:85: base::ScopedTempDir temp_dir_; On 2015/11/18 at 16:27:42, vasilii wrote: > Should be private. True if we decide to get rid of PasswordStoreDefaultTestDelegateBase. For now, the member is accessed by the inheriting class PasswordStoreDefaultTestDelegate through its terminate() method. https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:88: void initialize(); On 2015/11/18 at 16:27:42, vasilii wrote: > Get rid of the method by inlining. I defined the method in the class body. In case you meant to inline directly in the constructor: That's not possible because Google Test doesn't allow ASSERT_* statements in constructors and destructors. https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:110: void terminate(); On 2015/11/18 at 16:27:42, vasilii wrote: > Also inline Done; see my comment above though. https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:143: store_->Shutdown(); On 2015/11/18 at 16:27:42, vasilii wrote: > First Shutdown, then everything else. Done.
The CL should try-build now again.
Vasilii, all things should be ready for another look of yours.
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414463004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414463004/80001
https://codereview.chromium.org/1414463004/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:80: class PasswordStoreDefaultTestDelegateBase { On 2015/11/17 23:10:56, Timo Reimann wrote: > On 2015/10/21 at 16:47:57, vasilii wrote: > > Why do you need a base class if it's not used without > PasswordStoreDefaultTestDelegate? > > To have initialize() be called automatically through the base class constructor > once any of the two PasswordStoreDefaultTestDelegate constructors is invoked. > Otherwise, there would be a chance we'd miss triggering initialization for the > existing and any future overloaded constructors. > > I did remove ~PasswordStoreDefaultTestDelegateBase() though by moving > terminate() into PasswordStoreDefaultTestDelegate and calling it from the child > class destructor. Also missed to use public inheritance. Invoke your method from both constructors. Note that a delegating constructor is also allowed in the C++11 style guide. https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:189: content::TestBrowserThread ui_thread_; On 2015/11/18 23:42:15, Timo Reimann wrote: > On 2015/11/18 at 16:27:42, vasilii wrote: > > It's deprecated. Use TestBrowserThreadBundle. It can also be used to get rid > of |thread_ and |message_loop_| > > Okay, done. Apart from adding TestBrowserThreadBundle, I also replaced all > references to threads by ThreadTaskRunnerHandle. Please have a look and let me > know if the approach is correct. In the implementation you made the test single-threaded because you don't set up a real background thread while constructing TestBrowserThreadBundle. As I already said I'm fine with that if you don't touch PasswordStoreMacTest. Otherwise, you should set up a second thread. FYI, this class is deprecated and will be ultimately removed. https://codereview.chromium.org/1414463004/diff/40001/components/password_man... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:80: class PasswordStoreDefaultTestDelegateBase { On 2015/11/18 23:42:15, Timo Reimann wrote: > On 2015/11/18 at 16:27:42, vasilii wrote: > > Why do you need a base class if it's unused. > > In order to guarantee invocation of the initialization method. See also my > comment in patch set 1: > https://codereview.chromium.org/1414463004/diff/1/components/password_manager... Very technical reason. PasswordStoreDefaultTestDelegateBase should be removed. I replied to your previous comment. https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:110: void terminate(); On 2015/11/18 23:42:15, Timo Reimann wrote: > On 2015/11/18 at 16:27:42, vasilii wrote: > > Also inline > > Done; see my comment above though. I meant get rid of the method by inlining it. https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac.h (right): https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.h:8: #include <string> You don't use string in the new method. https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:185: PasswordStoreMac* store() const { return store_.get(); } What is the point of having the const method? https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:221: ASSERT_TRUE(base::ThreadTaskRunnerHandle::Get()->PostTask( All this PostTasks make no sense in a single threaded environment. You post to the current thread. https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_store_proxy_mac.cc (right): https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_store_proxy_mac.cc:5: #include <string> Remove
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
https://codereview.chromium.org/1414463004/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:80: class PasswordStoreDefaultTestDelegateBase { On 2015/11/23 at 15:04:55, vasilii wrote: > On 2015/11/17 23:10:56, Timo Reimann wrote: > > On 2015/10/21 at 16:47:57, vasilii wrote: > > > Why do you need a base class if it's not used without > > PasswordStoreDefaultTestDelegate? > > > > To have initialize() be called automatically through the base class constructor > > once any of the two PasswordStoreDefaultTestDelegate constructors is invoked. > > Otherwise, there would be a chance we'd miss triggering initialization for the > > existing and any future overloaded constructors. > > > > I did remove ~PasswordStoreDefaultTestDelegateBase() though by moving > > terminate() into PasswordStoreDefaultTestDelegate and calling it from the child > > class destructor. Also missed to use public inheritance. > > Invoke your method from both constructors. Note that a delegating constructor is also allowed in the C++11 style guide. Done. The downside now is that I cannot set |store_| in the initializer list anymore because temp_dir_.CreateUniqueTempDir() needs to be called first. A delegating constructor did not seem to help here either. (On the plus side, I could now remove PasswordStoreDefaultTestDelegate::CreateInitializedStore()). https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:189: content::TestBrowserThread ui_thread_; On 2015/11/23 at 15:04:56, vasilii wrote: > On 2015/11/18 23:42:15, Timo Reimann wrote: > > On 2015/11/18 at 16:27:42, vasilii wrote: > > > It's deprecated. Use TestBrowserThreadBundle. It can also be used to get rid > > of |thread_ and |message_loop_| > > > > Okay, done. Apart from adding TestBrowserThreadBundle, I also replaced all > > references to threads by ThreadTaskRunnerHandle. Please have a look and let me > > know if the approach is correct. > > In the implementation you made the test single-threaded because you don't set up a real background thread while constructing TestBrowserThreadBundle. As I already said I'm fine with that if you don't touch PasswordStoreMacTest. Otherwise, you should set up a second thread. > FYI, this class is deprecated and will be ultimately removed. This part was and still is somewhat unclear to me: As I needed to refactor PasswordStoreMacTest in order to break out PasswordStoreMacTestDelegate, it seems impossible to keep the former completely unchanged. So as I understand, I have no other option but to set up a second thread if I want to stick to the delegate (which is a requirement for the type-parameterized tests in PasswordStoreOriginTest). What I did now was enable the IO_MAINLOOP option on the TestBrowserThreadBundle instance and post all tasks to the IO thread. (I tried doing the same on the REAL_DB_THREAD but that seg-faulted miserably.) This was my best guess on how to use TestBrowserThreadBundle in order to create a separate thread. Please let me know if this is the right way or not. Alternatively, will the deprecation of this class simplify things somehow? My understanding is that SimplePasswordStoreMac (which is implemented in terms of PasswordStoreDefault) will take over in the future. Does this mean we could possibly dump this CL altogether (assuming my feature won't need to ship before PasswordStoreMac's deprecation)? (In case the latter is true, please still let me know how to create a second thread properly . I'd like to understand how things work.) https://codereview.chromium.org/1414463004/diff/40001/components/password_man... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:80: class PasswordStoreDefaultTestDelegateBase { On 2015/11/23 at 15:04:56, vasilii wrote: > On 2015/11/18 23:42:15, Timo Reimann wrote: > > On 2015/11/18 at 16:27:42, vasilii wrote: > > > Why do you need a base class if it's unused. > > > > In order to guarantee invocation of the initialization method. See also my > > comment in patch set 1: > > https://codereview.chromium.org/1414463004/diff/1/components/password_manager... > > Very technical reason. PasswordStoreDefaultTestDelegateBase should be removed. I replied to your previous comment. I removed it. https://codereview.chromium.org/1414463004/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:110: void terminate(); On 2015/11/23 at 15:04:56, vasilii wrote: > On 2015/11/18 23:42:15, Timo Reimann wrote: > > On 2015/11/18 at 16:27:42, vasilii wrote: > > > Also inline > > > > Done; see my comment above though. > > I meant get rid of the method by inlining it. As mentioned with initialize(), you cannot call Google Test ASSERT_* statements in constructors or destructors, so I cannot inline the method in ~PasswordStoreDefaultTestDelegate(). https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac.h (right): https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.h:8: #include <string> On 2015/11/23 at 15:04:56, vasilii wrote: > You don't use string in the new method. This is a fix to a (pre-existing) cpplint complaint stemming from the fact that ReportMetricsImpl() takes an std::string ("Add #include <string> for string [build/include_what_you_use]"). https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:185: PasswordStoreMac* store() const { return store_.get(); } On 2015/11/23 at 15:04:56, vasilii wrote: > What is the point of having the const method? As mentioned before, "I had to add a const overload for PasswordStoreMacTestDelegate.store() in order to satisfy login_db()'s constness". https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:221: ASSERT_TRUE(base::ThreadTaskRunnerHandle::Get()->PostTask( On 2015/11/23 15:04:56, vasilii wrote: > All this PostTasks make no sense in a single threaded environment. You post to > the current thread. I updated it, hopefully for the better. https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_store_proxy_mac.cc (right): https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_store_proxy_mac.cc:5: #include <string> On 2015/11/23 at 15:04:56, vasilii wrote: > Remove Again, a cpplint fix for a pre-existing issue.
https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:189: content::TestBrowserThread ui_thread_; On 2015/11/24 02:36:09, Timo Reimann wrote: > On 2015/11/23 at 15:04:56, vasilii wrote: > > On 2015/11/18 23:42:15, Timo Reimann wrote: > > > On 2015/11/18 at 16:27:42, vasilii wrote: > > > > It's deprecated. Use TestBrowserThreadBundle. It can also be used to get > rid > > > of |thread_ and |message_loop_| > > > > > > Okay, done. Apart from adding TestBrowserThreadBundle, I also replaced all > > > references to threads by ThreadTaskRunnerHandle. Please have a look and let > me > > > know if the approach is correct. > > > > In the implementation you made the test single-threaded because you don't set > up a real background thread while constructing TestBrowserThreadBundle. As I > already said I'm fine with that if you don't touch PasswordStoreMacTest. > Otherwise, you should set up a second thread. > > FYI, this class is deprecated and will be ultimately removed. > > This part was and still is somewhat unclear to me: As I needed to refactor > PasswordStoreMacTest in order to break out PasswordStoreMacTestDelegate, it > seems impossible to keep the former completely unchanged. So as I understand, I > have no other option but to set up a second thread if I want to stick to the > delegate (which is a requirement for the type-parameterized tests in > PasswordStoreOriginTest). > > What I did now was enable the IO_MAINLOOP option on the TestBrowserThreadBundle > instance and post all tasks to the IO thread. (I tried doing the same on the > REAL_DB_THREAD but that seg-faulted miserably.) This was my best guess on how to > use TestBrowserThreadBundle in order to create a separate thread. Please let me > know if this is the right way or not. > > Alternatively, will the deprecation of this class simplify things somehow? My > understanding is that SimplePasswordStoreMac (which is implemented in terms of > PasswordStoreDefault) will take over in the future. Does this mean we could > possibly dump this CL altogether (assuming my feature won't need to ship before > PasswordStoreMac's deprecation)? > > (In case the latter is true, please still let me know how to create a second > thread properly . I'd like to understand how things work.) You don't have to refactor PasswordStoreMacTest. You could just write the delegate and instantiate your tests. That's it. If you want to refactor then you need to create a TestBrowserThreadBundle with REAL_FILE_THREAD (for example) flag. You can post to that thread BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, callback); or you can get a task runner GetMessageLoopProxyForThread(BrowserThread::FILE). We don't know when the class will be deleted. It's not happening soon. Thus we need to test your feature. Ideally without putting a lot of efforts. That's why I prefer to repeat basically what you did for PasswordStoreDefault. https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_store_proxy_mac.cc (right): https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_store_proxy_mac.cc:5: #include <string> On 2015/11/24 02:36:09, Timo Reimann wrote: > On 2015/11/23 at 15:04:56, vasilii wrote: > > Remove > > Again, a cpplint fix for a pre-existing issue. The class header should always be the first include in cc. https://codereview.chromium.org/1414463004/diff/120001/chrome/browser/passwor... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/120001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:206: : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) { By passing IO_MAINLOOP you still create a single-threaded environment but the thread is the IO thread (not UI). Basically it's the same as if you pass DEFAULT. https://codereview.chromium.org/1414463004/diff/120001/components/password_ma... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:86: void terminate() { Should be private and start with a capital letter. Same for init. https://codereview.chromium.org/1414463004/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:94: void FinishAsyncProcessing() { base::MessageLoop::current()->RunUntilIdle(); } Break the line. https://codereview.chromium.org/1414463004/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:97: scoped_refptr<PasswordStoreDefault> CreateInitializedStore(); not used https://codereview.chromium.org/1414463004/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:102: void initialize() { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); } Make both init/terminate out of line.
https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:189: content::TestBrowserThread ui_thread_; On 2015/11/24 18:14:54, vasilii wrote: > On 2015/11/24 02:36:09, Timo Reimann wrote: > > On 2015/11/23 at 15:04:56, vasilii wrote: > > > On 2015/11/18 23:42:15, Timo Reimann wrote: > > > > On 2015/11/18 at 16:27:42, vasilii wrote: > > > > > It's deprecated. Use TestBrowserThreadBundle. It can also be used to get > > rid > > > > of |thread_ and |message_loop_| > > > > > > > > Okay, done. Apart from adding TestBrowserThreadBundle, I also replaced all > > > > references to threads by ThreadTaskRunnerHandle. Please have a look and > let > > me > > > > know if the approach is correct. > > > > > > In the implementation you made the test single-threaded because you don't > set > > up a real background thread while constructing TestBrowserThreadBundle. As I > > already said I'm fine with that if you don't touch PasswordStoreMacTest. > > Otherwise, you should set up a second thread. > > > FYI, this class is deprecated and will be ultimately removed. > > > > This part was and still is somewhat unclear to me: As I needed to refactor > > PasswordStoreMacTest in order to break out PasswordStoreMacTestDelegate, it > > seems impossible to keep the former completely unchanged. So as I understand, > I > > have no other option but to set up a second thread if I want to stick to the > > delegate (which is a requirement for the type-parameterized tests in > > PasswordStoreOriginTest). > > > > What I did now was enable the IO_MAINLOOP option on the > TestBrowserThreadBundle > > instance and post all tasks to the IO thread. (I tried doing the same on the > > REAL_DB_THREAD but that seg-faulted miserably.) This was my best guess on how > to > > use TestBrowserThreadBundle in order to create a separate thread. Please let > me > > know if this is the right way or not. > > > > Alternatively, will the deprecation of this class simplify things somehow? My > > understanding is that SimplePasswordStoreMac (which is implemented in terms of > > PasswordStoreDefault) will take over in the future. Does this mean we could > > possibly dump this CL altogether (assuming my feature won't need to ship > before > > PasswordStoreMac's deprecation)? > > > > (In case the latter is true, please still let me know how to create a second > > thread properly . I'd like to understand how things work.) > > You don't have to refactor PasswordStoreMacTest. You could just write the > delegate and instantiate your tests. That's it. Without any refactoring permitted, wouldn't that prevent usage of the Mac delegate as a typed test in PasswordStoreOriginTest? As of now, all parameterized test delegates need to adhere to a certain interface and encapsulate the store life cycle. For instance, they must support a FinishAsyncProcessing() method. I couldn't provide that for PasswordStoreMacTestDelegate if I wasn't allowed to move (refactor) it from PasswordStoreMacTest into the delegate. > If you want to refactor then you need to create a TestBrowserThreadBundle with > REAL_FILE_THREAD (for example) flag. You can post to that thread > BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, callback); or you can > get a task runner GetMessageLoopProxyForThread(BrowserThread::FILE). > We don't know when the class will be deleted. It's not happening soon. Thus we > need to test your feature. Ideally without putting a lot of efforts. That's why > I prefer to repeat basically what you did for PasswordStoreDefault. I'm using REAL_FILE_THREAD now. I think it's not the worst choice after all as we've made a few improvements to PasswordStoreMacTest along the way. These might come handy even if the class will be removed at some remote point in the future. https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_store_proxy_mac.cc (right): https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_store_proxy_mac.cc:5: #include <string> On 2015/11/24 18:14:54, vasilii wrote: > On 2015/11/24 02:36:09, Timo Reimann wrote: > > On 2015/11/23 at 15:04:56, vasilii wrote: > > > Remove > > > > Again, a cpplint fix for a pre-existing issue. > > The class header should always be the first include in cc. Reordered. https://codereview.chromium.org/1414463004/diff/120001/chrome/browser/passwor... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/120001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:206: : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) { On 2015/11/24 18:14:55, vasilii wrote: > By passing IO_MAINLOOP you still create a single-threaded environment but the > thread is the IO thread (not UI). Basically it's the same as if you pass > DEFAULT. Gotcha. Replaced by REAL_FILE_THREAD. https://codereview.chromium.org/1414463004/diff/120001/components/password_ma... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:86: void terminate() { On 2015/11/24 18:14:55, vasilii wrote: > Should be private and start with a capital letter. Same for init. Done. https://codereview.chromium.org/1414463004/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:94: void FinishAsyncProcessing() { base::MessageLoop::current()->RunUntilIdle(); } On 2015/11/24 18:14:55, vasilii wrote: > Break the line. The given formatting was the result of running cpplint/git cl format: If I break the line, cpplint reverts to a single line as it fits within the 80 chars limit. Thus, It seems to adhere to the preferred code style. https://codereview.chromium.org/1414463004/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:97: scoped_refptr<PasswordStoreDefault> CreateInitializedStore(); On 2015/11/24 18:14:55, vasilii wrote: > not used Removed. https://codereview.chromium.org/1414463004/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:102: void initialize() { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); } On 2015/11/24 18:14:55, vasilii wrote: > Make both init/terminate out of line. See my comment above regarding FinishAsyncProcessing(): cpplint seems to prefer it this way.
https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:189: content::TestBrowserThread ui_thread_; On 2015/11/24 23:06:02, Timo Reimann wrote: > On 2015/11/24 18:14:54, vasilii wrote: > > On 2015/11/24 02:36:09, Timo Reimann wrote: > > > On 2015/11/23 at 15:04:56, vasilii wrote: > > > > On 2015/11/18 23:42:15, Timo Reimann wrote: > > > > > On 2015/11/18 at 16:27:42, vasilii wrote: > > > > > > It's deprecated. Use TestBrowserThreadBundle. It can also be used to > get > > > rid > > > > > of |thread_ and |message_loop_| > > > > > > > > > > Okay, done. Apart from adding TestBrowserThreadBundle, I also replaced > all > > > > > references to threads by ThreadTaskRunnerHandle. Please have a look and > > let > > > me > > > > > know if the approach is correct. > > > > > > > > In the implementation you made the test single-threaded because you don't > > set > > > up a real background thread while constructing TestBrowserThreadBundle. As I > > > already said I'm fine with that if you don't touch PasswordStoreMacTest. > > > Otherwise, you should set up a second thread. > > > > FYI, this class is deprecated and will be ultimately removed. > > > > > > This part was and still is somewhat unclear to me: As I needed to refactor > > > PasswordStoreMacTest in order to break out PasswordStoreMacTestDelegate, it > > > seems impossible to keep the former completely unchanged. So as I > understand, > > I > > > have no other option but to set up a second thread if I want to stick to the > > > delegate (which is a requirement for the type-parameterized tests in > > > PasswordStoreOriginTest). > > > > > > What I did now was enable the IO_MAINLOOP option on the > > TestBrowserThreadBundle > > > instance and post all tasks to the IO thread. (I tried doing the same on the > > > REAL_DB_THREAD but that seg-faulted miserably.) This was my best guess on > how > > to > > > use TestBrowserThreadBundle in order to create a separate thread. Please let > > me > > > know if this is the right way or not. > > > > > > Alternatively, will the deprecation of this class simplify things somehow? > My > > > understanding is that SimplePasswordStoreMac (which is implemented in terms > of > > > PasswordStoreDefault) will take over in the future. Does this mean we could > > > possibly dump this CL altogether (assuming my feature won't need to ship > > before > > > PasswordStoreMac's deprecation)? > > > > > > (In case the latter is true, please still let me know how to create a second > > > thread properly . I'd like to understand how things work.) > > > > You don't have to refactor PasswordStoreMacTest. You could just write the > > delegate and instantiate your tests. That's it. > > Without any refactoring permitted, wouldn't that prevent usage of the Mac > delegate as a typed test in PasswordStoreOriginTest? As of now, all > parameterized test delegates need to adhere to a certain interface and > encapsulate the store life cycle. For instance, they must support a > FinishAsyncProcessing() method. I couldn't provide that for > PasswordStoreMacTestDelegate if I wasn't allowed to move (refactor) it from > PasswordStoreMacTest into the delegate. > > > If you want to refactor then you need to create a TestBrowserThreadBundle with > > REAL_FILE_THREAD (for example) flag. You can post to that thread > > BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, callback); or you can > > get a task runner GetMessageLoopProxyForThread(BrowserThread::FILE). > > We don't know when the class will be deleted. It's not happening soon. Thus > we > > need to test your feature. Ideally without putting a lot of efforts. That's > why > > I prefer to repeat basically what you did for PasswordStoreDefault. > > I'm using REAL_FILE_THREAD now. I think it's not the worst choice after all as > we've made a few improvements to PasswordStoreMacTest along the way. These might > come handy even if the class will be removed at some remote point in the future. I don't understand your concern. You have PasswordStoreMacTest which works fine. Then you wrote PasswordStoreOriginTest and PasswordStoreMacTestDelegate as a delegate. Why do you feel like you have to change PasswordStoreMacTest? You don't have to move, you can copy what you need. I am suggesting the single-threaded PasswordStoreMacTestDelegate because of simplicity. The current patch set contains quite a few mistakes related to multi-threading. https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:185: PasswordStoreMac* store() const { return store_.get(); } On 2015/11/24 02:36:09, Timo Reimann wrote: > On 2015/11/23 at 15:04:56, vasilii wrote: > > What is the point of having the const method? > > As mentioned before, "I had to add a const overload for > PasswordStoreMacTestDelegate.store() in order to satisfy login_db()'s > constness". Then remove the non-const method. https://codereview.chromium.org/1414463004/diff/120001/components/password_ma... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:102: void initialize() { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); } On 2015/11/24 23:06:02, Timo Reimann wrote: > On 2015/11/24 18:14:55, vasilii wrote: > > Make both init/terminate out of line. > > See my comment above regarding FinishAsyncProcessing(): cpplint seems to prefer > it this way. Then make it out of line. https://codereview.chromium.org/1414463004/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:177: static void FinishAsyncProcessing() { Make it out of line. https://codereview.chromium.org/1414463004/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:180: ASSERT_TRUE(base::ThreadTaskRunnerHandle::Get()->PostTaskAndReply( You don't sync with the IO thread here. You post to your current thread. https://codereview.chromium.org/1414463004/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:206: : thread_bundle_(content::TestBrowserThreadBundle::REAL_IO_THREAD) { You create a real IO thread but use the FILE thread below. https://codereview.chromium.org/1414463004/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:233: base::ThreadTaskRunnerHandle::Get()))); You set the background task runner as a current task runner which means you don't use the second thread at all. https://codereview.chromium.org/1414463004/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:244: // static https://codereview.chromium.org/1414463004/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:1337: PasswordStoreMacTestDelegate& delegate() { return delegate_; } I'd make delegate_ private and expose whatever you need from it as PasswordStoreMacTest methods https://codereview.chromium.org/1414463004/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:1840: base::ThreadTaskRunnerHandle::Get().get(), FROM_HERE, You post to the current thread here. https://codereview.chromium.org/1414463004/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:1871: base::ThreadTaskRunnerHandle::Get().get(), FROM_HERE, Same here. https://codereview.chromium.org/1414463004/diff/140001/components/password_ma... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/140001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:88: void FinishAsyncProcessing() { base::MessageLoop::current()->RunUntilIdle(); } Make it out-of-line. Same for Init/terminate. https://codereview.chromium.org/1414463004/diff/140001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:91: void Initialize() { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); } I'd rename it to something like SetupTempDir
https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:189: content::TestBrowserThread ui_thread_; On 2015/11/25 18:18:39, vasilii wrote: > On 2015/11/24 23:06:02, Timo Reimann wrote: > > On 2015/11/24 18:14:54, vasilii wrote: > > > On 2015/11/24 02:36:09, Timo Reimann wrote: > > > > On 2015/11/23 at 15:04:56, vasilii wrote: > > > > > On 2015/11/18 23:42:15, Timo Reimann wrote: > > > > > > On 2015/11/18 at 16:27:42, vasilii wrote: > > > > > > > It's deprecated. Use TestBrowserThreadBundle. It can also be used to > > get > > > > rid > > > > > > of |thread_ and |message_loop_| > > > > > > > > > > > > Okay, done. Apart from adding TestBrowserThreadBundle, I also replaced > > all > > > > > > references to threads by ThreadTaskRunnerHandle. Please have a look > and > > > let > > > > me > > > > > > know if the approach is correct. > > > > > > > > > > In the implementation you made the test single-threaded because you > don't > > > set > > > > up a real background thread while constructing TestBrowserThreadBundle. As > I > > > > already said I'm fine with that if you don't touch PasswordStoreMacTest. > > > > Otherwise, you should set up a second thread. > > > > > FYI, this class is deprecated and will be ultimately removed. > > > > > > > > This part was and still is somewhat unclear to me: As I needed to refactor > > > > PasswordStoreMacTest in order to break out PasswordStoreMacTestDelegate, > it > > > > seems impossible to keep the former completely unchanged. So as I > > understand, > > > I > > > > have no other option but to set up a second thread if I want to stick to > the > > > > delegate (which is a requirement for the type-parameterized tests in > > > > PasswordStoreOriginTest). > > > > > > > > What I did now was enable the IO_MAINLOOP option on the > > > TestBrowserThreadBundle > > > > instance and post all tasks to the IO thread. (I tried doing the same on > the > > > > REAL_DB_THREAD but that seg-faulted miserably.) This was my best guess on > > how > > > to > > > > use TestBrowserThreadBundle in order to create a separate thread. Please > let > > > me > > > > know if this is the right way or not. > > > > > > > > Alternatively, will the deprecation of this class simplify things somehow? > > My > > > > understanding is that SimplePasswordStoreMac (which is implemented in > terms > > of > > > > PasswordStoreDefault) will take over in the future. Does this mean we > could > > > > possibly dump this CL altogether (assuming my feature won't need to ship > > > before > > > > PasswordStoreMac's deprecation)? > > > > > > > > (In case the latter is true, please still let me know how to create a > second > > > > thread properly . I'd like to understand how things work.) > > > > > > You don't have to refactor PasswordStoreMacTest. You could just write the > > > delegate and instantiate your tests. That's it. > > > > Without any refactoring permitted, wouldn't that prevent usage of the Mac > > delegate as a typed test in PasswordStoreOriginTest? As of now, all > > parameterized test delegates need to adhere to a certain interface and > > encapsulate the store life cycle. For instance, they must support a > > FinishAsyncProcessing() method. I couldn't provide that for > > PasswordStoreMacTestDelegate if I wasn't allowed to move (refactor) it from > > PasswordStoreMacTest into the delegate. > > > > > If you want to refactor then you need to create a TestBrowserThreadBundle > with > > > REAL_FILE_THREAD (for example) flag. You can post to that thread > > > BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, callback); or you > can > > > get a task runner GetMessageLoopProxyForThread(BrowserThread::FILE). > > > We don't know when the class will be deleted. It's not happening soon. Thus > > we > > > need to test your feature. Ideally without putting a lot of efforts. That's > > why > > > I prefer to repeat basically what you did for PasswordStoreDefault. > > > > I'm using REAL_FILE_THREAD now. I think it's not the worst choice after all as > > we've made a few improvements to PasswordStoreMacTest along the way. These > might > > come handy even if the class will be removed at some remote point in the > future. > > I don't understand your concern. You have PasswordStoreMacTest which works fine. > Then you wrote PasswordStoreOriginTest and PasswordStoreMacTestDelegate as a > delegate. Why do you feel like you have to change PasswordStoreMacTest? You > don't have to move, you can copy what you need. > I am suggesting the single-threaded PasswordStoreMacTestDelegate because of > simplicity. The current patch set contains quite a few mistakes related to > multi-threading. I believe I'm starting to understand where you're heading: Create the delegate and re-implement/copy whatever functionality is necessary to support store management. I didn't consider this because it will create a bit of duplicated code which I tried to avoid (such as the Initialize, Terminate, and FinishAsyncProcessing methods). Is this what you mean?
https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:189: content::TestBrowserThread ui_thread_; On 2015/11/25 18:47:32, Timo Reimann wrote: > On 2015/11/25 18:18:39, vasilii wrote: > > On 2015/11/24 23:06:02, Timo Reimann wrote: > > > On 2015/11/24 18:14:54, vasilii wrote: > > > > On 2015/11/24 02:36:09, Timo Reimann wrote: > > > > > On 2015/11/23 at 15:04:56, vasilii wrote: > > > > > > On 2015/11/18 23:42:15, Timo Reimann wrote: > > > > > > > On 2015/11/18 at 16:27:42, vasilii wrote: > > > > > > > > It's deprecated. Use TestBrowserThreadBundle. It can also be used > to > > > get > > > > > rid > > > > > > > of |thread_ and |message_loop_| > > > > > > > > > > > > > > Okay, done. Apart from adding TestBrowserThreadBundle, I also > replaced > > > all > > > > > > > references to threads by ThreadTaskRunnerHandle. Please have a look > > and > > > > let > > > > > me > > > > > > > know if the approach is correct. > > > > > > > > > > > > In the implementation you made the test single-threaded because you > > don't > > > > set > > > > > up a real background thread while constructing TestBrowserThreadBundle. > As > > I > > > > > already said I'm fine with that if you don't touch PasswordStoreMacTest. > > > > > Otherwise, you should set up a second thread. > > > > > > FYI, this class is deprecated and will be ultimately removed. > > > > > > > > > > This part was and still is somewhat unclear to me: As I needed to > refactor > > > > > PasswordStoreMacTest in order to break out PasswordStoreMacTestDelegate, > > it > > > > > seems impossible to keep the former completely unchanged. So as I > > > understand, > > > > I > > > > > have no other option but to set up a second thread if I want to stick to > > the > > > > > delegate (which is a requirement for the type-parameterized tests in > > > > > PasswordStoreOriginTest). > > > > > > > > > > What I did now was enable the IO_MAINLOOP option on the > > > > TestBrowserThreadBundle > > > > > instance and post all tasks to the IO thread. (I tried doing the same on > > the > > > > > REAL_DB_THREAD but that seg-faulted miserably.) This was my best guess > on > > > how > > > > to > > > > > use TestBrowserThreadBundle in order to create a separate thread. Please > > let > > > > me > > > > > know if this is the right way or not. > > > > > > > > > > Alternatively, will the deprecation of this class simplify things > somehow? > > > My > > > > > understanding is that SimplePasswordStoreMac (which is implemented in > > terms > > > of > > > > > PasswordStoreDefault) will take over in the future. Does this mean we > > could > > > > > possibly dump this CL altogether (assuming my feature won't need to ship > > > > before > > > > > PasswordStoreMac's deprecation)? > > > > > > > > > > (In case the latter is true, please still let me know how to create a > > second > > > > > thread properly . I'd like to understand how things work.) > > > > > > > > You don't have to refactor PasswordStoreMacTest. You could just write the > > > > delegate and instantiate your tests. That's it. > > > > > > Without any refactoring permitted, wouldn't that prevent usage of the Mac > > > delegate as a typed test in PasswordStoreOriginTest? As of now, all > > > parameterized test delegates need to adhere to a certain interface and > > > encapsulate the store life cycle. For instance, they must support a > > > FinishAsyncProcessing() method. I couldn't provide that for > > > PasswordStoreMacTestDelegate if I wasn't allowed to move (refactor) it from > > > PasswordStoreMacTest into the delegate. > > > > > > > If you want to refactor then you need to create a TestBrowserThreadBundle > > with > > > > REAL_FILE_THREAD (for example) flag. You can post to that thread > > > > BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, callback); or you > > can > > > > get a task runner GetMessageLoopProxyForThread(BrowserThread::FILE). > > > > We don't know when the class will be deleted. It's not happening soon. > Thus > > > we > > > > need to test your feature. Ideally without putting a lot of efforts. > That's > > > why > > > > I prefer to repeat basically what you did for PasswordStoreDefault. > > > > > > I'm using REAL_FILE_THREAD now. I think it's not the worst choice after all > as > > > we've made a few improvements to PasswordStoreMacTest along the way. These > > might > > > come handy even if the class will be removed at some remote point in the > > future. > > > > I don't understand your concern. You have PasswordStoreMacTest which works > fine. > > Then you wrote PasswordStoreOriginTest and PasswordStoreMacTestDelegate as a > > delegate. Why do you feel like you have to change PasswordStoreMacTest? You > > don't have to move, you can copy what you need. > > I am suggesting the single-threaded PasswordStoreMacTestDelegate because of > > simplicity. The current patch set contains quite a few mistakes related to > > multi-threading. > > I believe I'm starting to understand where you're heading: Create the delegate > and re-implement/copy whatever functionality is necessary to support store > management. I didn't consider this because it will create a bit of duplicated > code which I tried to avoid (such as the Initialize, Terminate, and > FinishAsyncProcessing methods). Is this what you mean? I followed your advice and build a simplified PasswordStoreMacDelegate. Turned out it contains much less duplicate code than I thought due to its single-threaded nature -- apologies for not realizing this earlier. The multi-threading-related problems you addressed in the other comments should all be obsolete now. The changes to PasswordStoreMacTest have been reverted (except for the minor MockPasswordStoreObserver update). Let me know what you think. https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:185: PasswordStoreMac* store() const { return store_.get(); } On 2015/11/25 18:18:39, vasilii wrote: > On 2015/11/24 02:36:09, Timo Reimann wrote: > > On 2015/11/23 at 15:04:56, vasilii wrote: > > > What is the point of having the const method? > > > > As mentioned before, "I had to add a const overload for > > PasswordStoreMacTestDelegate.store() in order to satisfy login_db()'s > > constness". > > Then remove the non-const method. Obsoleted by the PasswordStoreMacDelegate rewrite. https://codereview.chromium.org/1414463004/diff/120001/components/password_ma... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/120001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:102: void initialize() { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); } On 2015/11/25 18:18:39, vasilii wrote: > On 2015/11/24 23:06:02, Timo Reimann wrote: > > On 2015/11/24 18:14:55, vasilii wrote: > > > Make both init/terminate out of line. > > > > See my comment above regarding FinishAsyncProcessing(): cpplint seems to > prefer > > it this way. > > Then make it out of line. My bad: I didn't really know what out-of-line means until now and misunderstood. Fixed. https://codereview.chromium.org/1414463004/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:177: static void FinishAsyncProcessing() { On 2015/11/25 18:18:39, vasilii wrote: > Make it out of line. Done. https://codereview.chromium.org/1414463004/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:244: On 2015/11/25 18:18:39, vasilii wrote: > // static I decided to inline the method in the (new) PasswordStoreMacTestDelegate::Initialize() instead. https://codereview.chromium.org/1414463004/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:1337: PasswordStoreMacTestDelegate& delegate() { return delegate_; } On 2015/11/25 18:18:39, vasilii wrote: > I'd make delegate_ private and expose whatever you need from it as > PasswordStoreMacTest methods Obsolete now as PasswordStoreMacTest doesn't use the delegate anymore. https://codereview.chromium.org/1414463004/diff/140001/components/password_ma... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/140001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:88: void FinishAsyncProcessing() { base::MessageLoop::current()->RunUntilIdle(); } On 2015/11/25 18:18:39, vasilii wrote: > Make it out-of-line. Same for Init/terminate. Done. https://codereview.chromium.org/1414463004/diff/140001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:91: void Initialize() { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); } On 2015/11/25 18:18:39, vasilii wrote: > I'd rename it to something like SetupTempDir Done. Also renamed Terminate to ClosePasswordStore in order to be consistent with PasswordStoreMacDelegate.
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414463004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414463004/180001
https://codereview.chromium.org/1414463004/diff/180001/chrome/browser/passwor... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:202: login_db_.reset(); You don't need it. Also I think that |login_db_| should outlive |store_|. Thus, the default destruction order is fine. https://codereview.chromium.org/1414463004/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:221: base::FilePath PasswordStoreMacTestDelegate::test_login_db_file_path() const { Declaration order should match definition order. https://codereview.chromium.org/1414463004/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:236: namespace password_manager { We shouldn't declare namespace password_manager in chrome/. https://codereview.chromium.org/1414463004/diff/180001/components/password_ma... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/180001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:80: class PasswordStoreDefaultTestDelegate { You can move it to the anonymous namespace https://codereview.chromium.org/1414463004/diff/180001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:149: void PasswordStoreDefaultTestDelegate::FinishAsyncProcessing() { Declaration order and definition order should match each other. https://codereview.chromium.org/1414463004/diff/180001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:254: make_scoped_ptr<LoginDatabase>(new BadLoginDatabase)); It should work without the type.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
Comments addressed. One last question regarding the password_manager namespace left. Do you know why the Android build fails? I see a bunch of CommandTimeoutErrors in the logs and apparently unrelated tests failing. Flakiness? Will update-rebase again just to be sure. https://codereview.chromium.org/1414463004/diff/180001/chrome/browser/passwor... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:202: login_db_.reset(); On 2015/11/26 11:03:38, vasilii wrote: > You don't need it. Also I think that |login_db_| should outlive |store_|. Thus, > the default destruction order is fine. Removed. https://codereview.chromium.org/1414463004/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:221: base::FilePath PasswordStoreMacTestDelegate::test_login_db_file_path() const { On 2015/11/26 11:03:38, vasilii wrote: > Declaration order should match definition order. Updated. https://codereview.chromium.org/1414463004/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:236: namespace password_manager { On 2015/11/26 11:03:38, vasilii wrote: > We shouldn't declare namespace password_manager in chrome/. I tried to keep the typed test case instantiations out of the password_manager namespace before already but kept running into Google Test-related compiler errors I couldn't resolve: Trying without gives ../../chrome/browser/password_manager/password_store_mac_unittest.cc:236:31: use of undeclared identifier 'PasswordStoreOriginTest'; did you mean 'password_manager::PasswordStoreOriginTest'? and fully qualifying as suggested leads to BEGIN-OF-COMPILER-OUTPUT ../../chrome/browser/password_manager/password_store_mac_unittest.cc:235:1: error: use of undeclared identifier 'gtest_Mac_password_manager' INSTANTIATE_TYPED_TEST_CASE_P(Mac, ^ ../../testing/gtest/include/gtest/gtest-typed-test.h:252:8: note: expanded from macro 'INSTANTIATE_TYPED_TEST_CASE_P' bool gtest_##Prefix##_##CaseName GTEST_ATTRIBUTE_UNUSED_ = \ ^ <scratch space>:102:1: note: expanded from here gtest_Mac_password_manager END-OF-COMPILER-OUTPUT Any idea how to resolve this? https://codereview.chromium.org/1414463004/diff/180001/components/password_ma... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/180001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:80: class PasswordStoreDefaultTestDelegate { On 2015/11/26 11:03:38, vasilii wrote: > You can move it to the anonymous namespace Done. https://codereview.chromium.org/1414463004/diff/180001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:149: void PasswordStoreDefaultTestDelegate::FinishAsyncProcessing() { On 2015/11/26 11:03:38, vasilii wrote: > Declaration order and definition order should match each other. Updated. https://codereview.chromium.org/1414463004/diff/180001/components/password_ma... components/password_manager/core/browser/password_store_default_unittest.cc:254: make_scoped_ptr<LoginDatabase>(new BadLoginDatabase)); On 2015/11/26 11:03:38, vasilii wrote: > It should work without the type. Removed.
I think Android is flaky indeed. https://codereview.chromium.org/1414463004/diff/180001/chrome/browser/passwor... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1414463004/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/password_store_mac_unittest.cc:236: namespace password_manager { On 2015/11/26 14:38:29, Timo Reimann wrote: > On 2015/11/26 11:03:38, vasilii wrote: > > We shouldn't declare namespace password_manager in chrome/. > > I tried to keep the typed test case instantiations out of the password_manager > namespace before already but kept running into Google Test-related compiler > errors I couldn't resolve: Trying without gives > > ../../chrome/browser/password_manager/password_store_mac_unittest.cc:236:31: use > of undeclared identifier 'PasswordStoreOriginTest'; did you mean > 'password_manager::PasswordStoreOriginTest'? > > and fully qualifying as suggested leads to > > BEGIN-OF-COMPILER-OUTPUT > ../../chrome/browser/password_manager/password_store_mac_unittest.cc:235:1: > error: use of undeclared identifier 'gtest_Mac_password_manager' > INSTANTIATE_TYPED_TEST_CASE_P(Mac, > ^ > ../../testing/gtest/include/gtest/gtest-typed-test.h:252:8: note: expanded from > macro 'INSTANTIATE_TYPED_TEST_CASE_P' > bool gtest_##Prefix##_##CaseName GTEST_ATTRIBUTE_UNUSED_ = \ > ^ > <scratch space>:102:1: note: expanded from here > gtest_Mac_password_manager > END-OF-COMPILER-OUTPUT > > Any idea how to resolve this? Try to write 'using password_manager::PasswordStoreOriginTest;' and then use the unqualified class name.
On 2015/11/26 14:53:39, vasilii wrote: > I think Android is flaky indeed. > > https://codereview.chromium.org/1414463004/diff/180001/chrome/browser/passwor... > File chrome/browser/password_manager/password_store_mac_unittest.cc (right): > > https://codereview.chromium.org/1414463004/diff/180001/chrome/browser/passwor... > chrome/browser/password_manager/password_store_mac_unittest.cc:236: namespace > password_manager { > On 2015/11/26 14:38:29, Timo Reimann wrote: > > On 2015/11/26 11:03:38, vasilii wrote: > > > We shouldn't declare namespace password_manager in chrome/. > > > > I tried to keep the typed test case instantiations out of the password_manager > > namespace before already but kept running into Google Test-related compiler > > errors I couldn't resolve: Trying without gives > > > > ../../chrome/browser/password_manager/password_store_mac_unittest.cc:236:31: > use > > of undeclared identifier 'PasswordStoreOriginTest'; did you mean > > 'password_manager::PasswordStoreOriginTest'? > > > > and fully qualifying as suggested leads to > > > > BEGIN-OF-COMPILER-OUTPUT > > ../../chrome/browser/password_manager/password_store_mac_unittest.cc:235:1: > > error: use of undeclared identifier 'gtest_Mac_password_manager' > > INSTANTIATE_TYPED_TEST_CASE_P(Mac, > > ^ > > ../../testing/gtest/include/gtest/gtest-typed-test.h:252:8: note: expanded > from > > macro 'INSTANTIATE_TYPED_TEST_CASE_P' > > bool gtest_##Prefix##_##CaseName GTEST_ATTRIBUTE_UNUSED_ = \ > > ^ > > <scratch space>:102:1: note: expanded from here > > gtest_Mac_password_manager > > END-OF-COMPILER-OUTPUT > > > > Any idea how to resolve this? > > Try to write 'using password_manager::PasswordStoreOriginTest;' and then use the > unqualified class name. Unfortunately, it still doesn't work: ../../chrome/browser/password_manager/password_store_mac_unittest.cc:236:1: error: use of undeclared identifier 'gtest_case_PasswordStoreOriginTest_'; did you mean 'password_manager::gtest_case_PasswordStoreOriginTest_'? INSTANTIATE_TYPED_TEST_CASE_P(Mac, ^
You can leave the tests in the namespace. https://codereview.chromium.org/1414463004/diff/200001/components/password_ma... File components/password_manager/core/browser/password_store_origin_unittest.h (right): https://codereview.chromium.org/1414463004/diff/200001/components/password_ma... components/password_manager/core/browser/password_store_origin_unittest.h:61: EXPECT_CALL(observer, OnLoginsChanged(ElementsAre(PasswordStoreChange( Move the expectation just before the call which triggers it. Same below.
https://codereview.chromium.org/1414463004/diff/200001/components/password_ma... File components/password_manager/core/browser/password_store_origin_unittest.h (right): https://codereview.chromium.org/1414463004/diff/200001/components/password_ma... components/password_manager/core/browser/password_store_origin_unittest.h:61: EXPECT_CALL(observer, OnLoginsChanged(ElementsAre(PasswordStoreChange( On 2015/11/27 10:41:40, vasilii wrote: > Move the expectation just before the call which triggers it. > Same below. Done (below too).
lgtm
The CQ bit was checked by ttr314@googlemail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414463004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414463004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
I'm not an owner of password_store_mac.cc. Ask vabr@ or mkwst@ to review.
vabr@chromium.org changed reviewers: + vabr@chromium.org
//chrome/browser/password_manager LGTM
On 2015/11/27 12:16:42, vasilii wrote: > I'm not an owner of password_store_mac.cc. Ask vabr@ or mkwst@ to review. Being fixed in https://codereview.chromium.org/1482713002 :)
On 2015/11/27 12:56:16, vabr (Chromium) wrote: > On 2015/11/27 12:16:42, vasilii wrote: > > I'm not an owner of password_store_mac.cc. Ask vabr@ or mkwst@ to review. > > Being fixed in https://codereview.chromium.org/1482713002 :) Missed to see I needed another owner -- thanks for the prompt response. :-) Let's try this again...
The CQ bit was checked by ttr314@googlemail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414463004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414463004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
On 2015/11/27 16:12:02, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) Not so sure anymore if this is really flakiness since the group of tests failing seems to be the same each time. Apart from rebase-updating, I don't have a clear idea what to do.
On 2015/11/27 16:43:17, Timo Reimann wrote: > On 2015/11/27 16:12:02, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > Not so sure anymore if this is really flakiness since the group of tests failing > seems to be the same each time. > > Apart from rebase-updating, I don't have a clear idea what to do. Looking at https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_..., some PasswordStore-related tests seem to be failing as well.
On 2015/11/27 16:46:01, vabr (Chromium) wrote: > On 2015/11/27 16:43:17, Timo Reimann wrote: > > On 2015/11/27 16:12:02, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > Not so sure anymore if this is really flakiness since the group of tests > failing > > seems to be the same each time. > > > > Apart from rebase-updating, I don't have a clear idea what to do. > > Looking at > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_..., > some PasswordStore-related tests seem to be failing as well. Concretely Default/PasswordStoreOriginTest/0.RemoveLoginsByOriginAndTimeImpl_FittingOriginAndTime
On 2015/11/27 16:43:17, Timo Reimann wrote: > On 2015/11/27 16:12:02, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > Not so sure anymore if this is really flakiness since the group of tests failing > seems to be the same each time. > > Apart from rebase-updating, I don't have a clear idea what to do. There is a timeout. My guess is that it's related to your refactoring of PasswordStoreDefaultTest. Initialization/deinitialization used to be done in SetUp/TearDown. Now you moved it to the test body.
On 2015/11/27 17:00:04, vasilii wrote: > On 2015/11/27 16:43:17, Timo Reimann wrote: > > On 2015/11/27 16:12:02, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > Not so sure anymore if this is really flakiness since the group of tests > failing > > seems to be the same each time. > > > > Apart from rebase-updating, I don't have a clear idea what to do. > > There is a timeout. My guess is that it's related to your refactoring of > PasswordStoreDefaultTest. Initialization/deinitialization used to be done in > SetUp/TearDown. Now you moved it to the test body. I compared the initialization/termination order: It seems equivalent between the old and the new implementation. One thing noticeable is that the other two Default/PasswordStoreOriginTests (RemoveLoginsByOriginAndTimeImpl_NonMatchingOrigin and RemoveLoginsByOriginAndTimeImpl_NotWithinTimeInterval) run through just fine even though the fixture logic is identical. What makes them different from the failing RemoveLoginsByOriginAndTimeImpl_FittingOriginAndTime test is that they do not actually remove any passwords from the password store. So maybe the failure is related to a side-effect of the actual store removal operation? Is there an easy way to run just the other two tests on the Android bot to validate my assumption? I guess I could create a separate CL without the offending test and let one of you trigger the try bot job, but that's not a fast feedback loop exactly. (Maybe this is the right opportunity for me to ask for try job privileges...)
On 2015/11/27 17:51:29, Timo Reimann wrote: > On 2015/11/27 17:00:04, vasilii wrote: > > On 2015/11/27 16:43:17, Timo Reimann wrote: > > > On 2015/11/27 16:12:02, commit-bot: I haz the power wrote: > > > > Try jobs failed on following builders: > > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > > > Not so sure anymore if this is really flakiness since the group of tests > > failing > > > seems to be the same each time. > > > > > > Apart from rebase-updating, I don't have a clear idea what to do. > > > > There is a timeout. My guess is that it's related to your refactoring of > > PasswordStoreDefaultTest. Initialization/deinitialization used to be done in > > SetUp/TearDown. Now you moved it to the test body. > > I compared the initialization/termination order: It seems equivalent between the > old and the new implementation. > > One thing noticeable is that the other two Default/PasswordStoreOriginTests > (RemoveLoginsByOriginAndTimeImpl_NonMatchingOrigin and > RemoveLoginsByOriginAndTimeImpl_NotWithinTimeInterval) run through just fine > even though the fixture logic is identical. What makes them different from the > failing RemoveLoginsByOriginAndTimeImpl_FittingOriginAndTime test is that they > do not actually remove any passwords from the password store. So maybe the > failure is related to a side-effect of the actual store removal operation? > > Is there an easy way to run just the other two tests on the Android bot to > validate my assumption? I guess I could create a separate CL without the > offending test and let one of you trigger the try bot job, but that's not a fast > feedback loop exactly. (Maybe this is the right opportunity for me to ask for > try job privileges...) I just nominated you for tryjob access and crbug.com bug editing privileges. I'm afraid it can take about a week to get processed, though. You should get notified once that is done. I don't have time to have a closer look at the failure right now, sorry about that. What you write about the access to the password store sounds like a good lead. Feel free to ping me on any CLs for running trybots, I'm just generally not available during weekends, so until Monday I am not likely to respond. Have a nice weekend! Vaclav
On 2015/11/27 18:13:15, vabr (Chromium) wrote: > On 2015/11/27 17:51:29, Timo Reimann wrote: > > On 2015/11/27 17:00:04, vasilii wrote: > > > On 2015/11/27 16:43:17, Timo Reimann wrote: > > > > On 2015/11/27 16:12:02, commit-bot: I haz the power wrote: > > > > > Try jobs failed on following builders: > > > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > > > > > Not so sure anymore if this is really flakiness since the group of tests > > > failing > > > > seems to be the same each time. > > > > > > > > Apart from rebase-updating, I don't have a clear idea what to do. > > > > > > There is a timeout. My guess is that it's related to your refactoring of > > > PasswordStoreDefaultTest. Initialization/deinitialization used to be done in > > > SetUp/TearDown. Now you moved it to the test body. > > > > I compared the initialization/termination order: It seems equivalent between > the > > old and the new implementation. > > > > One thing noticeable is that the other two Default/PasswordStoreOriginTests > > (RemoveLoginsByOriginAndTimeImpl_NonMatchingOrigin and > > RemoveLoginsByOriginAndTimeImpl_NotWithinTimeInterval) run through just fine > > even though the fixture logic is identical. What makes them different from the > > failing RemoveLoginsByOriginAndTimeImpl_FittingOriginAndTime test is that they > > do not actually remove any passwords from the password store. So maybe the > > failure is related to a side-effect of the actual store removal operation? > > > > Is there an easy way to run just the other two tests on the Android bot to > > validate my assumption? I guess I could create a separate CL without the > > offending test and let one of you trigger the try bot job, but that's not a > fast > > feedback loop exactly. (Maybe this is the right opportunity for me to ask for > > try job privileges...) > > I just nominated you for tryjob access and http://crbug.com bug editing privileges. I'm > afraid it can take about a week to get processed, though. You should get > notified once that is done. > > I don't have time to have a closer look at the failure right now, sorry about > that. What you write about the access to the password store sounds like a good > lead. Feel free to ping me on any CLs for running trybots, I'm just generally > not available during weekends, so until Monday I am not likely to respond. > > Have a nice weekend! > Vaclav Vaclav, thanks for the nomination -- very much appreciated. Following up next week is perfectly fine for me. I'll try to prepare something over the weekend. Enjoy your weekend!
On 2015/11/27 18:22:57, Timo Reimann wrote: > On 2015/11/27 18:13:15, vabr (Chromium) wrote: > > On 2015/11/27 17:51:29, Timo Reimann wrote: > > > On 2015/11/27 17:00:04, vasilii wrote: > > > > On 2015/11/27 16:43:17, Timo Reimann wrote: > > > > > On 2015/11/27 16:12:02, commit-bot: I haz the power wrote: > > > > > > Try jobs failed on following builders: > > > > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > > > > > > > Not so sure anymore if this is really flakiness since the group of tests > > > > failing > > > > > seems to be the same each time. > > > > > > > > > > Apart from rebase-updating, I don't have a clear idea what to do. > > > > > > > > There is a timeout. My guess is that it's related to your refactoring of > > > > PasswordStoreDefaultTest. Initialization/deinitialization used to be done > in > > > > SetUp/TearDown. Now you moved it to the test body. > > > > > > I compared the initialization/termination order: It seems equivalent between > > the > > > old and the new implementation. > > > > > > One thing noticeable is that the other two Default/PasswordStoreOriginTests > > > (RemoveLoginsByOriginAndTimeImpl_NonMatchingOrigin and > > > RemoveLoginsByOriginAndTimeImpl_NotWithinTimeInterval) run through just fine > > > even though the fixture logic is identical. What makes them different from > the > > > failing RemoveLoginsByOriginAndTimeImpl_FittingOriginAndTime test is that > they > > > do not actually remove any passwords from the password store. So maybe the > > > failure is related to a side-effect of the actual store removal operation? > > > > > > Is there an easy way to run just the other two tests on the Android bot to > > > validate my assumption? I guess I could create a separate CL without the > > > offending test and let one of you trigger the try bot job, but that's not a > > fast > > > feedback loop exactly. (Maybe this is the right opportunity for me to ask > for > > > try job privileges...) > > > > I just nominated you for tryjob access and http://crbug.com bug editing > privileges. I'm > > afraid it can take about a week to get processed, though. You should get > > notified once that is done. > > > > I don't have time to have a closer look at the failure right now, sorry about > > that. What you write about the access to the password store sounds like a good > > lead. Feel free to ping me on any CLs for running trybots, I'm just generally > > not available during weekends, so until Monday I am not likely to respond. > > > > Have a nice weekend! > > Vaclav > > Vaclav, thanks for the nomination -- very much appreciated. Following up next > week is perfectly fine for me. I'll try to prepare something over the weekend. > > Enjoy your weekend! Interesting to see it doesn't fail on the debug build. Is PasswordStoreDefault implemented differently on Android? I had a glimpse at the code but couldn't spot any platform-specific differences.
The debug bot didn't run any test for some reason.
The latest patch set finishes successfully on the Android bot. For reasons I can't explain, I added the extra namespace qualifier when moving the tests from PasswordStoreDefaulfTest to PasswordStoreOriginTest. Interestingly, I did the same mistake in https://codereview.chromium.org/1369173002/ (and fixed it in https://codereview.chromium.org/1369173002/#ps80001). If I am not totally mistaken, however, at the time the compiler barked at me for having the extra qualifier. I'm not sure why it slipped through now -- maybe a compiler update changed things for the worse. Vasilii, major thanks for helping me out on this one!
The CQ bit was checked by ttr314@googlemail.com
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/1414463004/#ps260001 (title: "Remove extra base::Time namespace qualifier causing compiler problems.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414463004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414463004/260001
On 2015/12/17 16:39:02, Timo Reimann wrote: > The latest patch set finishes successfully on the Android bot. > > For reasons I can't explain, I added the extra namespace qualifier when moving > the tests from PasswordStoreDefaulfTest to PasswordStoreOriginTest. > Interestingly, I did the same mistake in > https://codereview.chromium.org/1369173002/ (and fixed it in > https://codereview.chromium.org/1369173002/#ps80001). If I am not totally > mistaken, however, at the time the compiler barked at me for having the extra > qualifier. I'm not sure why it slipped through now -- maybe a compiler update > changed things for the worse. > > Vasilii, major thanks for helping me out on this one! Re-read your comment to the GCC bug report and realized that the error would be caught in my previous review as no templates where involved back then.
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from
==========
Implement origin-based deletion for passwords in PasswordDefaultMac.
This change adds the ability to remove passwords by origin (and time) for the
password store under Mac.
While the implementation is fairly straight forward, the major work has been
invested into moving the existing origin-based tests from
PasswordStoreDefaultTest into a new type-parameterized test suite and
refactoring PasswordStore{Default,Mac}Test so that the tests can be shared by
both classes. We enabled the latter by means of introducing test delegates which
encapsulate parts of the test setup and test helper logic.
Finally, we do some refactorings to extract common mock implementations into
helper classes and improve test print output for PasswordStoreChanges.
BUG=113973
==========
to
==========
Implement origin-based deletion for passwords in PasswordDefaultMac.
This change adds the ability to remove passwords by origin (and time) for the
password store under Mac.
While the implementation is fairly straight forward, the major work has been
invested into moving the existing origin-based tests from
PasswordStoreDefaultTest into a new type-parameterized test suite and
refactoring PasswordStore{Default,Mac}Test so that the tests can be shared by
both classes. We enabled the latter by means of introducing test delegates which
encapsulate parts of the test setup and test helper logic.
Finally, we do some refactorings to extract common mock implementations into
helper classes and improve test print output for PasswordStoreChanges.
BUG=113973
Committed: https://crrev.com/b4f01c0a90dcba304b45b280f18c66b3c65822f2
Cr-Commit-Position: refs/heads/master@{#365852}
==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/b4f01c0a90dcba304b45b280f18c66b3c65822f2 Cr-Commit-Position: refs/heads/master@{#365852} |
