Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27233003:
[Sync] Implementation of model association for passwords using sync API (Closed)
|
Created:
7 years, 2 months ago by lipalani1 Modified:
6 years, 9 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplementation of the |MergeDataAndStartSyncing| along with unit tests.
BUG=95758
Patch Set 1 #Patch Set 2 : For review #Patch Set 3 : For review. #
Total comments: 15
Patch Set 4 : For review.\ #
Total comments: 22
Patch Set 5 : For review. #Patch Set 6 : #Patch Set 7 : For review. #
Total comments: 16
Patch Set 8 : For review. #Patch Set 9 : For review. #
Total comments: 17
Patch Set 10 : For review. #Patch Set 11 : For review. #
Total comments: 2
Patch Set 12 : For review. #
Total comments: 2
Patch Set 13 : #
Total comments: 90
Patch Set 14 : For review. #Patch Set 15 : For review #
Total comments: 97
Patch Set 16 : For review. #
Total comments: 27
Messages
Total messages: 25 (0 generated)
Please take a look.
https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... chrome/browser/password_manager/password_syncable_service.cc:72: std::vector<autofill::PasswordForm*>* new_entries, pass ScopedVectors instead? https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... chrome/browser/password_manager/password_syncable_service.cc:73: std::vector<autofill::PasswordForm*>* udpated_entries) { udpated -> updated https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... chrome/browser/password_manager/password_syncable_service.cc:85: // passowrd store. Add the entry to the new_entries list. passowrd -> password https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... chrome/browser/password_manager/password_syncable_service.cc:97: new_password.get())) { fix indent https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... chrome/browser/password_manager/password_syncable_service.cc:140: PasswordEntryMap new_db_entries; I think it's cleaner to track the entries separately from the changes. For example, use an std::set to track the local entries that have been matched with sync, and use a SyncChangeList to track the changes to sync. See for example pref_model_associator.cc. CreateOrUpdateEntry no longer has to have confusing lines like "delete *(it->second.second)" https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... chrome/browser/password_manager/password_syncable_service.cc:142: it = password_entries.begin(); indent four more spaces https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... File chrome/browser/password_manager/password_syncable_service.h (right): https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... chrome/browser/password_manager/password_syncable_service.h:62: // Static Rebase onto your other patch?
PTAL https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... chrome/browser/password_manager/password_syncable_service.cc:72: std::vector<autofill::PasswordForm*>* new_entries, On 2013/10/15 22:04:10, Nicolas Zea wrote: > pass ScopedVectors instead? Done. https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... chrome/browser/password_manager/password_syncable_service.cc:73: std::vector<autofill::PasswordForm*>* udpated_entries) { On 2013/10/15 22:04:10, Nicolas Zea wrote: > udpated -> updated Done. https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... chrome/browser/password_manager/password_syncable_service.cc:85: // passowrd store. Add the entry to the new_entries list. On 2013/10/15 22:04:10, Nicolas Zea wrote: > passowrd -> password Done. https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... chrome/browser/password_manager/password_syncable_service.cc:97: new_password.get())) { On 2013/10/15 22:04:10, Nicolas Zea wrote: > fix indent Done. https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... chrome/browser/password_manager/password_syncable_service.cc:140: PasswordEntryMap new_db_entries; Possible I am missing something here: delete *(it->second.second) - I have done this slightly differently now.(but it is only marginally better :) **(it->second.second) = foo ) The logic to compute the un-synced db entries is by having all the db entries in a set and removing the entries that are identical to sync entries and modifying the entries with sync updates. Then this set is converted to a SyncChangeList. To compute sync entries that are not in passwords db: Compare each entry in sync db with the entries in password db and when not present add them to a vector, when they are present but different add them to another vector. Then these lists are used to populate the passwordstore. The it->second.second is present because we use a single list to track both updates and additions. Is your point to seperate them to 2 lists. Even if we did that we still have to write it->second. and when creating the syncchangelist from these we will have to have 2 ways, one for converting addtions to syncchangelist and another for converting updates, as additions will be stored in set and updates will be stored in vector. Refer to line 170 on how it is done now. I am not opposed to this change though, let me know. On 2013/10/15 22:04:10, Nicolas Zea wrote: > I think it's cleaner to track the entries separately from the changes. For > example, use an std::set to track the local entries that have been matched with > sync, and use a SyncChangeList to track the changes to sync. See for example > pref_model_associator.cc. > > CreateOrUpdateEntry no longer has to have confusing lines like "delete > *(it->second.second)" https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... chrome/browser/password_manager/password_syncable_service.cc:142: it = password_entries.begin(); On 2013/10/15 22:04:10, Nicolas Zea wrote: > indent four more spaces Done. https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... File chrome/browser/password_manager/password_syncable_service.h (right): https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... chrome/browser/password_manager/password_syncable_service.h:62: // Static On 2013/10/15 22:04:10, Nicolas Zea wrote: > Rebase onto your other patch? Done.
Ping! Thanks!
https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/27233003/diff/6001/chrome/browser/password_ma... chrome/browser/password_manager/password_syncable_service.cc:140: PasswordEntryMap new_db_entries; On 2013/10/18 23:33:03, lipalani1 wrote: > Possible I am missing something here: > delete *(it->second.second) - I have done this slightly differently now.(but it > is only marginally better :) **(it->second.second) = foo ) > > The logic to compute the un-synced db entries is by having all the db entries in > a set and removing the entries that are identical to sync entries and modifying > the entries with sync updates. Then this set is converted to a SyncChangeList. > > To compute sync entries that are not in passwords db: Compare each entry in sync > db with the entries in password db and when not present add them to a vector, > when they are present but different add them to another vector. Then these lists > are used to populate the passwordstore. > > The it->second.second is present because we use a single list to track both > updates and additions. Is your point to seperate them to 2 lists. Even if we did > that we still have to write it->second. and when creating the syncchangelist > from these we will have to have 2 ways, one for converting addtions to > syncchangelist and another for converting updates, as additions will be stored > in set and updates will be stored in vector. Refer to line 170 on how it is done > now. I am not opposed to this change though, let me know. > On 2013/10/15 22:04:10, Nicolas Zea wrote: > > I think it's cleaner to track the entries separately from the changes. For > > example, use an std::set to track the local entries that have been matched > with > > sync, and use a SyncChangeList to track the changes to sync. See for example > > pref_model_associator.cc. > > > > CreateOrUpdateEntry no longer has to have confusing lines like "delete > > *(it->second.second)" > I'm not really suggesting to have two lists for tracking changes, but rather have a single SyncChangeList that gets passed to CreateOrUpdateEntry, which will append an Update as necessary. Then, in the section later where you iterate through new_db_entries, you can create ADDs for all the remaining entries. At that point you can just pass new_changes directly to the ProcessSyncChanges method. You could keep the password entry map around if you like; it just would now be a map of string to password form pointer https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.cc:8: #include "base/memory/scoped_ptr.h" already have scoped_ptr in header https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.cc:16: #include "sync/api/sync_error.h" already have in header https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.cc:162: WriteToPasswordStore(&(new_synced_entries.get()), nit: WriteToPasswordStore should be taking const refs as params, since they're not being modified https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... File chrome/browser/password_manager/password_syncable_service.h (right): https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.h:63: // Converts the |PasswordForm| to |SyncData| suitable for syncing. nit: no need to mention suitable for syncing; that's implied by converting to SyncData https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.h:65: const autofill::PasswordForm& password); nit: fix indent https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.h:73: typedef std::map<std::string, nit: remove newline above https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.h:81: void CreateOrUpdateEntry( Comment about what function does https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.h:85: ScopedVector<autofill::PasswordForm>* udpated_entries); udpated -> updated https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service_unittest.cc:32: typedef std::vector<SyncChange> SyncChangeList; put this and the rest of the file in an anonymous namespace? https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service_unittest.cc:37: class MockPasswordSyncableService : public PasswordSyncableService { It's generally cleaner to use fakes when overriding functionality than mocks. Mocks are better for purely virtual interfaces. Perhaps just have this be a TestPasswordSyncableService whose NotifyPasswordStore implementation counts the number of notifications? Similarly, you could abstract the other password store interaction into separate methods, which the test version overrides and supplies test data for/counts invokations, and avoid the need for using the MockPasswordStore below. This will probably also make it easier for future refactorings that remove the need for friending. See https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/mocks/... for more discussion on that https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service_unittest.cc:48: class MockSyncChangeProcessor : public syncer::SyncChangeProcessor { naming nit: This isn't a mock, so maybe just call it TestSyncChangeProccesor?
PTAL. https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.cc:8: #include "base/memory/scoped_ptr.h" On 2013/10/21 23:55:14, Nicolas Zea wrote: > already have scoped_ptr in header Done. https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.cc:16: #include "sync/api/sync_error.h" On 2013/10/21 23:55:14, Nicolas Zea wrote: > already have in header Done. https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.cc:162: WriteToPasswordStore(&(new_synced_entries.get()), On 2013/10/21 23:55:14, Nicolas Zea wrote: > nit: WriteToPasswordStore should be taking const refs as params, since they're > not being modified Done. https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... File chrome/browser/password_manager/password_syncable_service.h (right): https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.h:63: // Converts the |PasswordForm| to |SyncData| suitable for syncing. On 2013/10/21 23:55:14, Nicolas Zea wrote: > nit: no need to mention suitable for syncing; that's implied by converting to > SyncData Done. https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.h:65: const autofill::PasswordForm& password); On 2013/10/21 23:55:14, Nicolas Zea wrote: > nit: fix indent Done. https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.h:73: typedef std::map<std::string, On 2013/10/21 23:55:14, Nicolas Zea wrote: > nit: remove newline above Done. https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.h:81: void CreateOrUpdateEntry( On 2013/10/21 23:55:14, Nicolas Zea wrote: > Comment about what function does Done. https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service.h:85: ScopedVector<autofill::PasswordForm>* udpated_entries); On 2013/10/21 23:55:14, Nicolas Zea wrote: > udpated -> updated Done. https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service_unittest.cc:32: typedef std::vector<SyncChange> SyncChangeList; On 2013/10/21 23:55:14, Nicolas Zea wrote: > put this and the rest of the file in an anonymous namespace? Done. https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service_unittest.cc:37: class MockPasswordSyncableService : public PasswordSyncableService { I tried this approach. it ended up with the boiler plate code in tests, to check the number of times NotifyPasswordStore was invoked(EXPECT_EQ(test_password_syncable_service.invocation_count(),1)). EXPECT_CALL gives this functionality built in. If there is need to add custom methods for other password store methods it is possible to add it in this class itself(those methods would be regular virtual methods rather than mocks) Let me know what you think. On 2013/10/21 23:55:14, Nicolas Zea wrote: > It's generally cleaner to use fakes when overriding functionality than mocks. > Mocks are better for purely virtual interfaces. Perhaps just have this be a > TestPasswordSyncableService whose NotifyPasswordStore implementation counts the > number of notifications? > > Similarly, you could abstract the other password store interaction into separate > methods, which the test version overrides and supplies test data for/counts > invokations, and avoid the need for using the MockPasswordStore below. This will > probably also make it easier for future refactorings that remove the need for > friending. > > See > https://groups.google.com/a/chromium.org/forum/#%21searchin/chromium-dev/mock... > for more discussion on that https://codereview.chromium.org/27233003/diff/13001/chrome/browser/password_m... chrome/browser/password_manager/password_syncable_service_unittest.cc:48: class MockSyncChangeProcessor : public syncer::SyncChangeProcessor { On 2013/10/21 23:55:14, Nicolas Zea wrote: > naming nit: This isn't a mock, so maybe just call it TestSyncChangeProccesor? Done.
https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:148: ScopedVector<autofill::PasswordForm> new_synced_entries; nit: I think the name here would better represent whether or not the data is new to the db. In other words, because the data is known only to sync, it should be added to a list of "new_db_entries". Similarly, the variable below would be "updated_db_entries". https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:177: merge_result.set_error( could you also fill calculate the other variables in merge result? (items before, after, added, etc.) https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.h (right): https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:63: // Converts the |PasswordForm| to |SyncData|. nit: the || usually refer to variable names, so "the |password| into a SyncData object" probably makes more sense https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:54: ~TestSyncChangeProcessor() {} virtual destructor, newline after https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:106: private: newline before https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:107: SyncChangeList expected_changes_; disallow copy and assign? https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:165: std::vector<autofill::PasswordForm> update_changes; disallow copy and assign https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:203: TestPasswordSyncableService service(password_store_.get()); Looks like all tests define this. Maybe just have it be a private member in the PasswordSyncableServiceTest class, and have a public service() getter? Similarly, the behavior below where you set the local data, and the expectations of the changes, seems to be pretty similar across the various tests. Perhaps have helper methods that do this for you? For example, the final code for this test might look like: sync_change_processor()->AddExpectedChange(form1, ACTION_ADD); verifier()->AddExpectedChange(sync_data); SetPasswordStoreData(forms); service()->MergeDataAndStartSyncing(..) EXPECT_EQ(0, verifier.AddChangeCount()) And hopefully the other tests might be similar.
Fixed the feedback and made comments for the couple that I did not fix. PTAL. https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:148: ScopedVector<autofill::PasswordForm> new_synced_entries; May be I am missing something here: The names refer to the state of the item in the respective stores(sync or local). new_sync_entries are items that are new in sync sync db. there is already a variable above called new_db_entries(which are items new in passwords db!) I suppose you mean to make them refer to what action we will do on those items.(i.e., switch these names) I am OK either way, let me know. My assumption is with the comment above each variable it should be easy to follow. On 2013/10/25 20:20:13, Nicolas Zea wrote: > nit: I think the name here would better represent whether or not the data is new > to the db. In other words, because the data is known only to sync, it should be > added to a list of "new_db_entries". Similarly, the variable below would be > "updated_db_entries". https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:177: merge_result.set_error( On 2013/10/25 20:20:13, Nicolas Zea wrote: > could you also fill calculate the other variables in merge result? (items > before, after, added, etc.) Done. https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.h (right): https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:63: // Converts the |PasswordForm| to |SyncData|. On 2013/10/25 20:20:13, Nicolas Zea wrote: > nit: the || usually refer to variable names, so "the |password| into a SyncData > object" probably makes more sense Done. https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:54: ~TestSyncChangeProcessor() {} On 2013/10/25 20:20:13, Nicolas Zea wrote: > virtual destructor, newline after Done. https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:106: private: On 2013/10/25 20:20:13, Nicolas Zea wrote: > newline before Done. https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:107: SyncChangeList expected_changes_; On 2013/10/25 20:20:13, Nicolas Zea wrote: > disallow copy and assign? Done. https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:165: std::vector<autofill::PasswordForm> update_changes; On 2013/10/25 20:20:13, Nicolas Zea wrote: > disallow copy and assign Done. https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:203: TestPasswordSyncableService service(password_store_.get()); I have done some refactoring and it is better now. The problem is most of the tests don't need one aspect of the bundle so I bundled few common denominators to functions of each. On 2013/10/25 20:20:13, Nicolas Zea wrote: > Looks like all tests define this. Maybe just have it be a private member in the > PasswordSyncableServiceTest class, and have a public service() getter? > > Similarly, the behavior below where you set the local data, and the expectations > of the changes, seems to be pretty similar across the various tests. Perhaps > have helper methods that do this for you? > > For example, the final code for this test might look like: > sync_change_processor()->AddExpectedChange(form1, ACTION_ADD); > verifier()->AddExpectedChange(sync_data); > SetPasswordStoreData(forms); > service()->MergeDataAndStartSyncing(..) > EXPECT_EQ(0, verifier.AddChangeCount()) > > And hopefully the other tests might be similar.
On 2013/10/29 20:16:36, lipalani1 wrote: > Fixed the feedback and made comments for the couple that I did not fix. PTAL. > > https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... > File chrome/browser/password_manager/password_syncable_service.cc (right): > > https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... > chrome/browser/password_manager/password_syncable_service.cc:148: > ScopedVector<autofill::PasswordForm> new_synced_entries; > May be I am missing something here: > The names refer to the state of the item in the respective stores(sync or > local). new_sync_entries are items that are new in sync sync db. > > there is already a variable above called new_db_entries(which are items new in > passwords db!) > > I suppose you mean to make them refer to what action we will do on those > items.(i.e., switch these names) I am OK either way, let me know. > > My assumption is with the comment above each variable it should be easy to > follow. > On 2013/10/25 20:20:13, Nicolas Zea wrote: > > nit: I think the name here would better represent whether or not the data is > new > > to the db. In other words, because the data is known only to sync, it should > be > > added to a list of "new_db_entries". Similarly, the variable below would be > > "updated_db_entries". > > https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... > chrome/browser/password_manager/password_syncable_service.cc:177: > merge_result.set_error( > On 2013/10/25 20:20:13, Nicolas Zea wrote: > > could you also fill calculate the other variables in merge result? (items > > before, after, added, etc.) > > Done. > > https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... > File chrome/browser/password_manager/password_syncable_service.h (right): > > https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... > chrome/browser/password_manager/password_syncable_service.h:63: // Converts the > |PasswordForm| to |SyncData|. > On 2013/10/25 20:20:13, Nicolas Zea wrote: > > nit: the || usually refer to variable names, so "the |password| into a > SyncData > > object" probably makes more sense > > Done. > > https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... > File chrome/browser/password_manager/password_syncable_service_unittest.cc > (right): > > https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... > chrome/browser/password_manager/password_syncable_service_unittest.cc:54: > ~TestSyncChangeProcessor() {} > On 2013/10/25 20:20:13, Nicolas Zea wrote: > > virtual destructor, newline after > > Done. > > https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... > chrome/browser/password_manager/password_syncable_service_unittest.cc:106: > private: > On 2013/10/25 20:20:13, Nicolas Zea wrote: > > newline before > > Done. > > https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... > chrome/browser/password_manager/password_syncable_service_unittest.cc:107: > SyncChangeList expected_changes_; > On 2013/10/25 20:20:13, Nicolas Zea wrote: > > disallow copy and assign? > > Done. > > https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... > chrome/browser/password_manager/password_syncable_service_unittest.cc:165: > std::vector<autofill::PasswordForm> update_changes; > On 2013/10/25 20:20:13, Nicolas Zea wrote: > > disallow copy and assign > > Done. > > https://codereview.chromium.org/27233003/diff/143001/chrome/browser/password_... > chrome/browser/password_manager/password_syncable_service_unittest.cc:203: > TestPasswordSyncableService service(password_store_.get()); > I have done some refactoring and it is better now. > > The problem is most of the tests don't need one aspect of the bundle so I > bundled few common denominators to functions of each. > On 2013/10/25 20:20:13, Nicolas Zea wrote: > > Looks like all tests define this. Maybe just have it be a private member in > the > > PasswordSyncableServiceTest class, and have a public service() getter? > > > > Similarly, the behavior below where you set the local data, and the > expectations > > of the changes, seems to be pretty similar across the various tests. Perhaps > > have helper methods that do this for you? > > > > For example, the final code for this test might look like: > > sync_change_processor()->AddExpectedChange(form1, ACTION_ADD); > > verifier()->AddExpectedChange(sync_data); > > SetPasswordStoreData(forms); > > service()->MergeDataAndStartSyncing(..) > > EXPECT_EQ(0, verifier.AddChangeCount()) > > > > And hopefully the other tests might be similar. Ping!
https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:35: namespace { nit: newline after https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:156: std::vector<autofill::PasswordForm>* password_list) { fix indent https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:160: it != password_list->end(); here too https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:208: scoped_ptr<TestSyncChangeProcessor>* sync_change_processor() { no need to return scoped ptr, just return normal pointer (sync_change_processor_get()) https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:245: EXPECT_CALL(*(password_store_.get()), AddLoginImpl(_)) you can just do *password_store_, here and elsewhere https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:249: EXPECT_CALL(*(service()), NotifyPasswordStore()); no need for parens around (service()), here and elsewhere https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:329: .Pass()); check verifier afterwards? https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:366: } nit: newline after
PTAL. https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:35: namespace { On 2013/10/30 23:57:18, Nicolas Zea wrote: > nit: newline after Done. https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:156: std::vector<autofill::PasswordForm>* password_list) { On 2013/10/30 23:57:18, Nicolas Zea wrote: > fix indent Done. https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:160: it != password_list->end(); On 2013/10/30 23:57:18, Nicolas Zea wrote: > here too Done. https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:208: scoped_ptr<TestSyncChangeProcessor>* sync_change_processor() { We have to pass this scoped ptr as a parameter to MergeDataAndStartSyncing(which is achieved by doing a .Pass) The pointer ownership transfers to PasswordSyncableService. Another option is we can have a method here called release_sync_change_processor which returns a pointer. Let me know what you think. On 2013/10/30 23:57:18, Nicolas Zea wrote: > no need to return scoped ptr, just return normal pointer > (sync_change_processor_get()) https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:245: EXPECT_CALL(*(password_store_.get()), AddLoginImpl(_)) On 2013/10/30 23:57:18, Nicolas Zea wrote: > you can just do *password_store_, here and elsewhere Done. https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:249: EXPECT_CALL(*(service()), NotifyPasswordStore()); On 2013/10/30 23:57:18, Nicolas Zea wrote: > no need for parens around (service()), here and elsewhere Done. https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:329: .Pass()); On 2013/10/30 23:57:18, Nicolas Zea wrote: > check verifier afterwards? Done. https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:366: } On 2013/10/30 23:57:18, Nicolas Zea wrote: > nit: newline after Done.
https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/27233003/diff/263001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:208: scoped_ptr<TestSyncChangeProcessor>* sync_change_processor() { On 2013/10/31 19:22:18, lipalani1 wrote: > We have to pass this scoped ptr as a parameter to MergeDataAndStartSyncing(which > is achieved by doing a .Pass) > > The pointer ownership transfers to PasswordSyncableService. Another option is we > can have a method here called release_sync_change_processor which returns a > pointer. Let me know what you think. > On 2013/10/30 23:57:18, Nicolas Zea wrote: > > no need to return scoped ptr, just return normal pointer > > (sync_change_processor_get()) > Ah, I see. Yeah, I think two different methods is better. sync_change_processor should just return the pointer, while ReleaseSyncChangeProcessor() should return sync_change_processor_.Pass(). https://codereview.chromium.org/27233003/diff/373001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/27233003/diff/373001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:255: scoped_ptr<syncer::SyncErrorFactory>() I don't think you need to call pass when you create a new scoped_ptr like this.
PTAL https://codereview.chromium.org/27233003/diff/373001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/27233003/diff/373001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:255: scoped_ptr<syncer::SyncErrorFactory>() On 2013/10/31 22:41:56, Nicolas Zea wrote: > I don't think you need to call pass when you create a new scoped_ptr like this. Done.
lgtm
Ilya - Would you mind doing a review so as to get a LGTM from an owner. Thanks!
https://codereview.chromium.org/27233003/diff/423001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.h (right): https://codereview.chromium.org/27233003/diff/423001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:74: PasswordEntryMap; I just realized this contains iterators to another vector, which seems error prone (the iterator could ge t invalidated if you move things around). I think it'd be safer to just have passwordform pointers.
good point, Agreed. Changed it to pointers.
On 2013/11/01 22:23:13, lipalani1 wrote: > good point, Agreed. > > Changed it to pointers. Nicolas - Come to think of it, would you mind doing a quick lgtm. It should take only a few mts to review.
lgtm
Sorry, been in M32 crunch mode today. I have one trivial nit for the one file that I started looking through. Will give the rest a more proper look on Monday. https://codereview.chromium.org/27233003/diff/423001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.h (right): https://codereview.chromium.org/27233003/diff/423001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:65: const autofill::PasswordForm& password); nit: Please leave a blank line between function declarations, and document the function below as well.
https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:20: // local password and stores the output in the |new_password_form| pointer. nit: Please document the semantics of the return value for this function. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:77: Optional nit: Omit this newline IMO https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:81: PasswordEntryMap::iterator it = loaded_data->find(tag); nit: Can you find a more descriptive name for this variable than "it"? https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:82: Optional nit: Omit this newline IMO. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:84: // The sync data is not in password store, so we need to create it in nit: "in password store" -> "in the password store" (twice) https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:86: autofill::PasswordForm* new_password = new autofill::PasswordForm(); Please store this in a scoped_ptr, and document ownership transfer by releasing the scoped_ptr's hold on the memory on line 89 (at least, I'm pretty sure that's where the ownership transfer happens). Once you do this, line 88 likely becomes unnecessary, as it's clear enough from the code itself. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:103: updated_entries->push_back(new_password.release()); Hmm, if I'm reading this code correctly, it looks like both the local DB and the Sync model are always updated together. That doesn't really make sense to me though -- shouldn't only one of them -- the one less up-to-date -- be updated, while the other remains constant? https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:129: syncer::MODEL_TYPE_COUNT); Please define a wrapper function for this histogram, so that the call sites look more like sync::LogLocalDataFailedToLoad(syncer::PASSWORDS). This reduces the likelihood of typos creeping in and causing individual codepaths to fail, which can be a hard bug to spot after the fact. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:133: return merge_result;; nit: Duplicate semicolon. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:136: PasswordEntryMap new_db_entries; nit: "db" -> "database" or "local" https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:137: for (std::vector<autofill::PasswordForm*>::iterator nit: Since you do have a typedef for this type, why not use it here? https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:143: // list will only contain entries that are not in sync. nit: This second sentence is worded a little bit strangely. Please try to reword it to be a little clearer. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:144: new_db_entries[MakeTag(**it)] = *it; nit: This line might be easier to read if the previous line gave a name to "*it" as e.g. "autofill::PasswordForm* password_entry = *it;" https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:153: // have to be updated in sync. Are these entries that need to be updated in Sync, or locally? Reading through the code, it kind of looks like these are being updated in the local DB rather than in Sync. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:154: ScopedVector<autofill::PasswordForm> updated_sync_entries; Thanks, the comments for both this variable and the one above it are very clear and helpful :) https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:174: merge_result.set_num_items_added(new_sync_entries.size()); Out of curiosity, why does the Sync API require clients to call both set_num_items_after_association() and set_num_items_added()? Shouldn't either one be deducible from the other, given that set_num_items_before_association() was already called? https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:323: nit: Spurious newline. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.h (right): https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:65: const autofill::PasswordForm& password); Why is this public? If it's just for testing, please do something more akin to the treatment of GetFingerprintInternal here: https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:68: autofill::PasswordForm* new_password); Why is this public? https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:79: // Checks if |data|, the entry in sync db, needs to be created or updated nit: "db" -> "database" (throughout) https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:82: // identical to an entry in passwords db no action is performed. If an nit: "an entry in passwords db no action" -> "an entry in the passwords database, no action" https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:84: // added to |updated_db_entries| list. Please document that this method has a side-effect of removing entries from |loaded_data|. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:90: syncer::SyncChangeList* updated_db_entries); nit: "db" -> "database" in variable names as well. Something like "updated_local_entries" would also work. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:92: // Virtual so tests can override. Please document what this method does. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:92: // Virtual so tests can override. Subclasses should not override private methods... please raise the visibility of this to be protected if it's meant to be overridden. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:93: virtual void NotifyPasswordStore(); This method name is somewhat unclear -- what is the password store being notified of? Please try to find a more descriptive name. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:44: explicit TestPasswordSyncableService(PasswordStore* password_store): nit: The colon belongs on the next line, formatted like so: explicit TestPasswordSyncableService(PasswordStore* password_store) : PasswordSyncableService(password_store) {} https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:50: // Concrete implementation of SyncChangeprocessor. The methods will nit: "SyncChangeprocessor" -> "SyncChangeProcessor" https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:71: password_specifics); nit: Might be helpful to decompose lines 66-71 into a helper function. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:137: // Verifies that the |password| is present in the |add_changes_| list. Please document that this has a side-effect of removing the |password| from the |add_changes_| list if it is found. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:147: int AddChangeCount() const { nit: Consider making the return value for this method have type size_t rather than int. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:170: } nit: Please leave a blank line after this one. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:172: std::vector<autofill::PasswordForm> update_changes_; nit: Consider naming these variables something more like "expected_new_entries_" and "expected_updated_entries_" to match the names the production code uses. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:176: SyncData CreateSyncData(std::string signon_realm) { nit: Please pass by const-reference. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:176: SyncData CreateSyncData(std::string signon_realm) { nit: Docs, please. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:184: } nit: Please tuck all of the code above this line into an anonymous namespace. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:215: } These four accessors seem unnecessary given that their backing variables all have protected visibility. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:218: const std::vector<autofill::PasswordForm*>& forms) { nit: Docs, please. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:222: .WillOnce(Return(true)); Please also add test coverage for FillBlacklistLogins returning data, and FillAutofillableLogins *not* returning data. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:234: autofill::PasswordForm *form1 = new autofill::PasswordForm; nit: Asterisk belongs before the space. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:234: autofill::PasswordForm *form1 = new autofill::PasswordForm; Please use a scoped_ptr to own the memory for this variable until it's passed to its next owner. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:352: verifier()->AddExpectedUpdateChange(*form1); Why are both the local and the sync value expected to be updated? What is the expected resulting value? https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:368: } Please also add test coverage to verify that *all* fields are copied correctly, both when there are new entries either on the Sync or local side, and when the data is being merged. The best way to do this might be to expand the data that's passed into AddExpectedChange() and AddExpectedUpdateChange(). https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:371: nit: Spurious newline.
Ilya - PTAL. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:20: // local password and stores the output in the |new_password_form| pointer. On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: Please document the semantics of the return value for this function. Done. it is in the header file. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:77: On 2013/11/05 08:39:21, Ilya Sherman wrote: > Optional nit: Omit this newline IMO Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:81: PasswordEntryMap::iterator it = loaded_data->find(tag); On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: Can you find a more descriptive name for this variable than "it"? Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:82: On 2013/11/05 08:39:21, Ilya Sherman wrote: > Optional nit: Omit this newline IMO. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:84: // The sync data is not in password store, so we need to create it in On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: "in password store" -> "in the password store" (twice) Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:86: autofill::PasswordForm* new_password = new autofill::PasswordForm(); On 2013/11/05 08:39:21, Ilya Sherman wrote: > Please store this in a scoped_ptr, and document ownership transfer by releasing > the scoped_ptr's hold on the memory on line 89 (at least, I'm pretty sure that's > where the ownership transfer happens). Once you do this, line 88 likely becomes > unnecessary, as it's clear enough from the code itself. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:103: updated_entries->push_back(new_password.release()); In this case the entries are merged. there could be changes in both. so both sync and passwords are updated. On 2013/11/05 08:39:21, Ilya Sherman wrote: > Hmm, if I'm reading this code correctly, it looks like both the local DB and the > Sync model are always updated together. That doesn't really make sense to me > though -- shouldn't only one of them -- the one less up-to-date -- be updated, > while the other remains constant? https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:129: syncer::MODEL_TYPE_COUNT); On 2013/11/05 08:39:21, Ilya Sherman wrote: > Please define a wrapper function for this histogram, so that the call sites look > more like sync::LogLocalDataFailedToLoad(syncer::PASSWORDS). This reduces the > likelihood of typos creeping in and causing individual codepaths to fail, which > can be a hard bug to spot after the fact. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:133: return merge_result;; On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: Duplicate semicolon. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:136: PasswordEntryMap new_db_entries; On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: "db" -> "database" or "local" Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:137: for (std::vector<autofill::PasswordForm*>::iterator On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: Since you do have a typedef for this type, why not use it here? Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:143: // list will only contain entries that are not in sync. On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: This second sentence is worded a little bit strangely. Please try to > reword it to be a little clearer. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:144: new_db_entries[MakeTag(**it)] = *it; On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: This line might be easier to read if the previous line gave a name to "*it" > as e.g. "autofill::PasswordForm* password_entry = *it;" Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:153: // have to be updated in sync. On 2013/11/05 08:39:21, Ilya Sherman wrote: > Are these entries that need to be updated in Sync, or locally? Reading through > the code, it kind of looks like these are being updated in the local DB rather > than in Sync. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:154: ScopedVector<autofill::PasswordForm> updated_sync_entries; On 2013/11/05 08:39:21, Ilya Sherman wrote: > Thanks, the comments for both this variable and the one above it are very clear > and helpful :) Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:174: merge_result.set_num_items_added(new_sync_entries.size()); these stats will get uploaded to the server. Having a separate field will help us run queries easily. On 2013/11/05 08:39:21, Ilya Sherman wrote: > Out of curiosity, why does the Sync API require clients to call both > set_num_items_after_association() and set_num_items_added()? Shouldn't either > one be deducible from the other, given that set_num_items_before_association() > was already called? https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:323: On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: Spurious newline. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.h (right): https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:65: const autofill::PasswordForm& password); On 2013/11/05 08:39:21, Ilya Sherman wrote: > Why is this public? If it's just for testing, please do something more akin to > the treatment of GetFingerprintInternal here: > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:68: autofill::PasswordForm* new_password); made it private. On 2013/11/05 08:39:21, Ilya Sherman wrote: > Why is this public? https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:79: // Checks if |data|, the entry in sync db, needs to be created or updated I borrowed the abbreviation db from db thread. So I think it is an accepted convention. let me know if you disagree. On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: "db" -> "database" (throughout) https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:82: // identical to an entry in passwords db no action is performed. If an On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: "an entry in passwords db no action" -> "an entry in the passwords > database, no action" Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:84: // added to |updated_db_entries| list. On 2013/11/05 08:39:21, Ilya Sherman wrote: > Please document that this method has a side-effect of removing entries from > |loaded_data|. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:90: syncer::SyncChangeList* updated_db_entries); On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: "db" -> "database" in variable names as well. Something like > "updated_local_entries" would also work. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:92: // Virtual so tests can override. On 2013/11/05 08:39:21, Ilya Sherman wrote: > Please document what this method does. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:92: // Virtual so tests can override. On 2013/11/05 08:39:21, Ilya Sherman wrote: > Subclasses should not override private methods... please raise the visibility of > this to be protected if it's meant to be overridden. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:93: virtual void NotifyPasswordStore(); On 2013/11/05 08:39:21, Ilya Sherman wrote: > This method name is somewhat unclear -- what is the password store being > notified of? Please try to find a more descriptive name. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:44: explicit TestPasswordSyncableService(PasswordStore* password_store): On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: The colon belongs on the next line, formatted like so: > > explicit TestPasswordSyncableService(PasswordStore* password_store) > : PasswordSyncableService(password_store) {} Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:50: // Concrete implementation of SyncChangeprocessor. The methods will On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: "SyncChangeprocessor" -> "SyncChangeProcessor" Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:71: password_specifics); On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: Might be helpful to decompose lines 66-71 into a helper function. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:137: // Verifies that the |password| is present in the |add_changes_| list. On 2013/11/05 08:39:21, Ilya Sherman wrote: > Please document that this has a side-effect of removing the |password| from the > |add_changes_| list if it is found. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:147: int AddChangeCount() const { On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: Consider making the return value for this method have type size_t rather > than int. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:170: } On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:172: std::vector<autofill::PasswordForm> update_changes_; 'Add' and 'Update' terminologies are also widely used. On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: Consider naming these variables something more like "expected_new_entries_" > and "expected_updated_entries_" to match the names the production code uses. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:176: SyncData CreateSyncData(std::string signon_realm) { On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: Please pass by const-reference. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:176: SyncData CreateSyncData(std::string signon_realm) { On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: Docs, please. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:184: } On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: Please tuck all of the code above this line into an anonymous namespace. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:215: } On 2013/11/05 08:39:21, Ilya Sherman wrote: > These four accessors seem unnecessary given that their backing variables all > have protected visibility. Done. However I have left the verififer() in place as it simplifies the calling site. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:218: const std::vector<autofill::PasswordForm*>& forms) { On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: Docs, please. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:222: .WillOnce(Return(true)); Good point. this cl has gotten big already. I have left a todo and will fix it in a subsequent patch. On 2013/11/05 08:39:21, Ilya Sherman wrote: > Please also add test coverage for FillBlacklistLogins returning data, and > FillAutofillableLogins *not* returning data. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:234: autofill::PasswordForm *form1 = new autofill::PasswordForm; On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: Asterisk belongs before the space. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:234: autofill::PasswordForm *form1 = new autofill::PasswordForm; On 2013/11/05 08:39:21, Ilya Sherman wrote: > Please use a scoped_ptr to own the memory for this variable until it's passed to > its next owner. Done. https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:352: verifier()->AddExpectedUpdateChange(*form1); In this case form1 wins. But in our production code we update both stores in these types of cases. On 2013/11/05 08:39:21, Ilya Sherman wrote: > Why are both the local and the sync value expected to be updated? What is the > expected resulting value? https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:368: } When verifying add/update we compare all fields. On 2013/11/05 08:39:21, Ilya Sherman wrote: > Please also add test coverage to verify that *all* fields are copied correctly, > both when there are new entries either on the Sync or local side, and when the > data is being merged. The best way to do this might be to expand the data > that's passed into AddExpectedChange() and AddExpectedUpdateChange(). https://codereview.chromium.org/27233003/diff/503001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:371: On 2013/11/05 08:39:21, Ilya Sherman wrote: > nit: Spurious newline. Done.
Thanks, I really like where this is headed. https://chromiumcodereview.appspot.com/27233003/diff/503001/chrome/browser/pa... File chrome/browser/password_manager/password_syncable_service.h (right): https://chromiumcodereview.appspot.com/27233003/diff/503001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.h:79: // Checks if |data|, the entry in sync db, needs to be created or updated I won't block the review on this, but I do think that "database" is marginally clearer than "db", and not so lengthy as to be overly verbose. On 2013/11/19 00:46:54, lipalani1 wrote: > I borrowed the abbreviation db from db thread. So I think it is an accepted > convention. let me know if you disagree. > > On 2013/11/05 08:39:21, Ilya Sherman wrote: > > nit: "db" -> "database" (throughout) https://chromiumcodereview.appspot.com/27233003/diff/503001/chrome/browser/pa... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://chromiumcodereview.appspot.com/27233003/diff/503001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:222: .WillOnce(Return(true)); I'm not comfortable with punting tests to follow-up patches. If you'd like to reduce the size of this CL, please split out functionality, not test coverage. On 2013/11/19 00:46:54, lipalani1 wrote: > Good point. this cl has gotten big already. I have left a todo and will fix it > in a subsequent patch. > On 2013/11/05 08:39:21, Ilya Sherman wrote: > > Please also add test coverage for FillBlacklistLogins returning data, and > > FillAutofillableLogins *not* returning data. > https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... File chrome/browser/password_manager/password_syncable_service.cc (right): https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.cc:53: } Can this method be tucked into an anonymous namespace in this implementation file, rather than being exposed in the header file? https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.cc:70: specifics.password().client_only_encrypted_data()); Could you explain to me what |client_only_encrypted_data| means? In MergeLocalAndSyncPasswords(), it looks like you compare PasswordSpecificsData to unencrypted local data. What does the "encrypted" part of the field name mean, then? https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.cc:73: // Find if the data from sync is already in password store. nit: "Find if" -> "Check whether" or "Test whether" or something like that. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.cc:74: PasswordEntryMap::iterator loaded_data_index = loaded_data->find(tag); nit: "index" -> "iter" https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.cc:83: // The entry is in password store. If the entries are not identical the nit: "are not identical the entries" -> "are not identical, then the entries" https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.cc:89: new_password.get())) { Please add a comment within this if-stmt along the lines of: "It's possible that both the Sync database and the passwords database need to be updated to reflect the merged data. Rather than testing whether each update is strictly needed, simply always update both together. The update is a no-op if the existing data identically matches the merged data." (Feel free to reword as much as you like -- what I think is important here is to convey that while both updates might not be strictly necessary, it's *possible* that they are. This wasn't obvious to me when first reading the code -- I expected one entry or the other to always be more up-to-date. In fact, looking at the code for MergeLocalAndSyncPasswords(), it still seems to me like one is always more up-to-date, so I don't really understand why we always issue two updates rather than just the one that's needed. Perhaps the better improvement here is to update the code rather than to add documentation.) https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.cc:100: loaded_data->erase(loaded_data_index); The comment in the header strongly implies that entries will only be removed from |loaded_data| if an *identical* entry is found in the Sync DB. Please update that comment to clarify that entries will be removed even if the match is not identical. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.cc:132: // association entries that match a sync entry will be removed and this nit: "While model association entries" -> "During model association, entries" https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.cc:139: // List that contains the entries that are known only to sync. nit: Needing comments like these generally implies that the variable names are not sufficiently clear. Part of the challenge, I think, is that "new_sync_entries" does not immediately clarify whether these are entries that are new to the local db, coming from Sync; or whether these are entries that are new to Sync, coming to the local db. I'd suggest trying to find names that are clear enough so that comments are not needed. For example, what do you think of the following set of names? initial_sync_data -> synced_passwords password_entries -> local_passwords new_local_passwords -> tentative_new_passwords_from_local_store new_sync_passwords -> new_passwords_from_sync updated_sync_entries -> updated_passwords_from_sync db_changes -> updated_passwords_from_local_store (Note: the variable name "tentative_new_passwords_from_local_store" is quite a mouthful, but I think that's partly because the variable is doing a lot: it's both the set of local passwords that's used to search for matches from Sync, and the set of passwords that aren't found in Sync, and need to be synced. That said, it might still be that the prefix |tentative_| is more distracting than it is helpful, i.e. I'm not sure whether new_passwords_from_local_db would be a better name.) https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.cc:146: // Changes from password db that needs to be propagated to sync. nit: "needs" -> "need" https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.cc:147: syncer::SyncChangeList db_changes; Optional nit: I'd suggest leaving a blank line after this one, to clarify that the comment applies just to this variable, and not to the block. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.cc:159: updated_sync_entries.get()); Does this line compile? It looks like you're passing pointers to a method that expects const-references. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.cc:312: } Can this be tucked into the anonymous namespace, rather than being exported in the header file? https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... File chrome/browser/password_manager/password_syncable_service.h (right): https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.h:74: friend class TestSyncChangeProcessor; Please do not add friend classes. Instead, use protected methods that tests can override. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.h:82: // entry is added to |new_entries| or |updated_entries|. If the item is nit: "is added to" -> "may be added to", since it's possible that this method is a no-op (if the entry already exists in the passwords db). https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.h:85: // added to |updated_local_entries| list.If |data| is identical to an entry nit: "|updated_local_entries| list.If" -> "|updated_local_entries|. If" https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.h:89: PasswordEntryMap* loaded_data, nit: Perhaps rename |loaded_data| to something more like |tentative_unsynced_data|? https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.h:91: ScopedVector<autofill::PasswordForm>* updated_entries, nit: Where is ScopedVector fwd-declared? It seems like you might be pulling this declaration in as an implicit dependency from other header files. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.h:96: const autofill::PasswordForm& password); nit: Please leave a blank line after this one. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service.h:97: static void ExtractPasswordFromSpecifics( nit: Please document. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. Ultra nit: Please omit the "(c)". https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:35: namespace { nit: Please leave a blank line after this one. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:48: // that touch the password db. nit: Suggested rephrasing: "A testable implementation of the |PasswordSyncableService| class that mocks out all interactions with the password database." https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:55: }; Rather than mocking out the method from the PasswordSyncableService, can we mock out the method in MockPasswordStore? https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:66: const SyncChangeList& change_list) { nit: OVERRIDE. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:67: // Loop through the |change_list| and verify they are present in the nit: Suggested rephrasing: "Verify that the |change_list| matches the |expected_changes_|." https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:72: SyncChange data = *it; nit: Can this be a const-ref? https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:88: if (data.change_type() == expected_data.change_type()) { It looks like this only verifies that the tags and the types match. IMO it would be better to verify that *all* the data matches. For example, that would catch a bug where the code attempted to merge the old entry rather than the merged entry back up to the Sync server. The simplest way to do that might be to serialize both data objects, and compare the serialized bytes. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:100: virtual SyncDataList GetAllSyncData(syncer::ModelType type) const { nit: OVERRIDE. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:113: SyncChangeList expected_changes_; What do you think of storing this as a map from tags to expected changes? Seems like that would simplify lines 78-94. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:124: void AddExpectedAddChange(const SyncData& data) { This is a strange API. Why doesn't this function accept an autofill::PasswordForm parameter instead? https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:125: const sync_pb::EntitySpecifics& specifics = data.GetSpecifics(); nit: Looks like this variable is unused. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:160: void VerifyChange(const autofill::PasswordForm& password, nit: Docs, especially mentioning the side-effect (the erase() call). https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:164: = password_list->begin(); nit: The equals sign should be on the previous line. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:177: std::vector<autofill::PasswordForm> update_changes_; nit: Please leave a blank line after this one. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:177: std::vector<autofill::PasswordForm> update_changes_; nit: Consider adding an "expected_" prefix to these variable names. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:179: }; nit: These two classes ought to be in the anonymous namespace as well. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:183: // Creates a sync data consisting of password specifics. The sign on real is nit: real -> realm https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:184: // set to |signon_realm|. nit: Suggested rephrasing: Creates a SyncData instance with the given |signon_realm|. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:195: class PasswordSyncableServiceTest : public testing::Test { nit: The test code itself should not be in the anonymous namespace, to avoid bad interactions caused by C++ name mangling. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:216: } nit: IMO a better API would be to provide a wrapper around MergeDataAndStartSyncing. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:226: // FillBlacklistLogins. I'm not comfortable with checking in untested production code for blacklisted logins. If you don't think it's appropriate to add test coverage as part of this CL, please also hold off on adding the production code, and log a bug to help make sure this doesn't get lost. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:263: scoped_ptr<syncer::SyncErrorFactory>()); nit: Indentation is slightly off. https://chromiumcodereview.appspot.com/27233003/diff/633001/chrome/browser/pa... chrome/browser/password_manager/password_syncable_service_unittest.cc:374: } // namespace Please include test coverage to make sure that all of the PasswordSpecificsData fields are copied correctly from Sync to the local datastore, and vice-versa. https://chromiumcodereview.appspot.com/27233003/diff/633001/sync/api/sync_err... File sync/api/sync_error.cc (right): https://chromiumcodereview.appspot.com/27233003/diff/633001/sync/api/sync_err... sync/api/sync_error.cc:144: MODEL_TYPE_COUNT); Please add this to histograms.xml as part of this CL. https://chromiumcodereview.appspot.com/27233003/diff/633001/sync/api/sync_err... File sync/api/sync_error.h (right): https://chromiumcodereview.appspot.com/27233003/diff/633001/sync/api/sync_err... sync/api/sync_error.h:78: static void LogDataLoadFailure(ModelType model_type); nit: Please document this method, and leave a blank line between this and the "private:" access specifier.
PTAL https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:53: } it calls into other private methods inside the class. All of them cannot be anonymous as they are used by test cases. On 2013/11/19 22:49:04, Ilya Sherman wrote: > Can this method be tucked into an anonymous namespace in this implementation > file, rather than being exposed in the header file? https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:70: specifics.password().client_only_encrypted_data()); This is the way things are stored in sync internally. Passwords data are encrypted before they are shared with the server. When clients receive this encrypted data from the server, it will be decrypted and this decrypted data(stored in client_only_encrypted_data) is used for populating the password store. On 2013/11/19 22:49:04, Ilya Sherman wrote: > Could you explain to me what |client_only_encrypted_data| means? In > MergeLocalAndSyncPasswords(), it looks like you compare PasswordSpecificsData to > unencrypted local data. What does the "encrypted" part of the field name mean, > then? https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:73: // Find if the data from sync is already in password store. On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: "Find if" -> "Check whether" or "Test whether" or something like that. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:74: PasswordEntryMap::iterator loaded_data_index = loaded_data->find(tag); On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: "index" -> "iter" Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:83: // The entry is in password store. If the entries are not identical the On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: "are not identical the entries" -> "are not identical, then the entries" Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:89: new_password.get())) { On 2013/11/19 22:49:04, Ilya Sherman wrote: > Please add a comment within this if-stmt along the lines of: "It's possible that > both the Sync database and the passwords database need to be updated to reflect > the merged data. Rather than testing whether each update is strictly needed, > simply always update both together. The update is a no-op if the existing data > identically matches the merged data." (Feel free to reword as much as you like > -- what I think is important here is to convey that while both updates might not > be strictly necessary, it's *possible* that they are. This wasn't obvious to me > when first reading the code -- I expected one entry or the other to always be > more up-to-date. In fact, looking at the code for MergeLocalAndSyncPasswords(), > it still seems to me like one is always more up-to-date, so I don't really > understand why we always issue two updates rather than just the one that's > needed. Perhaps the better improvement here is to update the code rather than > to add documentation.) Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:100: loaded_data->erase(loaded_data_index); On 2013/11/19 22:49:04, Ilya Sherman wrote: > The comment in the header strongly implies that entries will only be removed > from |loaded_data| if an *identical* entry is found in the Sync DB. Please > update that comment to clarify that entries will be removed even if the match is > not identical. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:132: // association entries that match a sync entry will be removed and this On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: "While model association entries" -> "During model association, entries" Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:139: // List that contains the entries that are known only to sync. These names will make sense when looking at it as the current state of things(i.e. before merging). The names proposed are more confusing. On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Needing comments like these generally implies that the variable names are > not sufficiently clear. Part of the challenge, I think, is that > "new_sync_entries" does not immediately clarify whether these are entries that > are new to the local db, coming from Sync; or whether these are entries that are > new to Sync, coming to the local db. > > I'd suggest trying to find names that are clear enough so that comments are not > needed. For example, what do you think of the following set of names? > > initial_sync_data -> synced_passwords > password_entries -> local_passwords > new_local_passwords -> tentative_new_passwords_from_local_store > new_sync_passwords -> new_passwords_from_sync > updated_sync_entries -> updated_passwords_from_sync > db_changes -> updated_passwords_from_local_store > > (Note: the variable name "tentative_new_passwords_from_local_store" is quite a > mouthful, but I think that's partly because the variable is doing a lot: it's > both the set of local passwords that's used to search for matches from Sync, and > the set of passwords that aren't found in Sync, and need to be synced. That > said, it might still be that the prefix |tentative_| is more distracting than it > is helpful, i.e. I'm not sure whether new_passwords_from_local_db would be a > better name.) https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:146: // Changes from password db that needs to be propagated to sync. On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: "needs" -> "need" Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:147: syncer::SyncChangeList db_changes; On 2013/11/19 22:49:04, Ilya Sherman wrote: > Optional nit: I'd suggest leaving a blank line after this one, to clarify that > the comment applies just to this variable, and not to the block. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:159: updated_sync_entries.get()); On 2013/11/19 22:49:04, Ilya Sherman wrote: > Does this line compile? It looks like you're passing pointers to a method that > expects const-references. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:312: } Are used in test cases. On 2013/11/19 22:49:04, Ilya Sherman wrote: > Can this be tucked into the anonymous namespace, rather than being exported in > the header file? https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.h (right): https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:74: friend class TestSyncChangeProcessor; These classes need to use private helpers to reduce their complexity. The inheritance relationship does not make sense as they test something else.(i.e,. we would have to do multiple inheritance). On 2013/11/19 22:49:04, Ilya Sherman wrote: > Please do not add friend classes. Instead, use protected methods that tests can > override. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:82: // entry is added to |new_entries| or |updated_entries|. If the item is On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: "is added to" -> "may be added to", since it's possible that this method is > a no-op (if the entry already exists in the passwords db). Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:85: // added to |updated_local_entries| list.If |data| is identical to an entry On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: "|updated_local_entries| list.If" -> "|updated_local_entries|. If" Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:89: PasswordEntryMap* loaded_data, On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Perhaps rename |loaded_data| to something more like > |tentative_unsynced_data|? Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:91: ScopedVector<autofill::PasswordForm>* updated_entries, On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Where is ScopedVector fwd-declared? It seems like you might be pulling > this declaration in as an implicit dependency from other header files. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:96: const autofill::PasswordForm& password); On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:97: static void ExtractPasswordFromSpecifics( On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Please document. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/11/19 22:49:04, Ilya Sherman wrote: > Ultra nit: Please omit the "(c)". Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:35: namespace { On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:48: // that touch the password db. On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Suggested rephrasing: "A testable implementation of the > |PasswordSyncableService| class that mocks out all interactions with the > password database." Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:55: }; It is calling private methods in password store. On 2013/11/19 22:49:04, Ilya Sherman wrote: > Rather than mocking out the method from the PasswordSyncableService, can we mock > out the method in MockPasswordStore? https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:66: const SyncChangeList& change_list) { On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: OVERRIDE. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:67: // Loop through the |change_list| and verify they are present in the On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Suggested rephrasing: "Verify that the |change_list| matches the > |expected_changes_|." Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:72: SyncChange data = *it; On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Can this be a const-ref? Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:88: if (data.change_type() == expected_data.change_type()) { On 2013/11/19 22:49:04, Ilya Sherman wrote: > It looks like this only verifies that the tags and the types match. IMO it > would be better to verify that *all* the data matches. For example, that would > catch a bug where the code attempted to merge the old entry rather than the > merged entry back up to the Sync server. > > The simplest way to do that might be to serialize both data objects, and compare > the serialized bytes. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:100: virtual SyncDataList GetAllSyncData(syncer::ModelType type) const { On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: OVERRIDE. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:113: SyncChangeList expected_changes_; After using the maps in production code I am shying away from it because of poor readability. As an example a statement here would be like: iterator->second.sync_data(); Even people in sync team, who know the class names, got confused as to what sync_data was referring to. But I understand your point. If it is all the same, this is easy to understand(production code still uses maps as it is more efficient). On 2013/11/19 22:49:04, Ilya Sherman wrote: > What do you think of storing this as a map from tags to expected changes? Seems > like that would simplify lines 78-94. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:124: void AddExpectedAddChange(const SyncData& data) { The callers generally have the element in this form. So instead of callers calling an additional function for transformation we let them pass what they have. On 2013/11/19 22:49:04, Ilya Sherman wrote: > This is a strange API. Why doesn't this function accept an > autofill::PasswordForm parameter instead? https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:125: const sync_pb::EntitySpecifics& specifics = data.GetSpecifics(); On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Looks like this variable is unused. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:160: void VerifyChange(const autofill::PasswordForm& password, That is an internal detail which the callers need not care about. Other than that the name is clear on what it does. But in any case I have added the comment. On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Docs, especially mentioning the side-effect (the erase() call). https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:164: = password_list->begin(); On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: The equals sign should be on the previous line. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:177: std::vector<autofill::PasswordForm> update_changes_; On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:177: std::vector<autofill::PasswordForm> update_changes_; On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Consider adding an "expected_" prefix to these variable names. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:179: }; They need to be friended to PasswordStore and PasswordSyncableService. On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: These two classes ought to be in the anonymous namespace as well. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:183: // Creates a sync data consisting of password specifics. The sign on real is On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: real -> realm Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:184: // set to |signon_realm|. On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Suggested rephrasing: Creates a SyncData instance with the given > |signon_realm|. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:195: class PasswordSyncableServiceTest : public testing::Test { On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: The test code itself should not be in the anonymous namespace, to avoid bad > interactions caused by C++ name mangling. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:216: } There will be test cases in future that pass in different parameter for error factory and set expectations on the error factory. This allows us to do that without breaking abstractions. On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: IMO a better API would be to provide a wrapper around > MergeDataAndStartSyncing. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:226: // FillBlacklistLogins. On 2013/11/19 22:49:04, Ilya Sherman wrote: > I'm not comfortable with checking in untested production code for blacklisted > logins. If you don't think it's appropriate to add test coverage as part of > this CL, please also hold off on adding the production code, and log a bug to > help make sure this doesn't get lost. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:263: scoped_ptr<syncer::SyncErrorFactory>()); On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Indentation is slightly off. Done. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:374: } // namespace It is verified when mergedataandstartsyncing is called. On 2013/11/19 22:49:04, Ilya Sherman wrote: > Please include test coverage to make sure that all of the PasswordSpecificsData > fields are copied correctly from Sync to the local datastore, and vice-versa. https://codereview.chromium.org/27233003/diff/633001/sync/api/sync_error.cc File sync/api/sync_error.cc (right): https://codereview.chromium.org/27233003/diff/633001/sync/api/sync_error.cc#n... sync/api/sync_error.cc:144: MODEL_TYPE_COUNT); It is already there. On 2013/11/19 22:49:04, Ilya Sherman wrote: > Please add this to histograms.xml as part of this CL. https://codereview.chromium.org/27233003/diff/633001/sync/api/sync_error.h File sync/api/sync_error.h (right): https://codereview.chromium.org/27233003/diff/633001/sync/api/sync_error.h#ne... sync/api/sync_error.h:78: static void LogDataLoadFailure(ModelType model_type); On 2013/11/19 22:49:04, Ilya Sherman wrote: > nit: Please document this method, and leave a blank line between this and the > "private:" access specifier. Done.
Apologies for the long delay in getting back to this review. I was perf sheriff last week, and have been furiously catching up on everything else this week. https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:70: specifics.password().client_only_encrypted_data()); Ok. That is a pretty confusing name for the field, since it stores *decrypted* data on the client side. Any chance of renaming it? (I understand that this is outside the scope of this CL.) On 2013/11/26 23:31:35, lipalani1 wrote: > This is the way things are stored in sync internally. > > Passwords data are encrypted before they are shared with the server. When > clients receive this encrypted data from the server, it will be decrypted and > this decrypted data(stored in client_only_encrypted_data) is used for populating > the password store. > > On 2013/11/19 22:49:04, Ilya Sherman wrote: > > Could you explain to me what |client_only_encrypted_data| means? In > > MergeLocalAndSyncPasswords(), it looks like you compare PasswordSpecificsData > to > > unencrypted local data. What does the "encrypted" part of the field name > mean, > > then? > https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.h (right): https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:74: friend class TestSyncChangeProcessor; My understanding is that you just want to be able to use the static methods CreateSyncData and ExtractPasswordFromSpecifics. If so, you can do this like so: class TestPasswordSyncableService : public PasswordSyncableService { public: TestPasswordSyncableService() {} virtual ~TestPasswordSyncableService() {} using PasswordSyncableService::CreateSyncData; using PasswordSyncableService::ExtractPasswordFromSpecifics; private: DISALLOW_COPY_AND_ASSIGN(TestPasswordSyncableService); }; Or, even better, you could do something like: # In password_syncable_service.cc namespace internal { syncer::SyncData CreateSyncData(const autofill::PasswordForm& password) { // impl } void ExtractPasswordFromSpecifics(...) { // impl } } // namespace internal # In password_syncable_service_unittest.cc // Forward-declare internal functions shared with tests: namespace internal { syncer::SyncData CreateSyncData(const autofill::PasswordForm& password); void ExtractPasswordFromSpecifics(...); } You can then call these free functions from the test code, but they are not exported as part of the header. You can then also tuck functions that need to call these into an anonymous namespace, if those functions themselves don't need to be exported. This isn't just a weird idea that I invented -- this was *the* pattern recommended to me by my C++ readability reviewer. On 2013/11/26 23:31:35, lipalani1 wrote: > These classes need to use private helpers to reduce their complexity. The > inheritance relationship does not make sense as they test something else.(i.e,. > we would have to do multiple inheritance). > > On 2013/11/19 22:49:04, Ilya Sherman wrote: > > Please do not add friend classes. Instead, use protected methods that tests > can > > override. > https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:160: void VerifyChange(const autofill::PasswordForm& password, This is a private method, so the only callers are the ones that care about internal details. This is an observable side-effect outside of this method, and a surprising one to at least one reader of this code (i.e. it surprised me). Please add docs to *this* method, i.e. VerifyChange(). On 2013/11/26 23:31:35, lipalani1 wrote: > That is an internal detail which the callers need not care about. > > Other than that the name is clear on what it does. But in any case I have added > the comment. > > On 2013/11/19 22:49:04, Ilya Sherman wrote: > > nit: Docs, especially mentioning the side-effect (the erase() call). > https://codereview.chromium.org/27233003/diff/633001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:226: // FillBlacklistLogins. On 2013/11/26 23:31:35, lipalani1 wrote: > On 2013/11/19 22:49:04, Ilya Sherman wrote: > > I'm not comfortable with checking in untested production code for blacklisted > > logins. If you don't think it's appropriate to add test coverage as part of > > this CL, please also hold off on adding the production code, and log a bug to > > help make sure this doesn't get lost. > > Done. Could you please annotate the corresponding production code with the tracking bug to restore this functionality? https://codereview.chromium.org/27233003/diff/633001/sync/api/sync_error.cc File sync/api/sync_error.cc (right): https://codereview.chromium.org/27233003/diff/633001/sync/api/sync_error.cc#n... sync/api/sync_error.cc:144: MODEL_TYPE_COUNT); Hmm, I don't see it included in the files modified by this CL, nor do I see "Sync.LocalDataFailedToLoad" listed in histograms.xml. What do you mean by "It is already there"? On 2013/11/26 23:31:35, lipalani1 wrote: > It is already there. > > On 2013/11/19 22:49:04, Ilya Sherman wrote: > > Please add this to histograms.xml as part of this CL. > https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:8: #include "base/memory/scoped_vector.h" nit: Redundant with the header's include. https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:9: #include "base/metrics/histogram.h" nit: unused. https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:10: #include "base/stl_util.h" nit: unused? https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:53: } nit: Please arrange the code in the implementation file to match the order in the header file. https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:73: // Check whether the data from sync is already in password store. nit: "in password store" -> "in the password store" https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:74: PasswordEntryMap::iterator umatched_data_from_password_db_iter = nit: Perhaps name this something more like "existing_local_entry" or "existing_entry_from_password_db"? https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:77: umatched_data_from_password_db->end()) { nit: Please indent this line four more spaces. https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:94: // than checking we simply update both. Is it actually possible that both need to be updated? If so, how? Looking at the code, it looks like one is always newer. If I'm understanding the code correctly, perhaps a more accurate comment would be something like: "Rather than checking which database -- sync or local -- needs updating, simply push an update to both. This will end up being a no-op for the database that didn't need an update." https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:150: // Changes from password db that needs to be propagated to sync. nit: "needs" -> "need" https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.cc:177: CreateSyncData(*(it->second)))); nit: Please align all three arguments to syncer::SyncChange(). https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service.h (right): https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:82: // in the passwords db. Depending on what action needs to be performed the nit: "needs to be performed the" -> "needs to be performed, the" https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:83: // entry may be added to |new_entries| or |updated_entries|. If the item is nit: "to |new_entries| or |updated_entries|" -> "to |new_entries| or to |updated_entries|" https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:84: // identical to an entry in passwords db, no action is performed. If an nit: "in passwords db" -> "in the passwords db" https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:85: // item needs to be updated in the sync database then the item is also nit: Might as well be consistent about "database" vs. "db" https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:85: // item needs to be updated in the sync database then the item is also nit: "in the sync database then" -> "in the sync database, then" https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service.h:87: // entry's tag in |umatched_data_from_password_db| then that entry nit: Spurious leading whitespace. https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:15: #include "chrome/browser/password_manager/password_syncable_service.h" nit: This include should be at the very top -- see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:49: class TestPasswordSyncableService : public PasswordSyncableService { nit: TestPasswordSyncableService -> MockPasswordSyncableService https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:170: // list. nit: Please document here as well that the entry will be removed if found. This is an externally observable side-effect, since a client calling UpdateChangeCount() before and after this method would see different results. https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:218: } nit: The anonymous namespace should end here. The actual test code should not be in the anonymous namespace, as otherwise the ensuing name-mangling can cause problems for our test runners. https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:312: scoped_ptr<syncer::SyncErrorFactory>()); nit: Please align params. https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:319: autofill::PasswordForm *form1 = new autofill::PasswordForm; nit: "autofill::PasswordForm *form1" -> "scoped_ptr<autofill::PasswordForm> form1" (applies throughout) https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:322: std::vector<autofill::PasswordForm*> forms; nit: ScopedVector (applies throughout) https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:325: SyncDataList list; nit: You could just pass SyncDataList() on line 333. https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:357: scoped_ptr<syncer::SyncErrorFactory>()); nit: Align args. https://codereview.chromium.org/27233003/diff/743001/chrome/browser/password_... chrome/browser/password_manager/password_syncable_service_unittest.cc:392: scoped_ptr<syncer::SyncErrorFactory>()); nit: Align args. https://codereview.chromium.org/27233003/diff/743001/sync/api/sync_error.h File sync/api/sync_error.h (right): https://codereview.chromium.org/27233003/diff/743001/sync/api/sync_error.h#ne... sync/api/sync_error.h:79: static void LogDataLoadFailure(ModelType model_type); nit: Please leave a blank line after this one. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
