Logs all reasons card upload was not offered in UKM and UMA.

components/autofill/core/browser/autofill_manager.cc

 // Copyright 2013 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 "components/autofill/core/browser/autofill_manager.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 24 matching lines @@
 #include "build/build_config.h"
 #include "components/autofill/core/browser/autocomplete_history_manager.h"
 #include "components/autofill/core/browser/autofill_client.h"
 #include "components/autofill/core/browser/autofill_data_model.h"
 #include "components/autofill/core/browser/autofill_experiments.h"
 #include "components/autofill/core/browser/autofill_external_delegate.h"
 #include "components/autofill/core/browser/autofill_field.h"
 #include "components/autofill/core/browser/autofill_manager_test_delegate.h"
 #include "components/autofill/core/browser/autofill_metrics.h"
 #include "components/autofill/core/browser/autofill_profile.h"
+#include "components/autofill/core/browser/autofill_profile_comparator.h"
 #include "components/autofill/core/browser/autofill_type.h"
 #include "components/autofill/core/browser/country_names.h"
 #include "components/autofill/core/browser/credit_card.h"
 #include "components/autofill/core/browser/field_types.h"
 #include "components/autofill/core/browser/form_structure.h"
 #include "components/autofill/core/browser/personal_data_manager.h"
 #include "components/autofill/core/browser/phone_number.h"
 #include "components/autofill/core/browser/phone_number_i18n.h"
 #include "components/autofill/core/browser/popup_item_ids.h"
 #include "components/autofill/core/browser/ui/save_card_bubble_controller.h"
@@ skipping 979 matching lines @@
 void AutofillManager::OnDidGetRealPan(AutofillClient::PaymentsRpcResult result,
                                       const std::string& real_pan) {
   DCHECK(full_card_request_);
   full_card_request_->OnDidGetRealPan(result, real_pan);
 }
 
 void AutofillManager::OnDidGetUploadDetails(
     AutofillClient::PaymentsRpcResult result,
     const base::string16& context_token,
     std::unique_ptr<base::DictionaryValue> legal_message) {
-  // TODO(jdonnelly): Log duration.
+  int card_upload_decision_metrics;
   if (result == AutofillClient::SUCCESS) {
     // Do *not* call payments_client_->Prepare() here. We shouldn't send
     // credentials until the user has explicitly accepted a prompt to upload.
     upload_request_.context_token = context_token;
     user_did_accept_upload_prompt_ = false;
     client_->ConfirmSaveCreditCardToCloud(
         upload_request_.card, std::move(legal_message),
         should_cvc_be_requested_,
         base::Bind(&AutofillManager::OnUserDidAcceptUpload,
                    weak_ptr_factory_.GetWeakPtr()));
     client_->LoadRiskData(base::Bind(&AutofillManager::OnDidGetUploadRiskData,
                                      weak_ptr_factory_.GetWeakPtr()));
-    AutofillMetrics::CardUploadDecisionMetric card_upload_decision_metric =
-        should_cvc_be_requested_ ? AutofillMetrics::UPLOAD_OFFERED_NO_CVC
-                                 : AutofillMetrics::UPLOAD_OFFERED;
-    AutofillMetrics::LogCardUploadDecisionMetric(card_upload_decision_metric);
-    LogCardUploadDecisionUkm(card_upload_decision_metric);
+    AutofillMetrics::LogCardUploadDecisionMetric(
+        AutofillMetrics::UPLOAD_OFFERED);
+    card_upload_decision_metrics = 1 << AutofillMetrics::UPLOAD_OFFERED;
+    if (should_cvc_be_requested_) {
+      AutofillMetrics::LogCardUploadDecisionMetric(
+          AutofillMetrics::UPLOAD_OFFERED_NO_CVC);
+      card_upload_decision_metrics |= 1

[Steven Holte, 2017/05/02 00:36:33]

Are you always calling AutofillMetrics::LogCardUploadDecisionMetric for each of
the individual bits you are setting in card_upload_decision_metrics?

If so, maybe just loop over the bits and report them at the same point you would
call LogCardUploadDecisionsUkm?

[csashi, 2017/05/02 00:43:55]

Done. Sebastien made same suggestion.

+                                      << AutofillMetrics::UPLOAD_OFFERED_NO_CVC;
+    }
   } else {
     // If the upload details request failed, fall back to a local save. The
     // reasoning here is as follows:
     // - This will sometimes fail intermittently, in which case it might be
     // better to not fall back, because sometimes offering upload and sometimes
     // offering local save is a poor user experience.
     // - However, in some cases, our local configuration limiting the feature to
     // countries that Payments is known to support will not match Payments' own
     // determination of what country the user is located in. In these cases,
     // the upload details request will consistently fail and if we don't fall
     // back to a local save then the user will never be offered any kind of
     // credit card save.
     client_->ConfirmSaveCreditCardLocally(
         upload_request_.card,
         base::Bind(
             base::IgnoreResult(&PersonalDataManager::SaveImportedCreditCard),
             base::Unretained(personal_data_), upload_request_.card));
     AutofillMetrics::LogCardUploadDecisionMetric(
+        AutofillMetrics::UPLOAD_NOT_OFFERED);
+    AutofillMetrics::LogCardUploadDecisionMetric(
         AutofillMetrics::UPLOAD_NOT_OFFERED_GET_UPLOAD_DETAILS_FAILED);

[Jared Saul, 2017/05/01 21:05:12]

It feels kind of flaky that if anything fails (or succeeds), the caller needs to
recognize and remember that UPLOAD_NOT_OFFERED or UPLOAD_OFFERED also needs to
be logged.  I was going to suggest a helper method for fool-proof-ness, but I
guess that would log UPLOAD_NOT_OFFERED or UPLOAD_OFFERED multiple times, which
is worse.  I guess my comment here is just an optional "I wish it could be safer
to be extended by future developers, but I don't have any ideas offhand."

[csashi, 2017/05/02 00:43:55]

Acknowledged. The best I could think of was to set the UPLOAD_OFFERED and
UPLOAD_NOT_OFFERED bits in the consolidated LogCardUploadDecisions function -
but I would need a switch to look at every other enum and set either of these 2
bits - not foolproof either.

-    LogCardUploadDecisionUkm(
-        AutofillMetrics::UPLOAD_NOT_OFFERED_GET_UPLOAD_DETAILS_FAILED);
+    card_upload_decision_metrics =
+        (1 << AutofillMetrics::UPLOAD_NOT_OFFERED_GET_UPLOAD_DETAILS_FAILED) |
+        (1 << AutofillMetrics::UPLOAD_NOT_OFFERED);
   }
+  LogCardUploadDecisionsUkm(card_upload_decision_metrics);
   pending_upload_request_url_ = GURL();
 }
 
 void AutofillManager::OnDidUploadCard(AutofillClient::PaymentsRpcResult result,
                                       const std::string& server_id) {
   // We don't do anything user-visible if the upload attempt fails.
   // If the upload succeeds, we will keep a copy of the card as a full server
   // card on the device.
   if (result == AutofillClient::SUCCESS && !server_id.empty()) {
     upload_request_.card.set_record_type(CreditCard::FULL_SERVER_CARD);
@@ skipping 156 matching lines @@
       if (field->Type().GetStorableType() == CREDIT_CARD_VERIFICATION_CODE &&
           IsValidCreditCardSecurityCode(field->value,
                                         upload_request_.card.type())) {
         upload_request_.cvc = field->value;
         break;
       }
     }
 
     // Upload requires that recently used or modified addresses meet the
     // client-side validation rules.
-    autofill::AutofillMetrics::CardUploadDecisionMetric
-        get_profiles_decision_metric = AutofillMetrics::UPLOAD_OFFERED;
     std::string rappor_metric_name;
-    bool get_profiles_succeeded =
-        GetProfilesForCreditCardUpload(*imported_credit_card,
-                                       &upload_request_.profiles,
-                                       &get_profiles_decision_metric,
-                                       &rappor_metric_name);
+    int upload_decision_metrics = GetProfilesForCreditCardUpload(
+        *imported_credit_card, &upload_request_.profiles, &rappor_metric_name);
 
     pending_upload_request_url_ = GURL(submitted_form.source_url());
 
     // Both the CVC and address checks are done.  Conform to the legacy order of
     // reporting on CVC then address.
     should_cvc_be_requested_ = false;
     if (upload_request_.cvc.empty()) {
-      if (IsAutofillUpstreamRequestCvcIfMissingExperimentEnabled()) {
-        should_cvc_be_requested_ = true;
-      } else {
+      should_cvc_be_requested_ =
+          (!upload_decision_metrics &&
+           IsAutofillUpstreamRequestCvcIfMissingExperimentEnabled());
+      if (!should_cvc_be_requested_) {
         AutofillMetrics::LogCardUploadDecisionMetric(
             AutofillMetrics::UPLOAD_NOT_OFFERED_NO_CVC);
-        LogCardUploadDecisionUkm(AutofillMetrics::UPLOAD_NOT_OFFERED_NO_CVC);
-        pending_upload_request_url_ = GURL();
-        CollectRapportSample(submitted_form.source_url(),
-                             "Autofill.CardUploadNotOfferedNoCvc");
-        return;
+        upload_decision_metrics |=
+            (1 << AutofillMetrics::UPLOAD_NOT_OFFERED_NO_CVC);
+        rappor_metric_name = "Autofill.CardUploadNotOfferedNoCvc";
       }
     }
-    if (!get_profiles_succeeded) {
-      DCHECK(get_profiles_decision_metric != AutofillMetrics::UPLOAD_OFFERED);
+    if (upload_decision_metrics) {
       AutofillMetrics::LogCardUploadDecisionMetric(
-          get_profiles_decision_metric);
-      LogCardUploadDecisionUkm(get_profiles_decision_metric);
+          AutofillMetrics::UPLOAD_NOT_OFFERED);
+      upload_decision_metrics |= 1 << AutofillMetrics::UPLOAD_NOT_OFFERED;
+      LogCardUploadDecisionsUkm(upload_decision_metrics);
       pending_upload_request_url_ = GURL();
       if (!rappor_metric_name.empty()) {
-        CollectRapportSample(submitted_form.source_url(), rappor_metric_name);
+        CollectRapporSample(submitted_form.source_url(), rappor_metric_name);
       }
       return;
     }
 
     // All required data is available, start the upload process.
     payments_client_->GetUploadDetails(upload_request_.profiles, app_locale_);
   }
 }
 
-bool AutofillManager::GetProfilesForCreditCardUpload(
+int AutofillManager::GetProfilesForCreditCardUpload(
     const CreditCard& card,
     std::vector<AutofillProfile>* profiles,
-    autofill::AutofillMetrics::CardUploadDecisionMetric*
-        address_upload_decision_metric,
     std::string* rappor_metric_name) const {
   std::vector<AutofillProfile> candidate_profiles;
   const base::Time now = AutofillClock::Now();
   const base::TimeDelta fifteen_minutes = base::TimeDelta::FromMinutes(15);
+  int upload_decision_metrics = 0;
 
   // First, collect all of the addresses used recently.
   for (AutofillProfile* profile : personal_data_->GetProfiles()) {
     if ((now - profile->use_date()) < fifteen_minutes ||
         (now - profile->modification_date()) < fifteen_minutes) {
       candidate_profiles.push_back(*profile);
     }
   }
   if (candidate_profiles.empty()) {
-    *address_upload_decision_metric =
-        AutofillMetrics::UPLOAD_NOT_OFFERED_NO_ADDRESS;
+    AutofillMetrics::LogCardUploadDecisionMetric(
+        AutofillMetrics::UPLOAD_NOT_OFFERED_NO_ADDRESS);
+    upload_decision_metrics |=
+        (1 << AutofillMetrics::UPLOAD_NOT_OFFERED_NO_ADDRESS);
     *rappor_metric_name = "Autofill.CardUploadNotOfferedNoAddress";
-    return false;
   }
 
   // If any of the names on the card or the addresses don't match (where
   // matching is case insensitive and ignores middle initials if present), the
   // candidate set is invalid. This matches the rules for name matching applied
   // server-side by Google Payments and ensures that we don't send upload
   // requests that are guaranteed to fail.
   base::string16 verified_name;
-  base::string16 card_name =
+  const base::string16 card_name =
       card.GetInfo(AutofillType(CREDIT_CARD_NAME_FULL), app_locale_);
   if (!card_name.empty()) {
     verified_name = RemoveMiddleInitial(card_name);
   }
   for (const AutofillProfile& profile : candidate_profiles) {
-    base::string16 address_name =
+    const base::string16 address_name =
         profile.GetInfo(AutofillType(NAME_FULL), app_locale_);
     if (!address_name.empty()) {
       if (verified_name.empty()) {
         verified_name = RemoveMiddleInitial(address_name);
       } else {
-        // TODO(crbug.com/590307): We only use ASCII case insensitivity here
-        // because the feature is launching US-only and middle initials only
-        // make sense for Western-style names anyway. As we launch in more
-        // countries, we'll need to make the name comparison more sophisticated.
+        // TODO(crbug.com/590307): We'll need to make the name comparison more
+        // sophisticated.
         if (!base::EqualsCaseInsensitiveASCII(
                 verified_name, RemoveMiddleInitial(address_name))) {
-          *address_upload_decision_metric =
-              AutofillMetrics::UPLOAD_NOT_OFFERED_CONFLICTING_NAMES;
-          *rappor_metric_name = "Autofill.CardUploadNotOfferedConflictingNames";
-          return false;
+          if (!upload_decision_metrics)
+            *rappor_metric_name =
+                "Autofill.CardUploadNotOfferedConflictingNames";
+          AutofillMetrics::LogCardUploadDecisionMetric(
+              AutofillMetrics::UPLOAD_NOT_OFFERED_CONFLICTING_NAMES);
+          upload_decision_metrics |=
+              (1 << AutofillMetrics::UPLOAD_NOT_OFFERED_CONFLICTING_NAMES);
+          break;
         }
       }
     }
   }
   // If neither the card nor any of the addresses have a name associated with
   // them, the candidate set is invalid.
   if (verified_name.empty()) {
-    *address_upload_decision_metric =
-        AutofillMetrics::UPLOAD_NOT_OFFERED_NO_NAME;
-    *rappor_metric_name = "Autofill.CardUploadNotOfferedNoName";
-    return false;
+    if (!upload_decision_metrics)
+      *rappor_metric_name = "Autofill.CardUploadNotOfferedNoName";
+    AutofillMetrics::LogCardUploadDecisionMetric(
+        AutofillMetrics::UPLOAD_NOT_OFFERED_NO_NAME);
+    upload_decision_metrics |=
+        (1 << AutofillMetrics::UPLOAD_NOT_OFFERED_NO_NAME);
   }
 
   // If any of the candidate addresses have a non-empty zip that doesn't match
   // any other non-empty zip, then the candidate set is invalid.
   base::string16 verified_zip;
   for (const AutofillProfile& profile : candidate_profiles) {
     // TODO(jdonnelly): Use GetInfo instead of GetRawInfo once zip codes are
     // canonicalized. See http://crbug.com/587465.
-    base::string16 zip = profile.GetRawInfo(ADDRESS_HOME_ZIP);
+    const base::string16 zip = profile.GetRawInfo(ADDRESS_HOME_ZIP);
     if (!zip.empty()) {
       if (verified_zip.empty()) {
         verified_zip = zip;
       } else {
         // To compare two zips, we check to see if either is a prefix of the
         // other. This allows us to consider a 5-digit zip and a zip+4 to be a
         // match if the first 5 digits are the same without hardcoding any
         // specifics of how postal codes are represented. (They can be numeric
         // or alphanumeric and vary from 3 to 10 digits long by country. See
         // https://en.wikipedia.org/wiki/Postal_code#Presentation.) The Payments
         // backend will apply a more sophisticated address-matching procedure.
         // This check is simply meant to avoid offering upload in cases that are
         // likely to fail.
         if (!(StartsWith(verified_zip, zip, base::CompareCase::SENSITIVE) ||
               StartsWith(zip, verified_zip, base::CompareCase::SENSITIVE))) {
-          *address_upload_decision_metric =
-              AutofillMetrics::UPLOAD_NOT_OFFERED_CONFLICTING_ZIPS;
-          return false;
+          AutofillMetrics::LogCardUploadDecisionMetric(
+              AutofillMetrics::UPLOAD_NOT_OFFERED_CONFLICTING_ZIPS);
+          upload_decision_metrics |=
+              (1 << AutofillMetrics::UPLOAD_NOT_OFFERED_CONFLICTING_ZIPS);
+          break;
         }
       }
     }
   }
 
   // If none of the candidate addresses have a zip, the candidate set is
   // invalid.
   if (verified_zip.empty()) {
-    *address_upload_decision_metric =
-        AutofillMetrics::UPLOAD_NOT_OFFERED_NO_ZIP_CODE;
-    return false;
+    AutofillMetrics::LogCardUploadDecisionMetric(
+        AutofillMetrics::UPLOAD_NOT_OFFERED_NO_ZIP_CODE);
+    upload_decision_metrics |=
+        (1 << AutofillMetrics::UPLOAD_NOT_OFFERED_NO_ZIP_CODE);
   }
 
-  profiles->assign(candidate_profiles.begin(), candidate_profiles.end());
-  return true;
+  if (!upload_decision_metrics)
+    profiles->assign(candidate_profiles.begin(), candidate_profiles.end());
+
+  return upload_decision_metrics;
 }
 
-void AutofillManager::CollectRapportSample(const GURL& source_url,
-                                           const std::string& metric_name)
-    const {
+void AutofillManager::CollectRapporSample(
+    const GURL& source_url,
+    const std::string& metric_name) const {
   if (source_url.is_valid() && client_->GetRapporServiceImpl()) {
     rappor::SampleDomainAndRegistryFromGURL(client_->GetRapporServiceImpl(),
                                             metric_name, source_url);
   }
 }
 
 // Note that |submitted_form| is passed as a pointer rather than as a reference
 // so that we can get memory management right across threads.  Note also that we
 // explicitly pass in all the time stamps of interest, as the cached ones might
 // get reset before this method executes.
@@ skipping 831 matching lines @@
     }
     if (i > 0)
       fputs("Next oldest form:\n", file);
   }
   fputs("\n", file);
 
   fclose(file);
 }
 #endif  // ENABLE_FORM_DEBUG_DUMP
 
-void AutofillManager::LogCardUploadDecisionUkm(
-    AutofillMetrics::CardUploadDecisionMetric upload_decision) {
-  AutofillMetrics::LogCardUploadDecisionUkm(
-      client_->GetUkmService(), pending_upload_request_url_, upload_decision);
+void AutofillManager::LogCardUploadDecisionsUkm(int upload_decision_metrics) {
+  AutofillMetrics::LogCardUploadDecisionsUkm(client_->GetUkmService(),
+                                             pending_upload_request_url_,
+                                             upload_decision_metrics);
 }
 
 }  // namespace autofill