Migrate AutofillProfile sync to new API.

chrome/browser/sync/glue/autofill_profile_syncable_service.cc

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/sync/glue/autofill_profile_syncable_service.h"
+
+#include "base/tracked.h"
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/sync/api/sync_error.h"
+#include "chrome/browser/sync/glue/do_optimistic_refresh_task.h"
+#include "chrome/browser/webdata/autofill_table.h"
+#include "chrome/browser/webdata/web_database.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/guid.h"
+#include "content/common/notification_service.h"
+
+namespace browser_sync {
+
+const char kAutofillProfileTag[] = "google_chrome_autofill_profiles";
+
+AutofillProfileSyncableService::AutofillProfileSyncableService(
+    WebDatabase* web_database,
+    PersonalDataManager* personal_data)
+    : web_database_(web_database),
+      personal_data_(personal_data),
+      sync_processor_(NULL),
+      number_of_profiles_created_(0) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
+  DCHECK(web_database_);
+  DCHECK(personal_data_);
+  notification_registrar_.Add(this,
+      chrome::NOTIFICATION_AUTOFILL_PROFILE_CHANGED,
+      NotificationService::AllSources());

[Ilya Sherman, 2011/09/01 07:22:31]

Are you sure this should be AllSources() rather than a specific chrome user
profile?  (Maybe ping erg@ if you're not sure.)

[dhollowa, 2011/09/01 23:11:46]

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?

[GeorgeY, 2011/09/02 04:34:12]

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 :)

+}
+
+AutofillProfileSyncableService::~AutofillProfileSyncableService() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));

[dhollowa, 2011/09/01 23:11:46]

It is fine to leave this here... but I believe it is redundant given that |this|
inherits from |base::NonThreadSafe|.

[GeorgeY, 2011/09/02 04:34:12]

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

+}
+
+AutofillProfileSyncableService::AutofillProfileSyncableService()
+    : web_database_(NULL),
+      personal_data_(NULL),
+      sync_processor_(NULL),
+      number_of_profiles_created_(0) {
+  notification_registrar_.Add(this,
+      chrome::NOTIFICATION_AUTOFILL_PROFILE_CHANGED,
+      NotificationService::AllSources());

[Ilya Sherman, 2011/09/01 07:22:31]

Ditto.

[GeorgeY, 2011/09/02 04:34:12]

Ditto.

+}
+
+bool AutofillProfileSyncableService::LoadAutofillData(
+    std::vector<AutofillProfile*>* profiles) const {
+  // const_cast it as GetAutofillProfiles() is a really const accessor, though
+  // not so declared.

[Ilya Sherman, 2011/09/01 07:22:31]

nit: Can we propagate const properly rather than casting away constness here?

[dhollowa, 2011/09/01 23:11:46]

+1

[GeorgeY, 2011/09/02 04:34:12]

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

+  if (!const_cast<WebDatabase*>(web_database_)->

[dhollowa, 2011/09/01 23:11:46]

nit: how about simply:
  return web_database_->...;

and remove the if/return false/return true; bits.

[dhollowa, 2011/09/07 01:48:15]

Ping.

[GeorgeY, 2011/09/07 20:55:07]

sure

+          GetAutofillTable()->GetAutofillProfiles(profiles))
+    return false;
+
+  return true;
+}
+
+SyncError AutofillProfileSyncableService::MergeDataAndStartSyncing(
+      syncable::ModelType type,
+      const SyncDataList& initial_sync_data,
+      SyncChangeProcessor* sync_processor) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(sync_processor_ == NULL);
+  VLOG(1) << "Associating Autofill: MergeDataAndStartSyncing";
+
+  ScopedVector<AutofillProfile> profiles;
+
+  if (!LoadAutofillData(&profiles.get())) {
+    return SyncError(
+        FROM_HERE, "Could not get the autofill data from WebDatabase.",
+        model_type());
+  }
+
+  if (VLOG_IS_ON(2)) {
+    VLOG(2) << "[AUTOFILL MIGRATION]"
+            << "Printing profiles from web db";
+
+    for (ScopedVector<AutofillProfile>::const_iterator ix =
+        profiles.begin(); ix != profiles.end(); ++ix) {
+      AutofillProfile* p = *ix;
+      VLOG(2) << "[AUTOFILL MIGRATION]  "
+              << p->GetInfo(NAME_FIRST)
+              << p->GetInfo(NAME_LAST)
+              << p->guid();
+    }
+  }
+
+  sync_processor_ = sync_processor;
+
+  GuidToProfileMap remaining_profiles;
+  CreateGuidToProfileMap(&profiles, &remaining_profiles);
+
+  DataBundle bundle;
+  // Go through and check for all the profiles that sync already knows about.
+  for (SyncDataList::const_iterator sync_iter = initial_sync_data.begin();
+       sync_iter != initial_sync_data.end();
+       ++sync_iter)
+    CreateOrUpdateProfile(*sync_iter, &profiles, &remaining_profiles, &bundle);

[dhollowa, 2011/09/01 23:11:46]

nit: style guide requires { } for multi-line for().

[GeorgeY, 2011/09/02 04:34:12]

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

[dhollowa, 2011/09/07 01:48:15]

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.

[GeorgeY, 2011/09/07 20:55:07]

Have you checked the last code -  it is already there.

[dhollowa, 2011/09/07 21:10:24]

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.

[GeorgeY, 2011/09/07 23:26:24]

Done. :)

+
+  if (!SaveChangesToWebData(bundle))
+    return SyncError(FROM_HERE, "Failed to update webdata.", model_type());
+
+  SyncChangeList new_changes;
+  for (GuidToProfileMap::iterator i = remaining_profiles.begin();
+       i != remaining_profiles.end(); ++i) {
+    new_changes.push_back(
+        CreateChange(SyncChange::ACTION_ADD, *(*(i->second))));
+  }
+
+  SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes);
+
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+      new DoOptimisticRefreshForAutofill(personal_data_));
+
+  return error;
+}
+
+void AutofillProfileSyncableService::StopSyncing(syncable::ModelType type) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(sync_processor_ != NULL);
+  DCHECK_EQ(type, syncable::AUTOFILL_PROFILE);
+
+  sync_processor_ = NULL;
+}
+
+SyncDataList AutofillProfileSyncableService::GetAllSyncData(
+    syncable::ModelType type) const {
+  DCHECK(CalledOnValidThread());
+  DCHECK(sync_processor_ != NULL);
+  DCHECK_EQ(type, syncable::AUTOFILL_PROFILE);
+
+  SyncDataList current_data;
+
+  ScopedVector<AutofillProfile> profiles;
+
+  if (!LoadAutofillData(&profiles.get()))
+    return current_data;
+
+  for (ScopedVector<AutofillProfile>::iterator i = profiles.begin();
+       i != profiles.end(); ++i) {
+    sync_pb::EntitySpecifics specifics;
+    WriteAutofillProfile(*(*i), &specifics);
+    current_data.push_back(SyncData::CreateLocalData(
+        (*i)->guid(), (*i)->guid(), specifics));
+  }
+  return current_data;
+}
+
+SyncError AutofillProfileSyncableService::ProcessSyncChanges(
+      const tracked_objects::Location& from_here,
+      const SyncChangeList& change_list) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(sync_processor_ != NULL);
+  if (sync_processor_ == NULL) {
+    SyncError error(FROM_HERE, "Models not yet associated.",
+                    syncable::AUTOFILL_PROFILE);
+    return error;
+  }

[lipalani1, 2011/09/01 05:06:02]

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.

[GeorgeY, 2011/09/02 04:34:12]

Done.

+
+  ScopedVector<AutofillProfile> profiles;
+  if (!LoadAutofillData(&profiles.get())) {
+    SyncError error(FROM_HERE, "Failed to read Web DB.",
+                    syncable::AUTOFILL_PROFILE);
+    return error;
+  }
+
+  GuidToProfileMap remaining_profiles;
+  CreateGuidToProfileMap(&profiles, &remaining_profiles);
+
+  std::vector<std::string> profiles_to_delete;
+  DataBundle bundle;
+
+  for (SyncChangeList::const_iterator i = change_list.begin();
+       i != change_list.end(); ++i) {
+    DCHECK(i->IsValid());
+    switch (i->change_type()) {
+      case SyncChange::ACTION_ADD:
+      case SyncChange::ACTION_UPDATE:
+        CreateOrUpdateProfile(i->sync_data(), &profiles, &remaining_profiles,
+                              &bundle);
+        break;
+      case SyncChange::ACTION_DELETE:
+        bundle.profiles_to_delete.push_back(
+          i->sync_data().GetSpecifics().
+              GetExtension(sync_pb::autofill_profile).guid());
+        break;
+      default:
+        NOTREACHED() << "Unexpected sync change state.";
+        return SyncError(FROM_HERE, "ProcessSyncChanges failed on ChangeType " +
+                         SyncChange::ChangeTypeToString(i->change_type()),
+                         syncable::AUTOFILL_PROFILE);
+    }
+  }
+
+  if (!SaveChangesToWebData(bundle))
+    return SyncError(FROM_HERE, "Failed to update webdata.", model_type());
+
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+      new DoOptimisticRefreshForAutofill(personal_data_));
+  return SyncError();
+}
+
+void AutofillProfileSyncableService::Observe(int type,
+    const NotificationSource& source,
+    const NotificationDetails& details) {
+  DCHECK_EQ(type, chrome::NOTIFICATION_AUTOFILL_PROFILE_CHANGED);
+  WebDataService* wds = Source<WebDataService>(source).ptr();
+
+  if (!wds || wds->GetDatabase() != web_database_)
+    return;
+
+  AutofillProfileChange* change = Details<AutofillProfileChange>(details).ptr();
+
+  ActOnChange(change);
+}
+
+// Helper to compare the local value and cloud value of a field, merge into
+// the local value if they differ, and return whether the merge happened.
+bool AutofillProfileSyncableService::MergeField(FormGroup* f,
+    AutofillFieldType t,
+    const std::string& specifics_field) {
+  if (UTF16ToUTF8(f->GetInfo(t)) == specifics_field)
+    return false;
+  f->SetInfo(t, UTF8ToUTF16(specifics_field));
+  return true;
+}
+
+// static
+bool AutofillProfileSyncableService::OverwriteProfileWithServerData(
+    AutofillProfile* merge_into,

[dhollowa, 2011/09/01 20:17:49]

Rename |merge_into| as |profile| and get rid of temporary |p| below.

[GeorgeY, 2011/09/02 04:34:12]

Done.

+    const sync_pb::AutofillProfileSpecifics& specifics) {
+  bool diff = false;
+  AutofillProfile* p = merge_into;
+  const sync_pb::AutofillProfileSpecifics& s(specifics);
+  diff = MergeField(p, NAME_FIRST, s.name_first()) || diff;
+  diff = MergeField(p, NAME_LAST, s.name_last()) || diff;
+  diff = MergeField(p, NAME_MIDDLE, s.name_middle()) || diff;
+  diff = MergeField(p, ADDRESS_HOME_LINE1, s.address_home_line1()) || diff;
+  diff = MergeField(p, ADDRESS_HOME_LINE2, s.address_home_line2()) || diff;
+  diff = MergeField(p, ADDRESS_HOME_CITY, s.address_home_city()) || diff;
+  diff = MergeField(p, ADDRESS_HOME_STATE, s.address_home_state()) || diff;
+  diff = MergeField(p, ADDRESS_HOME_COUNTRY, s.address_home_country()) || diff;
+  diff = MergeField(p, ADDRESS_HOME_ZIP, s.address_home_zip()) || diff;
+  diff = MergeField(p, EMAIL_ADDRESS, s.email_address()) || diff;
+  diff = MergeField(p, COMPANY_NAME, s.company_name()) || diff;
+  diff = MergeField(p, PHONE_FAX_WHOLE_NUMBER, s.phone_fax_whole_number())
+      || diff;
+  diff = MergeField(p, PHONE_HOME_WHOLE_NUMBER, s.phone_home_whole_number())
+      || diff;
+  return diff;
+}
+
+SyncChange AutofillProfileSyncableService::CreateChange(
+    SyncChange::SyncChangeType action,
+    const AutofillProfile& profile) {
+  return SyncChange(action, CreateData(profile));
+}
+
+SyncChange AutofillProfileSyncableService::CreateDeleteChange(
+    const std::string& guid) {
+  AutofillProfile empty_profile(guid);
+  return SyncChange(SyncChange::ACTION_DELETE, CreateData(empty_profile));
+}
+
+SyncData AutofillProfileSyncableService::CreateData(
+    const AutofillProfile& profile) {
+  sync_pb::EntitySpecifics specifics;
+  WriteAutofillProfile(profile, &specifics);
+  return SyncData::CreateLocalData(profile.guid(), profile.guid(), specifics);
+}
+
+// static
+void AutofillProfileSyncableService::WriteAutofillProfile(

[dhollowa, 2011/09/01 20:17:49]

nit: please reorder method to match the .h file.

[GeorgeY, 2011/09/02 04:34:12]

Done.

+    const AutofillProfile& profile,
+    sync_pb::EntitySpecifics* profile_specifics) {
+  sync_pb::AutofillProfileSpecifics* specifics =
+      profile_specifics->MutableExtension(sync_pb::autofill_profile);
+
+  // This would get compiled out in official builds. The caller is expected to
+  // pass in a valid profile object with valid guid.(i.e., the caller might
+  // have to a DCHECK and log before calling. Having to check in 2 places is
+  // not optimal.)
+  DCHECK(guid::IsValidGUID(profile.guid()));
+
+  specifics->set_guid(profile.guid());
+  specifics->set_name_first(UTF16ToUTF8(profile.GetInfo(NAME_FIRST)));
+  specifics->set_name_middle(UTF16ToUTF8(profile.GetInfo(NAME_MIDDLE)));
+  specifics->set_name_last(UTF16ToUTF8(profile.GetInfo(NAME_LAST)));
+  specifics->set_address_home_line1(
+      UTF16ToUTF8(profile.GetInfo(ADDRESS_HOME_LINE1)));
+  specifics->set_address_home_line2(
+      UTF16ToUTF8(profile.GetInfo(ADDRESS_HOME_LINE2)));
+  specifics->set_address_home_city(UTF16ToUTF8(profile.GetInfo(
+      ADDRESS_HOME_CITY)));
+  specifics->set_address_home_state(UTF16ToUTF8(profile.GetInfo(
+      ADDRESS_HOME_STATE)));
+  specifics->set_address_home_country(UTF16ToUTF8(profile.GetInfo(
+      ADDRESS_HOME_COUNTRY)));
+  specifics->set_address_home_zip(UTF16ToUTF8(profile.GetInfo(
+      ADDRESS_HOME_ZIP)));
+  specifics->set_email_address(UTF16ToUTF8(profile.GetInfo(EMAIL_ADDRESS)));
+  specifics->set_company_name(UTF16ToUTF8(profile.GetInfo(COMPANY_NAME)));
+  specifics->set_phone_fax_whole_number(UTF16ToUTF8(profile.GetInfo(
+      PHONE_FAX_WHOLE_NUMBER)));
+  specifics->set_phone_home_whole_number(UTF16ToUTF8(profile.GetInfo(
+      PHONE_HOME_WHOLE_NUMBER)));
+}
+
+void AutofillProfileSyncableService::CreateGuidToProfileMap(
+    ScopedVector<AutofillProfile>* profiles, GuidToProfileMap* profile_map) {

[Ilya Sherman, 2011/09/01 07:22:31]

nit: |profiles| should probably be passed by const reference.

[GeorgeY, 2011/09/02 04:34:12]

Nope. I needed ::iterator, not ::const_iterator.
Changed to index anyway as it guaranteed to be stable if vector grows.

+  DCHECK(profile_map);
+  profile_map->clear();
+  for (ScopedVector<AutofillProfile>::iterator i = profiles->begin();
+       i != profiles->end(); ++i)
+    (*profile_map)[(*i)->guid()] = i;
+}
+
+bool AutofillProfileSyncableService::CreateOrUpdateProfile(

[dhollowa, 2011/09/01 23:11:46]

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?

+    const SyncData& data, ScopedVector<AutofillProfile>* profiles,

[Ilya Sherman, 2011/09/01 07:22:31]

nit: |profiles| does not seem to be used in this function.

[GeorgeY, 2011/09/02 04:34:12]

Was left from older version - removed.

+    GuidToProfileMap* profile_map, DataBundle *bundle) {

[dhollowa, 2011/09/01 20:17:49]

nit: s/ */* /

[GeorgeY, 2011/09/02 04:34:12]

Done.

+  DCHECK(profiles);
+  DCHECK(profile_map);
+  DCHECK(bundle);
+
+  DCHECK_EQ(syncable::AUTOFILL_PROFILE, data.GetDataType());
+
+  const sync_pb::EntitySpecifics& specifics = data.GetSpecifics();
+  const sync_pb::AutofillProfileSpecifics& autofill_specifics(
+      specifics.GetExtension(sync_pb::autofill_profile));
+
+  bool added_new_profile = false;
+
+  GuidToProfileMap::iterator it = profile_map->find(
+        autofill_specifics.guid());
+  if (it != profile_map->end()) {
+    // Some profile that already present is synced.
+    if (OverwriteProfileWithServerData(*(it->second), autofill_specifics))
+      bundle->updated_profiles.push_back(*(it->second));
+    profile_map->erase(autofill_specifics.guid());
+  } else {
+    // New profile synced.
+    AutofillProfile *p(new AutofillProfile(autofill_specifics.guid()));

[dhollowa, 2011/09/01 20:17:49]

nit: s/ */* /

[GeorgeY, 2011/09/02 04:34:12]

Done.

+    OverwriteProfileWithServerData(p, autofill_specifics);
+
+    // Check if profile appears under a different guid.
+    GuidToProfileMap::iterator i;
+    for (i = profile_map->begin(); i != profile_map->end(); ++i) {
+      if ((*(i->second))->Compare(*p) == 0) {
+        bundle->profiles_to_delete.push_back((*(i->second))->guid());
+        VLOG(2) << "[AUTOFILL SYNC]"
+                << "Found in sync db but with a different guid: "
+                << UTF16ToUTF8((*(i->second))->GetInfo(NAME_FIRST))
+                << UTF16ToUTF8((*(i->second))->GetInfo(NAME_LAST))
+                << "New guid " << p->guid() << ". Profile to be deleted "
+                << (*(i->second))->guid();
+        profile_map->erase(i);

[lipalani1, 2011/09/01 05:06:02]

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.

[GeorgeY, 2011/09/02 04:34:12]

It is erased from web_db with a call to DataBundle up the stack.

+        break;
+      }
+    }
+    // Bundle takes ownership of new profiles only.
+    bundle->new_profiles.push_back(p);
+    added_new_profile = true;

[Ilya Sherman, 2011/09/01 07:22:31]

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.

[GeorgeY, 2011/09/02 04:34:12]

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.

+  }
+  return added_new_profile;
+}
+
+void AutofillProfileSyncableService::ActOnChange(
+     AutofillProfileChange* change) {
+  DCHECK(change->type() == AutofillProfileChange::REMOVE || change->profile());

[dhollowa, 2011/09/01 23:11:46]

This DCHECK is not quite accurate.  Better would be:

DCHECK(type == REMOVE && !change->profile() ||
              type == ADD && change->profile() ||
              type == UPDATE && change->profile());

[GeorgeY, 2011/09/02 04:34:12]

Further down we assert on any change that is not ADD/UPDATE/REMOVE
So my assert is accurate

[dhollowa, 2011/09/07 01:48:15]

Yes, but your checks wouldn't catch the case where type==REMOVE and
change->profile() is non-NULL.

[GeorgeY, 2011/09/07 20:55:07]

But that case is not and error - AFAIK, profile is optional for REMOVE message.

[dhollowa, 2011/09/07 21:10:24]

That is not correct.  See autofill_change.h:51

[GeorgeY, 2011/09/07 23:26:24]

Done.

+  DCHECK(sync_processor_);
+  SyncChangeList new_changes;
+  switch (change->type()) {
+    case AutofillProfileChange::ADD:
+      new_changes.push_back(
+          CreateChange(SyncChange::ACTION_ADD, *(change->profile())));

[dhollowa, 2011/09/01 23:11:46]

These CreateChange and CreateDeleteChange helpers seem unnecessary.  They don't
really reduce the code size over simply calling the SyncChange(...) constructor
directly.

[GeorgeY, 2011/09/02 04:34:12]

sure

+      break;
+    case AutofillProfileChange::UPDATE:
+      new_changes.push_back(
+          CreateChange(SyncChange::ACTION_UPDATE, *(change->profile())));
+      break;
+    case AutofillProfileChange::REMOVE:
+      new_changes.push_back(CreateDeleteChange(change->key()));
+      break;
+    default:
+      NOTREACHED();
+  }
+  SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes);
+  if (error.IsSet()) {

[lipalani1, 2011/09/01 05:06:02]

may be this is a vlog(warning)?

[GeorgeY, 2011/09/02 04:34:12]

Sure.

+    VLOG(2) << "[AUTOFILL SYNC]"
+            << " Failed processing change:"
+            << " Error:" << error.message()
+            << " Guid:" << change->profile()->guid();
+  }
+}
+
+bool AutofillProfileSyncableService::SaveChangesToWebData(
+    const DataBundle& bundle) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
+
+  for (size_t i = 0; i < bundle.new_profiles.size(); i++) {
+    if (!web_database_->GetAutofillTable()->AddAutofillProfile(
+        *bundle.new_profiles[i]))
+      return false;
+  }
+
+  for (size_t i = 0; i < bundle.updated_profiles.size(); i++) {
+    if (!web_database_->GetAutofillTable()->UpdateAutofillProfile(
+        *bundle.updated_profiles[i]))
+      return false;
+  }
+
+  for (size_t i = 0; i< bundle.profiles_to_delete.size(); ++i) {
+    if (!web_database_->GetAutofillTable()->RemoveAutofillProfile(
+        bundle.profiles_to_delete[i]))
+      return false;
+  }
+  return true;
+}
+
+AutofillProfileSyncableService::DataBundle::DataBundle() {}
+
+AutofillProfileSyncableService::DataBundle::~DataBundle() {
+  STLDeleteElements(&new_profiles);
+}
+
+}  // namespace browser_sync
+