[Autofill] Add user type metrics.

chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc

Index: chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
diff --git a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
index bec0a50f1396697ea4fcfd8958b636709e4bc2f8..de831b54ffd9d2737be28455165c51cf857120a6 100644
--- a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
@@ -1030,6 +1030,31 @@ void AutofillDialogControllerImpl::OnDidGetFullWallet(
 
 void AutofillDialogControllerImpl::OnDidGetWalletItems(
     scoped_ptr<wallet::WalletItems> wallet_items) {
+  if (!wallet_items_ && wallet_items) {

[Dan Beam, 2013/03/12 02:06:01]

wallet_items will always be non-NULL here

+    // On the first successful response, log any relevant user type metrics.
+    // Consider a user to be an Autofill user if the user has any credit cards
+    // or addresses saved.  Check that the item count is greater than 1 because
+    // an "empty" menu still has the "add new" menu item.
+    bool has_autofill_profiles =
+        suggested_cc_.GetItemCount() > 1 ||
+        suggested_billing_.GetItemCount() > 1;
+    AutofillMetrics::DialogUserTypeMetric user_type;
+    if (SignedInState() == SIGNED_IN) {
+      if (!wallet_items->instruments().empty())
+        user_type = AutofillMetrics::DIALOG_WALLET_USER_SIGNED_IN;

[Dan Beam, 2013/03/12 02:06:01]

why are we not logging whether a user has both autofill and wallet?

+      else if (has_autofill_profiles)
+        user_type = AutofillMetrics::DIALOG_AUTOFILL_USER_SIGNED_IN;
+      else
+        user_type = AutofillMetrics::DIALOG_NEW_USER_SIGNED_IN;

[Dan Beam, 2013/03/12 02:06:01]

I think this is a little strange to assume a user is new just because they have
no autofill profiles...

+    } else {
+      if (has_autofill_profiles)
+        user_type = AutofillMetrics::DIALOG_AUTOFILL_USER_NOT_SIGNED_IN;
+      else
+        user_type = AutofillMetrics::DIALOG_NEW_USER_NOT_SIGNED_IN;
+    }
+    metric_logger_.LogDialogUserType(dialog_type_, user_type);

[Dan Beam, 2013/03/12 02:06:01]

if I were writing this it'd go something like:

  class AutofillDialogControllerImpl {
   private:
    enum DialogUserType {
      NOT_SIGNED_IN = 0,
      HAS_AUTOFILL = 1,
      SIGNED_IN = 1 << 2,
      HAS_WALLET = 1 << 3,
    };
    void CheckForAutofillProfiles();
    int user_input_;
  };

  AutofillDialogControllerImpl::AutofillDialogControllerImpl()
      : user_input_(NOT_SIGNED_IN) {
    CheckForAutofillProfiles();
  }

  AutofillDialogControllerImpl::~AutofillDialogControllerImpl() {
    metric_logger_.LogDialogUserType(dialog_type_, user_type_);
  }

  AutofillDialogControllerImpl::OnDidGetWalletItems() {
    if (SignedInState() == SIGNED_IN)
      user_type_ |= SIGNED_IN;
    if (HasCompleteWallet())
      user_type_ |= HAS_WALLET;
  }

  AutofillDialogControllerImpl::OnPersonalDataChanged() {
    CheckForAutofillProfiles();
  }

  AutofillDialogControllerImpl::CheckForAutofillProfiles() {
    if (GetManager()->GetProfiles().size() > 0)
      user_type_ |= HAS_AUTOFILL;
  }

and align the UMA buckets accordingly (i.e. 3 == signed in and autofill, 13 ==
signed in, has wallet, has autofill) -- does this seem reasonable?

it certainly seems simpler and less error prone (except the bitwise math) but
it's my code so I'm obviously biased :P.

+  }
+
   bool items_changed = !wallet_items_ || *wallet_items != *wallet_items_;
   wallet_items_ = wallet_items.Pass();
   WalletRequestCompleted(true);