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 993 matching lines @@
         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);
+    LogCardUploadDecisionsUkm({card_upload_decision_metric});
   } 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_GET_UPLOAD_DETAILS_FAILED);
-    LogCardUploadDecisionUkm(
-        AutofillMetrics::UPLOAD_NOT_OFFERED_GET_UPLOAD_DETAILS_FAILED);
+    LogCardUploadDecisionsUkm(
+        {AutofillMetrics::UPLOAD_NOT_OFFERED_GET_UPLOAD_DETAILS_FAILED});
   }
   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()) {
@@ skipping 157 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::vector<AutofillMetrics::CardUploadDecisionMetric>
+        upload_decision_metrics;
     std::string rappor_metric_name;
-    bool get_profiles_succeeded =
-        GetProfilesForCreditCardUpload(*imported_credit_card,
-                                       &upload_request_.profiles,
-                                       &get_profiles_decision_metric,
-                                       &rappor_metric_name);
+    bool get_profiles_succeeded = GetProfilesForCreditCardUpload(

[Jared Saul, 2017/04/28 19:21:42]

Is this new formatting the result of a tool?  It's weird that it purposefully
breaks the rectangle rule.

[csashi, 2017/04/28 23:47:45]

I am using git cl format, which is the tool recommended by git cl upload.

+        *imported_credit_card, &upload_request_.profiles,
+        &upload_decision_metrics, &rappor_metric_name);
+    DCHECK_EQ(get_profiles_succeeded, upload_decision_metrics.empty());
 
     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()) {
+      if (get_profiles_succeeded &&

[sebsg, 2017/04/28 15:26:24]

Do you think we could log something like "Not able to get profiles" if
get_profile_succeeded is false? It feels weird to me to record no CVC if we
could not get the profiles.

[Jared Saul, 2017/04/28 19:21:42]

I think this is actually correct.  In this branch, the CVC is missing.  If there
are OTHER errors (aka get_profile_succeeded is false) then we want to
additionally log NO_CVC.  If there are NOT other errors, we can still offer to
save if the experiment is enabled.

Additionally, "not able to get profiles" is not necessary because the specific
reasons we couldn't get profiles is already populated.

[csashi, 2017/04/28 23:47:44]

Acknowledged. I modified the conditions to perhaps make it easier to parse.

[csashi, 2017/04/28 23:47:45]

Done.

[sebsg, 2017/05/01 02:46:20]

Thank you!

[csashi, 2017/05/01 16:01:56]

Acknowledged.

+          IsAutofillUpstreamRequestCvcIfMissingExperimentEnabled()) {
         should_cvc_be_requested_ = true;
       } else {
-        AutofillMetrics::LogCardUploadDecisionMetric(
+        upload_decision_metrics.push_back(
             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;
+        rappor_metric_name = "Autofill.CardUploadNotOfferedNoCvc";
       }
     }
-    if (!get_profiles_succeeded) {
-      DCHECK(get_profiles_decision_metric != AutofillMetrics::UPLOAD_OFFERED);
-      AutofillMetrics::LogCardUploadDecisionMetric(
-          get_profiles_decision_metric);
-      LogCardUploadDecisionUkm(get_profiles_decision_metric);
+    for (const auto it : upload_decision_metrics)
+      AutofillMetrics::LogCardUploadDecisionMetric(it);

[sebsg, 2017/04/28 15:26:24]

Do you think you could pass the vector here too like you did for the UKM.

EDIT: Actually I think this would also benefit from the bitfield I describe in
another comment :P

[Jared Saul, 2017/04/28 19:21:42]

I'm definitely against doing it this way.  Different users will log different
numbers of failures.  It might be nice from a standpoint of "NO_CVC was hit X
times", but it will completely wreck the "X% of users hit NO_CVC".  I think the
bitfield suggestion would fix this?

[csashi, 2017/04/28 23:47:44]

Thanks for the catch. Bit field will fix this for UKM. In theory, we could log
the bit field for histogram enumeration also. For e.g. Failures X and Y will be
1 bucket, Failure X will be another bucket, Failure Y will be another bucket and
so on. But that seems like an abuse of the histogram enumeration (not to mention
# buckets increase as 2**N).I created a new -1 Enumerator for NOT_OFFERED so we
don't conflict with existing constants.

[csashi, 2017/04/28 23:47:45]

Done.

[sebsg, 2017/05/01 02:46:20]

Yeah that's how I did it for Payment Request, one bit per event, so there will
be different buckets for X and Y VS just X like you said. However I found it to
be pretty easy to split them myself when I want to query the data in the
analysis tools.  

So I think it would be a good idea for uma data too :)

[csashi, 2017/05/01 16:01:56]

Acknowledged.

+    if (!upload_decision_metrics.empty()) {
+      LogCardUploadDecisionsUkm(upload_decision_metrics);
       pending_upload_request_url_ = GURL();
-      if (!rappor_metric_name.empty()) {
-        CollectRapportSample(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(
     const CreditCard& card,
     std::vector<AutofillProfile>* profiles,
-    autofill::AutofillMetrics::CardUploadDecisionMetric*
-        address_upload_decision_metric,
+    std::vector<AutofillMetrics::CardUploadDecisionMetric>*
+        upload_decision_metrics,
     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);
 
   // 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;
+    upload_decision_metrics->push_back(
+        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->empty())
+            *rappor_metric_name =
+                "Autofill.CardUploadNotOfferedConflictingNames";
+          upload_decision_metrics->push_back(
+              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->empty())
+      *rappor_metric_name = "Autofill.CardUploadNotOfferedNoName";
+    upload_decision_metrics->push_back(
+        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;
+          upload_decision_metrics->push_back(
+              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;
+    upload_decision_metrics->push_back(
+        AutofillMetrics::UPLOAD_NOT_OFFERED_NO_ZIP_CODE);
   }
 
-  profiles->assign(candidate_profiles.begin(), candidate_profiles.end());
-  return true;
+  if (upload_decision_metrics->empty()) {
+    profiles->assign(candidate_profiles.begin(), candidate_profiles.end());
+    return true;
+  }
+  return false;
 }
 
-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(
+    const std::vector<AutofillMetrics::CardUploadDecisionMetric>&
+        upload_decision_metrics) {
+  AutofillMetrics::LogCardUploadDecisionsUkm(client_->GetUkmService(),
+                                             pending_upload_request_url_,
+                                             upload_decision_metrics);

[Jared Saul, 2017/04/28 19:21:42]

still nit but very confused about style formatting: the change happening here
appears to be the exact opposite of the change happening at the top of the .h
file

[csashi, 2017/05/01 16:01:56]

Acknowledged.

 }
 
 }  // namespace autofill