[AF Profile] Fix reporting a debug metric

This CL removes a check that an entity that gets deleted exists. It
turns out it gets deleted before notifying so this check always fail,
producing useless results. As a result we compute
is_converted_from_server for DELETE changes differently, based on the
storage key.

The metric does not get renamed as it only lives on Canary / Dev so
far and is not used in production.

Bug: 904390
Change-Id: Iac3e5c42377d480671efce57ec225fea1cc8f625
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1496859
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Auto-Submit: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#637642}

components/autofill/core/browser/webdata/autofill_profile_sync_bridge.cc

@@ -49,20 +49,6 @@ namespace {
 // Address to this variable used as the user data key.
 static int kAutofillProfileSyncBridgeUserDataKey = 0;
 
-// TODO(crbug.com/904390): Remove when the investigation is over.
-base::Optional<AutofillProfile> FindLocalProfileByStorageKey(
-    const std::string& storage_key,
-    AutofillTable* table) {
-  std::vector<std::unique_ptr<AutofillProfile>> local_profiles;
-  table->GetAutofillProfiles(&local_profiles);
-  for (const auto& local_profile : local_profiles) {
-    if (storage_key == GetStorageKeyFromAutofillProfile(*local_profile)) {
-      return *local_profile;
-    }
-  }
-  return base::nullopt;
-}
-
 }  // namespace
 
 // static
@@ -232,19 +218,18 @@ void AutofillProfileSyncBridge::ActOnLocalChange(
           GetAutofillTable(), syncer::AUTOFILL_PROFILE);
 
   // TODO(crbug.com/904390): Remove when the investigation is over.
-  base::Optional<AutofillProfile> local_profile;
+  bool is_converted_from_server = false;
   if (change.type() == AutofillProfileChange::REMOVE) {
-    local_profile =
-        FindLocalProfileByStorageKey(change.key(), GetAutofillTable());
+    // The profile is not available any more so we cannot compare its value,
+    // instead we use a rougher test based on the id - whether it is a local
+    // GUID or a server id. As a result, it has a different semantics compared
+    // to AddOrUpdate.
+    is_converted_from_server = !base::IsValidGUID(change.key());
   } else {
-    local_profile = *change.data_model();
-  }
     std::vector<std::unique_ptr<AutofillProfile>> server_profiles;
     GetAutofillTable()->GetServerProfiles(&server_profiles);
-  bool is_converted_from_server = false;
-  if (local_profile != base::nullopt) {
     is_converted_from_server = IsLocalProfileEqualToServerProfile(
-        server_profiles, *local_profile, app_locale_);
+        server_profiles, *change.data_model(), app_locale_);
   }
 
   switch (change.type()) {
@@ -271,13 +256,10 @@ void AutofillProfileSyncBridge::ActOnLocalChange(
       change_processor()->Delete(change.key(), metadata_change_list.get());
 
       // TODO(crbug.com/904390): Remove when the investigation is over.
-      if (local_profile != base::nullopt) {
-        // Report only if we delete an existing entity.
       ReportAutofillProfileDeleteOrigin(
           is_converted_from_server
               ? AutofillProfileSyncChangeOrigin::kConvertedLocal
               : AutofillProfileSyncChangeOrigin::kTrulyLocal);
-      }
       break;
     case AutofillProfileChange::EXPIRE:
       // EXPIRE changes are not being issued for profiles.

components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc

@@ -614,21 +614,21 @@ void AutofillProfileSyncableService::ActOnChange(
   }
 
   // TODO(crbug.com/904390): Remove when the investigation is over.
-  const AutofillProfile* local_profile = nullptr;
+  bool is_converted_from_server = false;
   if (change.type() == AutofillProfileChange::REMOVE) {
-    if (profiles_map_.find(change.key()) != profiles_map_.end()) {
-      local_profile = profiles_map_[change.key()];
-    }
+    // The profile is not available any more so we cannot compare its value,
+    // instead we use a rougher test based on the id - whether it is a local
+    // GUID or a server id. As a result, it has a different semantics compared
+    // to AddOrUpdate.
+    is_converted_from_server = !base::IsValidGUID(change.key());
   } else {
-    local_profile = change.data_model();
-  }
-  bool is_converted_from_server = false;
-  // |webdata_backend_| may be null in unit-tests.
-  if (local_profile != nullptr && webdata_backend_ != nullptr) {
+    // |webdata_backend_|, used by GetAutofillTable() may be null in unit-tests.
+    if (webdata_backend_ != nullptr) {
       std::vector<std::unique_ptr<AutofillProfile>> server_profiles;
       GetAutofillTable()->GetServerProfiles(&server_profiles);
       is_converted_from_server = IsLocalProfileEqualToServerProfile(
-        server_profiles, *local_profile, app_locale_);
+          server_profiles, *change.data_model(), app_locale_);
+    }
   }
 
   syncer::SyncChangeList new_changes;

components/browser_sync/profile_sync_service_autofill_unittest.cc

@@ -998,10 +998,6 @@ TEST_F(ProfileSyncServiceAutofillTest, ProcessUserChangeRemoveProfile) {
   StartAutofillProfileSyncService(add_autofill.callback());
   ASSERT_TRUE(add_autofill.success());
 
-  // TODO(crbug.com/904390): Remove when the investigation is over. This call is
-  // needed in the AutofillProfileChanged() callback.
-  EXPECT_CALL(autofill_table(), GetServerProfiles(_)).WillOnce(Return(true));
-
   AutofillProfileChange change(AutofillProfileChange::REMOVE,
                                sync_profile.guid(), nullptr);
   web_data_service()->OnAutofillProfileChanged(change);