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

components/autofill/core/browser/autofill_experiments.cc

 // Copyright 2015 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_experiments.h"
 
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
@@ skipping 33 matching lines @@
           switches::kDisableOfferStoreUnmaskedWalletCards))
     return false;
 
   // Otherwise use the field trial to show the checkbox or not.
   return group_name != "Disabled";
 #endif
 }
 
 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");
 
+  // Check user settings.
+  if (!pref_service->GetBoolean(prefs::kAutofillWalletSyncExperimentEnabled) ||
+      !pref_service->GetBoolean(prefs::kAutofillWalletImportEnabled)) {
+    return false;
+  }
+
+  // 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 :)

+
+  // 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))
+          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))
+          switches::kDisableOfferUploadCreditCards)) {
     return false;
+  }
 
-  if (group_name.empty() || group_name == "Disabled")
-    return false;
-
-  if (!pref_service->GetBoolean(prefs::kAutofillWalletSyncExperimentEnabled) ||
-      !pref_service->GetBoolean(prefs::kAutofillWalletImportEnabled))
-    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