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

components/autofill/core/browser/autofill_manager.cc

Index: components/autofill/core/browser/autofill_manager.cc
diff --git a/components/autofill/core/browser/autofill_manager.cc b/components/autofill/core/browser/autofill_manager.cc
index 6921626a500115c18287ed9fd16677a5b43629e3..b9a8568dbf82f7ee833d828d604fe942858f5b8b 100644
--- a/components/autofill/core/browser/autofill_manager.cc
+++ b/components/autofill/core/browser/autofill_manager.cc
@@ -42,6 +42,7 @@
 #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"
@@ -1055,7 +1056,7 @@ void AutofillManager::OnDidGetUploadDetails(
         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:
@@ -1075,8 +1076,8 @@ void AutofillManager::OnDidGetUploadDetails(
             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();
 }
@@ -1254,14 +1255,13 @@ void AutofillManager::ImportFormData(const FormStructure& submitted_form) {
 
     // 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());
 
@@ -1269,27 +1269,20 @@ void AutofillManager::ImportFormData(const FormStructure& submitted_form) {
     // 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;
     }
 
@@ -1301,8 +1294,8 @@ void AutofillManager::ImportFormData(const FormStructure& submitted_form) {
 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();
@@ -1316,10 +1309,9 @@ bool AutofillManager::GetProfilesForCreditCardUpload(
     }
   }
   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
@@ -1328,28 +1320,28 @@ bool AutofillManager::GetProfilesForCreditCardUpload(
   // 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;
         }
       }
     }
@@ -1357,10 +1349,10 @@ bool AutofillManager::GetProfilesForCreditCardUpload(
   // 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
@@ -1369,7 +1361,7 @@ bool AutofillManager::GetProfilesForCreditCardUpload(
   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;
@@ -1385,9 +1377,9 @@ bool AutofillManager::GetProfilesForCreditCardUpload(
         // 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;
         }
       }
     }
@@ -1396,18 +1388,20 @@ bool AutofillManager::GetProfilesForCreditCardUpload(
   // 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);
@@ -2259,10 +2253,12 @@ void AutofillManager::DumpAutofillData(bool imported_cc) const {
 }
 #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