Check user state and settings *before* forcing credit card upload on.

components/autofill/core/browser/autofill_experiments.cc

Index: components/autofill/core/browser/autofill_experiments.cc
diff --git a/components/autofill/core/browser/autofill_experiments.cc b/components/autofill/core/browser/autofill_experiments.cc
index 23a6e25ba7ba89cda5840417e42c906116285f85..707fda41c984252694b2b7d433bce06f9eeef5a8 100644
--- a/components/autofill/core/browser/autofill_experiments.cc
+++ b/components/autofill/core/browser/autofill_experiments.cc
@@ -51,28 +51,37 @@ bool OfferStoreUnmaskedCards() {
 
 bool IsCreditCardUploadEnabled(const PrefService* pref_service,
                                const std::string& user_email) {
-  // Query the field trial before checking command line flags to ensure UMA
-  // reports the correct group.
+  // Query the field trial first to ensure UMA always reports the correct group.
   std::string group_name =
       base::FieldTrialList::FindFullName("OfferUploadCreditCards");
 
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableOfferUploadCreditCards))
-    return true;
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kDisableOfferUploadCreditCards))
+  // Check user settings.
+  if (!pref_service->GetBoolean(prefs::kAutofillWalletSyncExperimentEnabled) ||
+      !pref_service->GetBoolean(prefs::kAutofillWalletImportEnabled)) {
     return false;
+  }
 
-  if (group_name.empty() || group_name == "Disabled")
+  // Check that the user is logged into a supported domain.
+  if (user_email.empty())
     return false;
+  std::string domain = gaia::ExtractDomainName(user_email);
+  if (!(domain == "googlemail.com" || domain == "gmail.com" ||
+        domain == "google.com")) {
+    return false;
+  }

[Ilya Sherman, 2015/12/22 21:32:05]

What Finch/variations group do you expect to be reported if any of these early
returns are reached?

[Justin Donnelly, 2015/12/22 21:38:29]

My (limited, possibly incorrect) understanding is that users won't get reported
to be part of any group (experiment, control, default) if they don't reach the
FindFullName call. If it were moved after these checks, the population sizes
could change over time, not match between control and experiment, etc.

[Ilya Sherman, 2015/12/22 21:52:02]

Yes, your understanding is correct, and I see your point.  Thanks :)

 
-  if (!pref_service->GetBoolean(prefs::kAutofillWalletSyncExperimentEnabled) ||
-      !pref_service->GetBoolean(prefs::kAutofillWalletImportEnabled))
+  // With the user settings and logged in state verified, now we can consult the
+  // command-line flags and experiment settings.
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableOfferUploadCreditCards)) {
+    return true;
+  }
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(

[Evan Stade, 2015/12/29 17:46:44]

this one can still be above

[Justin Donnelly, 2015/12/29 18:35:19]

True. But I prefer to keep it with the other command-line check for readability.

+          switches::kDisableOfferUploadCreditCards)) {
     return false;
+  }
 
-  std::string domain = gaia::ExtractDomainName(user_email);
-  return domain == "googlemail.com" || domain == "gmail.com" ||
-         domain == "google.com";
+  return !group_name.empty() && group_name != "Disabled";
 }
 
 }  // namespace autofill