Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7819002:
Migrate AutofillProfile sync to new API. (Closed)
|
Created:
9 years, 3 months ago by GeorgeY Modified:
9 years, 3 months ago CC:
chromium-reviews, ncarter (slow), idana, Raghu Simha, Paweł Hajdan Jr., dhollowa Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionMigrate AutofillProfile sync to new API.
Still need to remove autofill_model_associator.cc/h - used for autocomplete
TEST=unit-tested + all AutofillProfile sync should work correctly.
BUG=83782
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100782
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 96
Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 36
Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 6
Patch Set 9 : '' #
Total comments: 9
Patch Set 10 : '' #Patch Set 11 : '' #Patch Set 12 : '' #Patch Set 13 : '' #
Total comments: 8
Messages
Total messages: 31 (0 generated)
Still working on unit-tests, so do not check them yet...
All should work!
http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:161: } I am concerned about the perf impact here Cam you keep the mapping in memory? When a change comes in you should be able to acccess the profile corresponding to the guid and process the action told by the server rather than iterating all over again. MergeDataAndStartSyncing had already done that. May be if the change is an update and the corresponding guid is not found then in that case we can create a new one. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:359: profile_map->erase(i); Profile is only erased from the map. Who is erasing it from the webdata? we want it gone in the web data too otherwiser we will have duplicates. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:391: if (error.IsSet()) { may be this is a vlog(warning)? http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc:104: .WillOnce(DoAll(CopyData(&profiles_from_web_db), Return(true))); Dont you want to verify the data that is being sent here?
http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:33: NotificationService::AllSources()); Are you sure this should be AllSources() rather than a specific chrome user profile? (Maybe ping erg@ if you're not sure.) http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:47: NotificationService::AllSources()); Ditto. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:53: // not so declared. nit: Can we propagate const properly rather than casting away constness here? http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:313: ScopedVector<AutofillProfile>* profiles, GuidToProfileMap* profile_map) { nit: |profiles| should probably be passed by const reference. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:322: const SyncData& data, ScopedVector<AutofillProfile>* profiles, nit: |profiles| does not seem to be used in this function. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:365: added_new_profile = true; I'm a little confused here: If the profile appears under a different guid, we want to *both* update bundle->profiles_to_delete and bundle->new_profiles? That seems to be what the current code does, but I don't understand why that's appropriate.
http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:233: AutofillProfile* merge_into, Rename |merge_into| as |profile| and get rid of temporary |p| below. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:276: void AutofillProfileSyncableService::WriteAutofillProfile( nit: please reorder method to match the .h file. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:323: GuidToProfileMap* profile_map, DataBundle *bundle) { nit: s/ */* / http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:345: AutofillProfile *p(new AutofillProfile(autofill_specifics.guid())); nit: s/ */* / http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:31: nit: please remove extra line. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:69: const NotificationDetails& details); nit: OVERRIDE http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:73: static bool OverwriteProfileWithServerData( Since these are helpers for unit tests, please make them private and add the unit tests as friends. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:74: AutofillProfile* merge_into, nit: output parameters come after input parameters. Also let's just call the parameter |profile| instead of |merge_into| and get rid of the temporary |p| variable in the implementation. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:81: protected: It looks like too much is "protected". Only |DataBundle|, |LoadAutofillData| and |SaveChangesToWebData| need to be. The rest can be private. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:83: GuidToProfileMap; nit: To keep consistent with webdata and other autofill code please use all-caps "GUID" throughout. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:101: SyncChange CreateChange(SyncChange::SyncChangeType action, It appears that all of these "Create*" methods don't need object state. So they'd be better moved into the .cc file in an anonymous namespace as helper methods. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:123: DataBundle *bundle); nit: s/ */* / http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:142: // struct DataBundle; nit: Looks like this comment should go. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:143: static bool MergeField(FormGroup* f, This could also be moved into the .cc file in an anonymous namespace. Also, please use more descriptive names: s/f/form_group/, s/t/field_type/. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:152: int number_of_profiles_created_; I don't see where this is used. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:157: struct AutofillProfileSyncableService::DataBundle { Move this to the .cc file. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:163: std::vector<AutofillProfile*> new_profiles; // We own these pointers. If these are owned, how about using a scoped vector? http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc:40: AutoSetSyncProcessor(MockAutofillProfileSyncableService *service, nit: s/ */* / http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc:48: MockAutofillProfileSyncableService *service_; nit: s/ */* / http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc:180: nit: remove extra blank line. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:199: ProfileSyncFactory *factory, nit: s/ */* /
http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:33: NotificationService::AllSources()); On 2011/09/01 07:22:31, Ilya Sherman wrote: > Are you sure this should be AllSources() rather than a specific chrome user > profile? (Maybe ping erg@ if you're not sure.) Yes, it seems like this code should only listen/respond to notification sent out by the WebDataService that fronts the |web_database_| we have here. This may mean registering something like: notification_registrar_.Add(this, NAPC, Source<WebDataService>(profile->wds)); or notification_registrar_.Add(this, NAPC, Source<WebDataService>(WDSForWDB(web_database_))); I'm seriously hand waving of course... But the danger is that we process change requests for ALL web databases when only one has changed. Oh wait... it looks like you are handling this properly in ::Observe by checking the correspondence between the database and the data service there. Right? http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:37: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB)); It is fine to leave this here... but I believe it is redundant given that |this| inherits from |base::NonThreadSafe|. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:53: // not so declared. On 2011/09/01 07:22:31, Ilya Sherman wrote: > nit: Can we propagate const properly rather than casting away constness here? +1 http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:54: if (!const_cast<WebDatabase*>(web_database_)-> nit: how about simply: return web_database_->...; and remove the if/return false/return true; bits. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:101: CreateOrUpdateProfile(*sync_iter, &profiles, &remaining_profiles, &bundle); nit: style guide requires { } for multi-line for(). http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:321: bool AutofillProfileSyncableService::CreateOrUpdateProfile( The name of this function doesn't really reflect what it does. Maybe we could call it |PartitionProfiles| or something? Also it seems a bit cleaner conceptually to put the "remaining" profiles into the bundle in an addition member |new_sync_profiles| maybe? http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:372: DCHECK(change->type() == AutofillProfileChange::REMOVE || change->profile()); This DCHECK is not quite accurate. Better would be: DCHECK(type == REMOVE && !change->profile() || type == ADD && change->profile() || type == UPDATE && change->profile()); http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:378: CreateChange(SyncChange::ACTION_ADD, *(change->profile()))); These CreateChange and CreateDeleteChange helpers seem unnecessary. They don't really reduce the code size over simply calling the SyncChange(...) constructor directly. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:162: std::vector<AutofillProfile*> updated_profiles; profiles_to_update? http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:163: std::vector<AutofillProfile*> new_profiles; // We own these pointers. profiles_to_add?
http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc:25: : public browser_sync::AutofillProfileSyncableService { nit: indent 4 spaces http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc:36: // Helper for tests that do not need to setup service completely throught the s/throught/through/
http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:33: NotificationService::AllSources()); On 2011/09/01 07:22:31, Ilya Sherman wrote: > Are you sure this should be AllSources() rather than a specific chrome user > profile? (Maybe ping erg@ if you're not sure.) That call is moved from original change processor, but AFAIK it would do exactly what we want - we get notification only from logged in profile, as that is the only one that can change autofill locally. Fixed anyway :) http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:37: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB)); On 2011/09/01 23:11:46, dhollowa wrote: > It is fine to leave this here... but I believe it is redundant given that |this| > inherits from |base::NonThreadSafe|. Yes, could be changed to DCHECK(CalledOnValidThread()); That code checks that thread is the same as the one constructor was executed on. So at least in constructo it needs to be DB thread check Changed here, though http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:47: NotificationService::AllSources()); On 2011/09/01 07:22:31, Ilya Sherman wrote: > Ditto. Ditto. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:53: // not so declared. On 2011/09/01 07:22:31, Ilya Sherman wrote: > nit: Can we propagate const properly rather than casting away constness here? Didn't do it because: #1. The internal state of db object that is eventually called is mutable - we would've needed to re-declare some SQL accessors as well. #2. it is used in 15 files and ~60 places total, so it would've been a huge cl, that would pollute this one. #3 Everything is cached for speed now, so this code is removed http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:101: CreateOrUpdateProfile(*sync_iter, &profiles, &remaining_profiles, &bundle); On 2011/09/01 23:11:46, dhollowa wrote: > nit: style guide requires { } for multi-line for(). AFAIK, {} are required *only* if executing statement more than one line, if the condition is more than one line brackets are optional. Some developers (including Peter) are against using curly brackets in this case http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:161: } On 2011/09/01 05:06:02, lipalani1 wrote: > I am concerned about the perf impact here Cam you keep the mapping in memory? > When a change comes in you should be able to acccess the profile corresponding > to the guid and process the action told by the server rather than iterating all > over again. MergeDataAndStartSyncing had already done that. > > May be if the change is an update and the corresponding guid is not found then > in that case we can create a new one. Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:233: AutofillProfile* merge_into, On 2011/09/01 20:17:49, dhollowa wrote: > Rename |merge_into| as |profile| and get rid of temporary |p| below. Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:276: void AutofillProfileSyncableService::WriteAutofillProfile( On 2011/09/01 20:17:49, dhollowa wrote: > nit: please reorder method to match the .h file. Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:313: ScopedVector<AutofillProfile>* profiles, GuidToProfileMap* profile_map) { On 2011/09/01 07:22:31, Ilya Sherman wrote: > nit: |profiles| should probably be passed by const reference. Nope. I needed ::iterator, not ::const_iterator. Changed to index anyway as it guaranteed to be stable if vector grows. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:322: const SyncData& data, ScopedVector<AutofillProfile>* profiles, On 2011/09/01 07:22:31, Ilya Sherman wrote: > nit: |profiles| does not seem to be used in this function. Was left from older version - removed. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:323: GuidToProfileMap* profile_map, DataBundle *bundle) { On 2011/09/01 20:17:49, dhollowa wrote: > nit: s/ */* / Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:345: AutofillProfile *p(new AutofillProfile(autofill_specifics.guid())); On 2011/09/01 20:17:49, dhollowa wrote: > nit: s/ */* / Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:359: profile_map->erase(i); On 2011/09/01 05:06:02, lipalani1 wrote: > Profile is only erased from the map. Who is erasing it from the webdata? we want > it gone in the web data too otherwiser we will have duplicates. It is erased from web_db with a call to DataBundle up the stack. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:365: added_new_profile = true; On 2011/09/01 07:22:31, Ilya Sherman wrote: > I'm a little confused here: If the profile appears under a different guid, we > want to *both* update bundle->profiles_to_delete and bundle->new_profiles? That > seems to be what the current code does, but I don't understand why that's > appropriate. We do not want to have multiple copies of a profile, so if two profiles are the same we substitute present one with the one from the sync. Old code did it as well. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:372: DCHECK(change->type() == AutofillProfileChange::REMOVE || change->profile()); On 2011/09/01 23:11:46, dhollowa wrote: > This DCHECK is not quite accurate. Better would be: > > DCHECK(type == REMOVE && !change->profile() || > type == ADD && change->profile() || > type == UPDATE && change->profile()); Further down we assert on any change that is not ADD/UPDATE/REMOVE So my assert is accurate http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:378: CreateChange(SyncChange::ACTION_ADD, *(change->profile()))); On 2011/09/01 23:11:46, dhollowa wrote: > These CreateChange and CreateDeleteChange helpers seem unnecessary. They don't > really reduce the code size over simply calling the SyncChange(...) constructor > directly. sure http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:391: if (error.IsSet()) { On 2011/09/01 05:06:02, lipalani1 wrote: > may be this is a vlog(warning)? Sure. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:31: On 2011/09/01 20:17:49, dhollowa wrote: > nit: please remove extra line. Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:69: const NotificationDetails& details); On 2011/09/01 20:17:49, dhollowa wrote: > nit: OVERRIDE Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:73: static bool OverwriteProfileWithServerData( On 2011/09/01 20:17:49, dhollowa wrote: > Since these are helpers for unit tests, please make them private and add the > unit tests as friends. Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:74: AutofillProfile* merge_into, On 2011/09/01 20:17:49, dhollowa wrote: > nit: output parameters come after input parameters. Also let's just call the > parameter |profile| instead of |merge_into| and get rid of the temporary |p| > variable in the implementation. Copied verbatim from the old file - changed, sure. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:81: protected: On 2011/09/01 20:17:49, dhollowa wrote: > It looks like too much is "protected". Only |DataBundle|, |LoadAutofillData| > and |SaveChangesToWebData| need to be. The rest can be private. Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:83: GuidToProfileMap; On 2011/09/01 20:17:49, dhollowa wrote: > nit: To keep consistent with webdata and other autofill code please use all-caps > "GUID" throughout. Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:101: SyncChange CreateChange(SyncChange::SyncChangeType action, On 2011/09/01 20:17:49, dhollowa wrote: > It appears that all of these "Create*" methods don't need object state. So > they'd be better moved into the .cc file in an anonymous namespace as helper > methods. Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:123: DataBundle *bundle); On 2011/09/01 20:17:49, dhollowa wrote: > nit: s/ */* / Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:142: // struct DataBundle; On 2011/09/01 20:17:49, dhollowa wrote: > nit: Looks like this comment should go. Done. Remnants from the old file http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:143: static bool MergeField(FormGroup* f, On 2011/09/01 20:17:49, dhollowa wrote: > This could also be moved into the .cc file in an anonymous namespace. Also, > please use more descriptive names: s/f/form_group/, s/t/field_type/. Again, most of supporting functions were copied verbatim from the old file. Sure - changed the names http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:152: int number_of_profiles_created_; On 2011/09/01 20:17:49, dhollowa wrote: > I don't see where this is used. Removed - was used in old unit-test. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:157: struct AutofillProfileSyncableService::DataBundle { On 2011/09/01 20:17:49, dhollowa wrote: > Move this to the .cc file. Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:163: std::vector<AutofillProfile*> new_profiles; // We own these pointers. On 2011/09/01 20:17:49, dhollowa wrote: > If these are owned, how about using a scoped vector? Originall class from Model* Not anymore :). http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc:40: AutoSetSyncProcessor(MockAutofillProfileSyncableService *service, On 2011/09/01 20:17:49, dhollowa wrote: > nit: s/ */* / Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc:48: MockAutofillProfileSyncableService *service_; On 2011/09/01 20:17:49, dhollowa wrote: > nit: s/ */* / Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc:104: .WillOnce(DoAll(CopyData(&profiles_from_web_db), Return(true))); On 2011/09/01 05:06:02, lipalani1 wrote: > Dont you want to verify the data that is being sent here? CopyData(&profiles_from_web_db) puts data into the parameter - that it is actually working is checked at line 111. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc:180: On 2011/09/01 20:17:49, dhollowa wrote: > nit: remove extra blank line. Done. Either this, or Lint complains :) http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:199: ProfileSyncFactory *factory, On 2011/09/01 20:17:49, dhollowa wrote: > nit: s/ */* / Done. and in other places in this file
http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:102: GUIDToProfileMap:: iterator it = nit: Extraneous space after ":: " http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:104: profiles_map_[it->first] = it->second; Are you sure you want to update the map here, rather than waiting for a change notification from the WDS? http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:151: profiles_[i->second]->guid(), profiles_[i->second]->guid(), specifics)); nit: I think you can call into CreateData() here. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:205: if (!wds || wds->GetDatabase() != web_database_) nit: Can this be a DCHECK instead? http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:287: // not optimal.) nit: I do not understand this comment. What's special about this DCHECK vs. other DCHECKs? http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:344: AutofillProfile* p(new AutofillProfile(autofill_specifics.guid())); nit: Please give the variable a more complete name than "p" :) http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:388: CreateData(*(change->profile())))); I believe you need to update the cached profiles in response to this change. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:402: VLOG(1) << "[AUTOFILL SYNC]" nit: Does this happen commonly enough to where LOG(WARNING) or even LOG(ERROR) is inappropriate? http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:45: // merge and sync. nit: This comment seems really out of context here. Perhaps it should be located with the implementation instead; and a replaced with a more general, high-level comment here? http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:76: typedef std::map<std::string, size_t> GUIDToProfileMap; nit: Can this be moved to "private:"? http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:76: typedef std::map<std::string, size_t> GUIDToProfileMap; nit: Why map to an index rather than to an |AutofillProfile*| ? http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:86: virtual bool LoadAutofillData(std::vector<AutofillProfile*>* profiles) const; nit: Probably worth mentioning in the function comment that the caller owns the returned |AutofillProfile|'s. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:114: void CreateGUIDToProfileMap(const ScopedVector<AutofillProfile>& profiles, nit: Passing around ScopedVectors, even by reference, makes me a little bit anxious. Can you change this to const std::vector<AutofillProfile>& instead? http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:121: // iterator pointing to added/updated profile. nit: There are several sentence breaks in this comment that are missing punctuation and/or capitalization. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:126: void ActOnChange(AutofillProfileChange* change); nit: Should |change| be passed by const-ref here? http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:151: struct AutofillProfileSyncableService::DataBundle { nit: Quoth David, "Move this to the .cc file". Quoth you, "Done." But it's still here ;)
http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:102: GUIDToProfileMap:: iterator it = On 2011/09/02 21:49:50, Ilya Sherman wrote: > nit: Extraneous space after ":: " Done. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:104: profiles_map_[it->first] = it->second; On 2011/09/02 21:49:50, Ilya Sherman wrote: > Are you sure you want to update the map here, rather than waiting for a change > notification from the WDS? Yes. Any changes from Sync service and WDS will be processed quicker. We go through all of them anyway here http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:151: profiles_[i->second]->guid(), profiles_[i->second]->guid(), specifics)); On 2011/09/02 21:49:50, Ilya Sherman wrote: > nit: I think you can call into CreateData() here. Done. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:205: if (!wds || wds->GetDatabase() != web_database_) On 2011/09/02 21:49:50, Ilya Sherman wrote: > nit: Can this be a DCHECK instead? sure http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:287: // not optimal.) On 2011/09/02 21:49:50, Ilya Sherman wrote: > nit: I do not understand this comment. What's special about this DCHECK vs. > other DCHECKs? Not mine - from the old file. Came with the function - removed. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:344: AutofillProfile* p(new AutofillProfile(autofill_specifics.guid())); On 2011/09/02 21:49:50, Ilya Sherman wrote: > nit: Please give the variable a more complete name than "p" :) Done. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:388: CreateData(*(change->profile())))); On 2011/09/02 21:49:50, Ilya Sherman wrote: > I believe you need to update the cached profiles in response to this change. true. Fixed. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:402: VLOG(1) << "[AUTOFILL SYNC]" On 2011/09/02 21:49:50, Ilya Sherman wrote: > nit: Does this happen commonly enough to where LOG(WARNING) or even LOG(ERROR) > is inappropriate? Done. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:76: typedef std::map<std::string, size_t> GUIDToProfileMap; On 2011/09/02 21:49:50, Ilya Sherman wrote: > nit: Why map to an index rather than to an |AutofillProfile*| ? All the Load/Save accessors use vector<AutofillProfile*>, so doing that would've required coping to and from before each operation. As for index instead of an iterator - it is guaranteed to be immutable to addition operation of the vector in all std:: implementations. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:76: typedef std::map<std::string, size_t> GUIDToProfileMap; On 2011/09/02 21:49:50, Ilya Sherman wrote: > nit: Can this be moved to "private:"? Done. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:86: virtual bool LoadAutofillData(std::vector<AutofillProfile*>* profiles) const; On 2011/09/02 21:49:50, Ilya Sherman wrote: > nit: Probably worth mentioning in the function comment that the caller owns the > returned |AutofillProfile|'s. Done. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:114: void CreateGUIDToProfileMap(const ScopedVector<AutofillProfile>& profiles, On 2011/09/02 21:49:50, Ilya Sherman wrote: > nit: Passing around ScopedVectors, even by reference, makes me a little bit > anxious. Can you change this to const std::vector<AutofillProfile>& instead? Done. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:121: // iterator pointing to added/updated profile. On 2011/09/02 21:49:50, Ilya Sherman wrote: > nit: There are several sentence breaks in this comment that are missing > punctuation and/or capitalization. Done. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:126: void ActOnChange(AutofillProfileChange* change); On 2011/09/02 21:49:50, Ilya Sherman wrote: > nit: Should |change| be passed by const-ref here? Done. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:151: struct AutofillProfileSyncableService::DataBundle { On 2011/09/02 21:49:50, Ilya Sherman wrote: > nit: Quoth David, "Move this to the .cc file". Quoth you, "Done." But it's > still here ;) Quoth me :): it is used in unit-tests, so it needs to be in header. "Done" meant checked that it needed here and so left it :)
http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:104: profiles_map_[it->first] = it->second; On 2011/09/03 00:01:29, GeorgeY wrote: > On 2011/09/02 21:49:50, Ilya Sherman wrote: > > Are you sure you want to update the map here, rather than waiting for a change > > notification from the WDS? > > Yes. Any changes from Sync service and WDS will be processed quicker. We go > through all of them anyway here But there's a chance that the SaveChangesToWebData() call can fail, right? http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:76: typedef std::map<std::string, size_t> GUIDToProfileMap; On 2011/09/03 00:01:29, GeorgeY wrote: > On 2011/09/02 21:49:50, Ilya Sherman wrote: > > nit: Why map to an index rather than to an |AutofillProfile*| ? > All the Load/Save accessors use vector<AutofillProfile*>, so doing that would've > required coping to and from before each operation. I don't follow what you mean here. Storing the index means that you need to index into |profiles_| to pull out the profile. Storing the pointer lets you skip the indexing, but is otherwise syntactically equivalent as far as I can tell. Storing the pointer also seems a bit semantically clearer to me. http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:151: struct AutofillProfileSyncableService::DataBundle { On 2011/09/03 00:01:29, GeorgeY wrote: > On 2011/09/02 21:49:50, Ilya Sherman wrote: > > nit: Quoth David, "Move this to the .cc file". Quoth you, "Done." But it's > > still here ;) > > Quoth me :): it is used in unit-tests, so it needs to be in header. > "Done" meant checked that it needed here and so left it :) Ok :) http://codereview.chromium.org/7819002/diff/18001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/18001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:305: const std::vector<AutofillProfile *>& profiles, Nit: No space before the "*" http://codereview.chromium.org/7819002/diff/18001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/18001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:7: // ProcessSyncChanges() and for each local change Observe() is called. nit: This comment should still be right above the class definition though -- it's not part of the license ;) http://codereview.chromium.org/7819002/diff/18001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:115: void CreateGUIDToProfileMap(const std::vector<AutofillProfile *>& profiles, nit: No space before the "*"
All done http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:104: profiles_map_[it->first] = it->second; On 2011/09/03 00:10:57, Ilya Sherman wrote: > On 2011/09/03 00:01:29, GeorgeY wrote: > > On 2011/09/02 21:49:50, Ilya Sherman wrote: > > > Are you sure you want to update the map here, rather than waiting for a > change > > > notification from the WDS? > > > > Yes. Any changes from Sync service and WDS will be processed quicker. We go > > through all of them anyway here > > But there's a chance that the SaveChangesToWebData() call can fail, right? Yes, this is a serious problem and breaks us locally anyway, but the data still be in sync, so that is OK. http://codereview.chromium.org/7819002/diff/18001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/18001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:305: const std::vector<AutofillProfile *>& profiles, On 2011/09/03 00:10:57, Ilya Sherman wrote: > Nit: No space before the "*" Done. http://codereview.chromium.org/7819002/diff/18001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/18001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:7: // ProcessSyncChanges() and for each local change Observe() is called. On 2011/09/03 00:10:57, Ilya Sherman wrote: > nit: This comment should still be right above the class definition though -- > it's not part of the license ;) Done. http://codereview.chromium.org/7819002/diff/18001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:115: void CreateGUIDToProfileMap(const std::vector<AutofillProfile *>& profiles, On 2011/09/03 00:10:57, Ilya Sherman wrote: > nit: No space before the "*" Done.
http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/15004/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:104: profiles_map_[it->first] = it->second; On 2011/09/07 00:28:14, GeorgeY wrote: > On 2011/09/03 00:10:57, Ilya Sherman wrote: > > On 2011/09/03 00:01:29, GeorgeY wrote: > > > On 2011/09/02 21:49:50, Ilya Sherman wrote: > > > > Are you sure you want to update the map here, rather than waiting for a > > change > > > > notification from the WDS? > > > > > > Yes. Any changes from Sync service and WDS will be processed quicker. We go > > > through all of them anyway here > > > > But there's a chance that the SaveChangesToWebData() call can fail, right? > Yes, this is a serious problem and breaks us locally anyway, but the data still > be in sync, so that is OK. Hmm, not sure I follow why the data would be correctly in sync in this case. We should chat about this tomorrow. Otherwise, everything looks good :)
http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:54: if (!const_cast<WebDatabase*>(web_database_)-> On 2011/09/01 23:11:46, dhollowa wrote: > nit: how about simply: > return web_database_->...; > > and remove the if/return false/return true; bits. Ping. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:101: CreateOrUpdateProfile(*sync_iter, &profiles, &remaining_profiles, &bundle); On 2011/09/02 04:34:12, GeorgeY wrote: > On 2011/09/01 23:11:46, dhollowa wrote: > > nit: style guide requires { } for multi-line for(). > AFAIK, {} are required *only* if executing statement more than one line, if the > condition is more than one line brackets are optional. Some developers > (including Peter) are against using curly brackets in this case src/base/... is littered with examples of this. A while ago I asked Brett about it and he cited a clause in the style guide (which I can't find now). Let's go with the "{ }" please. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:372: DCHECK(change->type() == AutofillProfileChange::REMOVE || change->profile()); On 2011/09/02 04:34:12, GeorgeY wrote: > On 2011/09/01 23:11:46, dhollowa wrote: > > This DCHECK is not quite accurate. Better would be: > > > > DCHECK(type == REMOVE && !change->profile() || > > type == ADD && change->profile() || > > type == UPDATE && change->profile()); > > Further down we assert on any change that is not ADD/UPDATE/REMOVE > So my assert is accurate Yes, but your checks wouldn't catch the case where type==REMOVE and change->profile() is non-NULL. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:101: SyncChange CreateChange(SyncChange::SyncChangeType action, On 2011/09/02 04:34:12, GeorgeY wrote: > On 2011/09/01 20:17:49, dhollowa wrote: > > It appears that all of these "Create*" methods don't need object state. So > > they'd be better moved into the .cc file in an anonymous namespace as helper > > methods. > > Done. CreateGUIDToProfileMap and CreateData did not get moved. It makes sense to leave CreateOrUpdateProfile here. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:157: struct AutofillProfileSyncableService::DataBundle { On 2011/09/02 04:34:12, GeorgeY wrote: > On 2011/09/01 20:17:49, dhollowa wrote: > > Move this to the .cc file. > > Done. It appears from other comments (and code) that this is needed in the .h for use in the unit tests. Please add a comment at the top of the class declaration to this effect. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:162: std::vector<AutofillProfile*> updated_profiles; On 2011/09/01 23:11:46, dhollowa wrote: > profiles_to_update? This didn't get changed. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:163: std::vector<AutofillProfile*> new_profiles; // We own these pointers. On 2011/09/01 20:17:49, dhollowa wrote: > If these are owned, how about using a scoped vector? This didn't get changed. (1) s/new_profiles/profiles_to_add/ (2) ScopedVector http://codereview.chromium.org/7819002/diff/24001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/24001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:23: bool MergeField(FormGroup* form_group, nit: output parameters should follow input parameters.
http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:54: if (!const_cast<WebDatabase*>(web_database_)-> On 2011/09/07 01:48:15, dhollowa wrote: > On 2011/09/01 23:11:46, dhollowa wrote: > > nit: how about simply: > > return web_database_->...; > > > > and remove the if/return false/return true; bits. > > Ping. sure http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:101: CreateOrUpdateProfile(*sync_iter, &profiles, &remaining_profiles, &bundle); On 2011/09/07 01:48:15, dhollowa wrote: > On 2011/09/02 04:34:12, GeorgeY wrote: > > On 2011/09/01 23:11:46, dhollowa wrote: > > > nit: style guide requires { } for multi-line for(). > > AFAIK, {} are required *only* if executing statement more than one line, if > the > > condition is more than one line brackets are optional. Some developers > > (including Peter) are against using curly brackets in this case > > src/base/... is littered with examples of this. A while ago I asked Brett about > it and he cited a clause in the style guide (which I can't find now). Let's go > with the "{ }" please. Have you checked the last code - it is already there. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:372: DCHECK(change->type() == AutofillProfileChange::REMOVE || change->profile()); On 2011/09/07 01:48:15, dhollowa wrote: > On 2011/09/02 04:34:12, GeorgeY wrote: > > On 2011/09/01 23:11:46, dhollowa wrote: > > > This DCHECK is not quite accurate. Better would be: > > > > > > DCHECK(type == REMOVE && !change->profile() || > > > type == ADD && change->profile() || > > > type == UPDATE && change->profile()); > > > > Further down we assert on any change that is not ADD/UPDATE/REMOVE > > So my assert is accurate > > Yes, but your checks wouldn't catch the case where type==REMOVE and > change->profile() is non-NULL. But that case is not and error - AFAIK, profile is optional for REMOVE message. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:101: SyncChange CreateChange(SyncChange::SyncChangeType action, On 2011/09/07 01:48:15, dhollowa wrote: > On 2011/09/02 04:34:12, GeorgeY wrote: > > On 2011/09/01 20:17:49, dhollowa wrote: > > > It appears that all of these "Create*" methods don't need object state. So > > > they'd be better moved into the .cc file in an anonymous namespace as helper > > > methods. > > > > Done. > > CreateGUIDToProfileMap and CreateData did not get moved. It makes sense to > leave CreateOrUpdateProfile here. Please verify that you are looking at latest code. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:157: struct AutofillProfileSyncableService::DataBundle { On 2011/09/07 01:48:15, dhollowa wrote: > On 2011/09/02 04:34:12, GeorgeY wrote: > > On 2011/09/01 20:17:49, dhollowa wrote: > > > Move this to the .cc file. > > > > Done. > > It appears from other comments (and code) that this is needed in the .h for use > in the unit tests. Please add a comment at the top of the class declaration to > this effect. Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:162: std::vector<AutofillProfile*> updated_profiles; On 2011/09/07 01:48:15, dhollowa wrote: > On 2011/09/01 23:11:46, dhollowa wrote: > > profiles_to_update? > > This didn't get changed. Done. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.h:163: std::vector<AutofillProfile*> new_profiles; // We own these pointers. On 2011/09/07 01:48:15, dhollowa wrote: > On 2011/09/01 20:17:49, dhollowa wrote: > > If these are owned, how about using a scoped vector? > > This didn't get changed. > (1) s/new_profiles/profiles_to_add/ > (2) ScopedVector 1. Done. 2. It is not owned by bundle anymore.
Getting there... http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:101: CreateOrUpdateProfile(*sync_iter, &profiles, &remaining_profiles, &bundle); On 2011/09/07 20:55:07, GeorgeY wrote: > On 2011/09/07 01:48:15, dhollowa wrote: > > On 2011/09/02 04:34:12, GeorgeY wrote: > > > On 2011/09/01 23:11:46, dhollowa wrote: > > > > nit: style guide requires { } for multi-line for(). > > > AFAIK, {} are required *only* if executing statement more than one line, if > > the > > > condition is more than one line brackets are optional. Some developers > > > (including Peter) are against using curly brackets in this case > > > > src/base/... is littered with examples of this. A while ago I asked Brett > about > > it and he cited a clause in the style guide (which I can't find now). Let's > go > > with the "{ }" please. > > Have you checked the last code - it is already there. It is helpful to the reviewer if you mark "Done". I had to go back to the original review to see what was done and not done, and the diff'ing with Reitveld is less than obvious sometimes. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:372: DCHECK(change->type() == AutofillProfileChange::REMOVE || change->profile()); On 2011/09/07 20:55:07, GeorgeY wrote: > On 2011/09/07 01:48:15, dhollowa wrote: > > On 2011/09/02 04:34:12, GeorgeY wrote: > > > On 2011/09/01 23:11:46, dhollowa wrote: > > > > This DCHECK is not quite accurate. Better would be: > > > > > > > > DCHECK(type == REMOVE && !change->profile() || > > > > type == ADD && change->profile() || > > > > type == UPDATE && change->profile()); > > > > > > Further down we assert on any change that is not ADD/UPDATE/REMOVE > > > So my assert is accurate > > > > Yes, but your checks wouldn't catch the case where type==REMOVE and > > change->profile() is non-NULL. > > But that case is not and error - AFAIK, profile is optional for REMOVE message. That is not correct. See autofill_change.h:51 http://codereview.chromium.org/7819002/diff/24001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/24001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:66: syncable::ModelType type, nit:indent http://codereview.chromium.org/7819002/diff/24001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:154: const tracked_objects::Location& from_here, nit: indent http://codereview.chromium.org/7819002/diff/24001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc (right): http://codereview.chromium.org/7819002/diff/24001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc:87: std::string guid_synced1 = "EDC609ED-7EEE-4F27-B00C-423242A9C44C"; It would be great to verify the four possible cases: (1) updated profile (2) deleted profile (3) new profile (from new sync data) (4) "remaining" profile (from existing local data, new to sync) this is partially covered by |guid_present|. Ideally each case would be verified against the expected data from each. I.e. compare John/Jane/Harry and their respective guids. http://codereview.chromium.org/7819002/diff/24001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc:109: EXPECT_CALL(sync_processor_, Is there a way to verify that this is called with |guid_present| guid and values?
Improved tests significantly. http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:101: CreateOrUpdateProfile(*sync_iter, &profiles, &remaining_profiles, &bundle); On 2011/09/07 21:10:24, dhollowa wrote: > On 2011/09/07 20:55:07, GeorgeY wrote: > > On 2011/09/07 01:48:15, dhollowa wrote: > > > On 2011/09/02 04:34:12, GeorgeY wrote: > > > > On 2011/09/01 23:11:46, dhollowa wrote: > > > > > nit: style guide requires { } for multi-line for(). > > > > AFAIK, {} are required *only* if executing statement more than one line, > if > > > the > > > > condition is more than one line brackets are optional. Some developers > > > > (including Peter) are against using curly brackets in this case > > > > > > src/base/... is littered with examples of this. A while ago I asked Brett > > about > > > it and he cited a clause in the style guide (which I can't find now). Let's > > go > > > with the "{ }" please. > > > > Have you checked the last code - it is already there. > > It is helpful to the reviewer if you mark "Done". I had to go back to the > original review to see what was done and not done, and the diff'ing with > Reitveld is less than obvious sometimes. Done. :) http://codereview.chromium.org/7819002/diff/7019/chrome/browser/sync/glue/aut... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:372: DCHECK(change->type() == AutofillProfileChange::REMOVE || change->profile()); On 2011/09/07 21:10:24, dhollowa wrote: > On 2011/09/07 20:55:07, GeorgeY wrote: > > On 2011/09/07 01:48:15, dhollowa wrote: > > > On 2011/09/02 04:34:12, GeorgeY wrote: > > > > On 2011/09/01 23:11:46, dhollowa wrote: > > > > > This DCHECK is not quite accurate. Better would be: > > > > > > > > > > DCHECK(type == REMOVE && !change->profile() || > > > > > type == ADD && change->profile() || > > > > > type == UPDATE && change->profile()); > > > > > > > > Further down we assert on any change that is not ADD/UPDATE/REMOVE > > > > So my assert is accurate > > > > > > Yes, but your checks wouldn't catch the case where type==REMOVE and > > > change->profile() is non-NULL. > > > > But that case is not and error - AFAIK, profile is optional for REMOVE > message. > > That is not correct. See autofill_change.h:51 Done. http://codereview.chromium.org/7819002/diff/24001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.cc (right): http://codereview.chromium.org/7819002/diff/24001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:66: syncable::ModelType type, On 2011/09/07 21:10:24, dhollowa wrote: > nit:indent Done. http://codereview.chromium.org/7819002/diff/24001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.cc:154: const tracked_objects::Location& from_here, On 2011/09/07 21:10:24, dhollowa wrote: > nit: indent Done. ?!@ autoformat in VS Editor when I move stuff around! :) http://codereview.chromium.org/7819002/diff/24001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc (right): http://codereview.chromium.org/7819002/diff/24001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc:87: std::string guid_synced1 = "EDC609ED-7EEE-4F27-B00C-423242A9C44C"; On 2011/09/07 21:10:24, dhollowa wrote: > It would be great to verify the four possible cases: > (1) updated profile > (2) deleted profile > (3) new profile (from new sync data) > (4) "remaining" profile (from existing local data, new to sync) this is > partially covered by |guid_present|. > > Ideally each case would be verified against the expected data from each. I.e. > compare John/Jane/Harry and their respective guids. 1) Sure, added. 2) It is not received here. It is checked in ProcessSyncChanges and in profile_sync_service_autofill_unittest.cc. 3-4 sure added. http://codereview.chromium.org/7819002/diff/24001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service_unittest.cc:109: EXPECT_CALL(sync_processor_, On 2011/09/07 21:10:24, dhollowa wrote: > Is there a way to verify that this is called with |guid_present| guid and > values? added
LGTM. Thanks. Please wait for all reviewer's approval before landing.
LGTM as well
Just one last comment before my L G T M. I think the memory saving from the comment is worth it. We already cache the sync data in memory and autofill service does the same thing. It is probably not needed once more. Thanks for being patient and I think the implementation has turned out well. Thanks! http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:144: GUIDToProfileMap profiles_map_; I dont think this is needed. This just takes up extra memory.
sorry the comment in the wrong place. changed it. http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:143: ScopedVector<AutofillProfile> profiles_; I dont think this is needed. This just takes up extra memory. http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:144: GUIDToProfileMap profiles_map_; sorry did not mean here. Ignore this. On 2011/09/08 23:52:02, lipalani1 wrote: > I dont think this is needed. This just takes up extra memory.
http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:143: ScopedVector<AutofillProfile> profiles_; On 2011/09/08 23:53:01, lipalani1 wrote: > I dont think this is needed. This just takes up extra memory. It is much cleaner this way, and the memory overhead is small (how many items are going to be deleted during one Chrome session? 5? 10? Probably less, and each is smaller than 1Kb on average). So the overhead would be 5-10 Kb, for the convenience of clear object responsible for lifetime of the Profiles.
LGTM! George has decided to file a M16 bug to address the memory issue. And I agree with that.
http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:4: #ifndef CHROME_BROWSER_SYNC_GLUE_AUTOFILL_PROFILE_SYNCABLE_SERVICE_H_ We should move this (and related syncable service impl code) to the autofill directory, not sync/glue.
http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:4: #ifndef CHROME_BROWSER_SYNC_GLUE_AUTOFILL_PROFILE_SYNCABLE_SERVICE_H_ On 2011/09/12 17:40:54, timsteele wrote: > We should move this (and related syncable service impl code) to the autofill > directory, not sync/glue. Will do this in the separate cl, when changing the rest of the AutofillSyncs.
http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:138: PersonalDataManager* personal_data_; Also, why didn't we make the PDM the syncable service, since it's the actual data service?
Hmm, well I'd prefer if it was done here, but if not, can you make sure there's a bug tracking that? Thanks.
On 2011/09/12 18:30:17, timsteele wrote: > Hmm, well I'd prefer if it was done here, but if not, can you make sure there's > a bug tracking that? Thanks. http://code.google.com/p/chromium/issues/detail?id=96316
http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/au... File chrome/browser/sync/glue/autofill_profile_syncable_service.h (right): http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/au... chrome/browser/sync/glue/autofill_profile_syncable_service.h:138: PersonalDataManager* personal_data_; On 2011/09/12 18:29:23, timsteele wrote: > Also, why didn't we make the PDM the syncable service, since it's the actual > data service? Want to re-use this service for autocomplete as well (which uses almost the same protobufs and so almost the same Specifics on the back-end)
Ok, thanks for clarifying. LGTM On Mon, Sep 12, 2011 at 3:26 PM, <georgey@chromium.org> wrote: > > http://codereview.chromium.**org/7819002/diff/39001/chrome/** > browser/sync/glue/autofill_**profile_syncable_service.h<http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/autofill_profile_syncable_service.h> > File chrome/browser/sync/glue/**autofill_profile_syncable_**service.h > (right): > > http://codereview.chromium.**org/7819002/diff/39001/chrome/** > browser/sync/glue/autofill_**profile_syncable_service.h#**newcode138<http://codereview.chromium.org/7819002/diff/39001/chrome/browser/sync/glue/autofill_profile_syncable_service.h#newcode138> > chrome/browser/sync/glue/**autofill_profile_syncable_**service.h:138: > PersonalDataManager* personal_data_; > On 2011/09/12 18:29:23, timsteele wrote: > >> Also, why didn't we make the PDM the syncable service, since it's the >> > actual > >> data service? >> > > Want to re-use this service for autocomplete as well (which uses almost > the same protobufs and so almost the same Specifics on the back-end) > > > http://codereview.chromium.**org/7819002/<http://codereview.chromium.org/7819... > |
