Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
139443004:
Password sync refactoring: implemented ProcessSyncChanges() and GetAllSyncData() (Closed)
|
Created:
6 years, 10 months ago by vasilii Modified:
6 years, 10 months ago CC:
chromium-reviews, Patrick Dubroy Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPassword sync refactoring: implemented ProcessSyncChanges() and GetAllSyncData()
BUG=95758
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250944
Patch Set 1 #
Total comments: 46
Patch Set 2 : Addressed comments #
Total comments: 9
Patch Set 3 : Fixed test crash + comments #Patch Set 4 : Added DCHECK() into ProcessSyncChanges() #
Messages
Total messages: 13 (0 generated)
Hi guys, please review this CL.
https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:211: if (ReadFromPasswordStore(&password_db_entries, It seems expensive to read all passwords from the password store every time a sync change arrives. Can we do better? Either by caching the metadata about passwords, or adding support for only reading the individual password affected? https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:275: password_store_->Shutdown(); Can you move SetUp and TearDown into the constructor/destructor? Then you don't have to have SetUp and TearDown in the test class either https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:552: TEST_F(PasswordSyncableServiceTest, MergeDataAndPushBack) { Nice.
Thanks for picking up this work! Apologies for not replying to your other code review fast enough -- I've had a huge pile of stuff to catch up on after getting back from vacation. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... File chrome/browser/password_manager/password_syncable_service.cc (left): https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:136: // removed and this list will only contain entries that are not in sync. This was a useful comment, IMO. It would be nice to preserve it. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:185: for (PasswordForms::iterator i = password_entries.begin(); nit: i -> it https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:211: if (ReadFromPasswordStore(&password_db_entries, On 2014/02/07 21:16:47, Nicolas Zea wrote: > It seems expensive to read all passwords from the password store every time a > sync change arrives. Can we do better? Either by caching the metadata about > passwords, or adding support for only reading the individual password affected? FWIW, my vote is against caching unless we really can't get the desired performance without it -- I've found that it tends to introduce lots of complexity and, consequently, buginess. It might indeed make sense to only read the individual passwords that are affected. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:224: // whether this sync item is an addition or update. Hmm, why is this needed? That is, why is it not safe to assume that the Sync change type is correct at this point? https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:231: } nit: Leave a blank line between cases, IMO. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:237: autofill::PasswordForm* new_password = new autofill::PasswordForm; nit: I prefer to immediately wrap all |new|s in scoped_ptr's or the like, and then explicitly transfer ownership. This is a nice habit to get into for quick and easy correct memory management. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:242: default: { nit: Rather than a default case, please have an explicit case for ACTION_INVALID. That way, if the enum ever grows, the compiler will remind us to think about updating this switch stmt. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:246: } nit: No need for curly braces. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:274: DCHECK(password_entries); nit: No need for this, IMO, since you dereference the variable on the next line. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:276: !password_store_->FillBlacklistLogins(&password_entries->get())) { Have you added test coverage for blacklisted logins? https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... File chrome/browser/password_manager/password_syncable_service_unittest.cc (left): https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:123: // |expected_db_update_changes_| list. Rather than removing this comment, please expand it to be more like the comment on lines 116-118. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:205: for (std::vector<autofill::PasswordForm*>::const_iterator i = nit: i -> it https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:282: scoped_ptr<MockPasswordSyncableService>& service() { Hmm, returning a scoped_ptr by reference is pretty unusual. Would it make sense to return a raw pointer here instead? https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:487: autofill::PasswordForm *form1 = new autofill::PasswordForm; nit: Please use a scoped_ptr here. If for some reason you're super opposed to using a scoped_ptr, then please swap the space and the asterisk (" *form1" -> "* form1"). https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:491: autofill::PasswordForm *form2 = new autofill::PasswordForm; Ditto. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:507: EXPECT_CALL(*service(), NotifyPasswordStoreOfLoginChanges(_)); nit: Please move this to be roughly right above the call that triggers this expectation. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:522: autofill::PasswordForm *form1 = new autofill::PasswordForm; Ditto. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:525: autofill::PasswordForm *form2 = new autofill::PasswordForm; Ditto. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:553: autofill::PasswordForm *form1 = new autofill::PasswordForm; Ditto.
https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... File chrome/browser/password_manager/password_syncable_service.cc (left): https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:136: // removed and this list will only contain entries that are not in sync. On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > This was a useful comment, IMO. It would be nice to preserve it. Done. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:185: for (PasswordForms::iterator i = password_entries.begin(); On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > nit: i -> it Done. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:211: if (ReadFromPasswordStore(&password_db_entries, On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > On 2014/02/07 21:16:47, Nicolas Zea wrote: > > It seems expensive to read all passwords from the password store every time a > > sync change arrives. Can we do better? Either by caching the metadata about > > passwords, or adding support for only reading the individual password > affected? > > FWIW, my vote is against caching unless we really can't get the desired > performance without it -- I've found that it tends to introduce lots of > complexity and, consequently, buginess. It might indeed make sense to only read > the individual passwords that are affected. This implementation isn't slower than the existing one. I'm not sure that this class is a proper place for the cache. Did you mean reading individual passwords through GetLoginsImpl()? Should it be part of this CL? https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:224: // whether this sync item is an addition or update. On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > Hmm, why is this needed? That is, why is it not safe to assume that the Sync > change type is correct at this point? It's safe. I just reuse the existing method. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:231: } On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > nit: Leave a blank line between cases, IMO. Done. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:237: autofill::PasswordForm* new_password = new autofill::PasswordForm; On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > nit: I prefer to immediately wrap all |new|s in scoped_ptr's or the like, and > then explicitly transfer ownership. This is a nice habit to get into for quick > and easy correct memory management. Done. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:242: default: { On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > nit: Rather than a default case, please have an explicit case for > ACTION_INVALID. That way, if the enum ever grows, the compiler will remind us > to think about updating this switch stmt. Done. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:246: } On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > nit: No need for curly braces. Done. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:274: DCHECK(password_entries); On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > nit: No need for this, IMO, since you dereference the variable on the next line. For better log readability? https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:276: !password_store_->FillBlacklistLogins(&password_entries->get())) { On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > Have you added test coverage for blacklisted logins? I modified PasswordSyncableServiceTest.ProcessSyncChanges and PasswordSyncableServiceTest.GetAllSyncData https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... File chrome/browser/password_manager/password_syncable_service_unittest.cc (left): https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:123: // |expected_db_update_changes_| list. On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > Rather than removing this comment, please expand it to be more like the comment > on lines 116-118. Done. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:205: for (std::vector<autofill::PasswordForm*>::const_iterator i = On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > nit: i -> it Done. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:275: password_store_->Shutdown(); On 2014/02/07 21:16:47, Nicolas Zea wrote: > Can you move SetUp and TearDown into the constructor/destructor? Then you don't > have to have SetUp and TearDown in the test class either Done. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:282: scoped_ptr<MockPasswordSyncableService>& service() { On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > Hmm, returning a scoped_ptr by reference is pretty unusual. Would it make sense > to return a raw pointer here instead? In PasswordSyncableServiceTest.MergeDataAndPushBack I have to transfer ownership of this object to MergeDataAndStartSyncing(). https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:487: autofill::PasswordForm *form1 = new autofill::PasswordForm; On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > nit: Please use a scoped_ptr here. If for some reason you're super opposed to > using a scoped_ptr, then please swap the space and the asterisk (" *form1" -> "* > form1"). Done. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:491: autofill::PasswordForm *form2 = new autofill::PasswordForm; On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > Ditto. Done. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:507: EXPECT_CALL(*service(), NotifyPasswordStoreOfLoginChanges(_)); On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > nit: Please move this to be roughly right above the call that triggers this > expectation. Done. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:522: autofill::PasswordForm *form1 = new autofill::PasswordForm; On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > Ditto. Done. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:525: autofill::PasswordForm *form2 = new autofill::PasswordForm; On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > Ditto. Done. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:552: TEST_F(PasswordSyncableServiceTest, MergeDataAndPushBack) { On 2014/02/07 21:16:47, Nicolas Zea wrote: > Nice. Done. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:553: autofill::PasswordForm *form1 = new autofill::PasswordForm; On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > Ditto. Done.
Thanks, LGTM with the remaining comments/nits addressed. https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service.cc:276: !password_store_->FillBlacklistLogins(&password_entries->get())) { On 2014/02/10 19:06:50, vasilii wrote: > On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > > Have you added test coverage for blacklisted logins? > > I modified PasswordSyncableServiceTest.ProcessSyncChanges and > PasswordSyncableServiceTest.GetAllSyncData Lovely, thanks :) https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:282: scoped_ptr<MockPasswordSyncableService>& service() { On 2014/02/10 19:06:50, vasilii wrote: > On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > > Hmm, returning a scoped_ptr by reference is pretty unusual. Would it make > sense > > to return a raw pointer here instead? > > In PasswordSyncableServiceTest.MergeDataAndPushBack I have to transfer ownership > of this object to MergeDataAndStartSyncing(). I see. It would be better -- in that it's less surprising to others reading the code -- to separate the simple accessor method from the method that can transfer ownership. So, I'd recommend having two methods: MockPasswordSyncableService* service(); // Document that this has the side-effect of NULLing out the service_. scoped_ptr<syncer::SyncChangeProcessor> GetSyncChangeProcessor(); https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... chrome/browser/password_manager/password_syncable_service.cc:199: // add/update change. nit: Now only on an update change, right? https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... chrome/browser/password_manager/password_syncable_service.cc:236: &sync_changes); Would be nice to DCHECK that sync_changes remains empty after this call. https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... chrome/browser/password_manager/password_syncable_service_unittest.cc:211: for (std::vector<autofill::PasswordForm*>::const_iterator i = nit: i -> it
https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/139443004/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_syncable_service_unittest.cc:282: scoped_ptr<MockPasswordSyncableService>& service() { On 2014/02/11 07:59:45, Ilya Sherman (catching up...) wrote: > On 2014/02/10 19:06:50, vasilii wrote: > > On 2014/02/07 22:56:35, Ilya Sherman (catching up...) wrote: > > > Hmm, returning a scoped_ptr by reference is pretty unusual. Would it make > > sense > > > to return a raw pointer here instead? > > > > In PasswordSyncableServiceTest.MergeDataAndPushBack I have to transfer > ownership > > of this object to MergeDataAndStartSyncing(). > > I see. It would be better -- in that it's less surprising to others reading the > code -- to separate the simple accessor method from the method that can transfer > ownership. So, I'd recommend having two methods: > > MockPasswordSyncableService* service(); > > // Document that this has the side-effect of NULLing out the service_. > scoped_ptr<syncer::SyncChangeProcessor> GetSyncChangeProcessor(); Done. https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... chrome/browser/password_manager/password_syncable_service.cc:199: // add/update change. On 2014/02/11 07:59:46, Ilya Sherman (catching up...) wrote: > nit: Now only on an update change, right? Done. https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... chrome/browser/password_manager/password_syncable_service.cc:236: &sync_changes); On 2014/02/11 07:59:46, Ilya Sherman (catching up...) wrote: > Would be nice to DCHECK that sync_changes remains empty after this call. CreateOrUpdateEntry() is written in the way that it pushes updates to both sync and local db. Do you think it should be fixed? https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... File chrome/browser/password_manager/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... chrome/browser/password_manager/password_syncable_service_unittest.cc:211: for (std::vector<autofill::PasswordForm*>::const_iterator i = On 2014/02/11 07:59:46, Ilya Sherman (catching up...) wrote: > nit: i -> it Done.
lgtm
https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... chrome/browser/password_manager/password_syncable_service.cc:236: &sync_changes); On 2014/02/11 12:23:33, vasilii wrote: > On 2014/02/11 07:59:46, Ilya Sherman (catching up...) wrote: > > Would be nice to DCHECK that sync_changes remains empty after this call. > > CreateOrUpdateEntry() is written in the way that it pushes updates to both sync > and local db. Do you think it should be fixed? Ah. Yes, I do think it should be fixed -- it's unnecessary to generate the extra updates, and it can make invariants like this one clearer.
https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... chrome/browser/password_manager/password_syncable_service.cc:236: &sync_changes); On 2014/02/11 21:58:25, Ilya Sherman wrote: > On 2014/02/11 12:23:33, vasilii wrote: > > On 2014/02/11 07:59:46, Ilya Sherman (catching up...) wrote: > > > Would be nice to DCHECK that sync_changes remains empty after this call. > > > > CreateOrUpdateEntry() is written in the way that it pushes updates to both > sync > > and local db. Do you think it should be fixed? > > Ah. Yes, I do think it should be fixed -- it's unnecessary to generate the > extra updates, and it can make invariants like this one clearer. Done.
The CQ bit was checked by vasilii@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vasilii@chromium.org/139443004/360001
https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... File chrome/browser/password_manager/password_syncable_service.cc (right): https://codereview.chromium.org/139443004/diff/140001/chrome/browser/password... chrome/browser/password_manager/password_syncable_service.cc:236: &sync_changes); On 2014/02/12 13:27:12, vasilii wrote: > On 2014/02/11 21:58:25, Ilya Sherman wrote: > > On 2014/02/11 12:23:33, vasilii wrote: > > > On 2014/02/11 07:59:46, Ilya Sherman (catching up...) wrote: > > > > Would be nice to DCHECK that sync_changes remains empty after this call. > > > > > > CreateOrUpdateEntry() is written in the way that it pushes updates to both > > sync > > > and local db. Do you think it should be fixed? > > > > Ah. Yes, I do think it should be fixed -- it's unnecessary to generate the > > extra updates, and it can make invariants like this one clearer. > > Done. Thanks! (Still LGTM, so the CQ doesn't freak out.)
Message was sent while issue was closed.
Change committed as 250944 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
