[Autofill] Add user type metrics.

[Autofill] Add user type metrics.

Add metrics identifying the type of user interacting with the autocomplete
dialog.  Users are categorized according to three parameters:
(1) Are they signed in?
(2) For signed in users, do they have any data saved in Wallet?
(3) For all users without data saved in Wallet, do they have any data saved in
    local Autofill?

BUG=165570
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188163

[Dan Beam, 2013-03-07 17:16:50]

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/auto...
File chrome/browser/autofill/autofill_metrics.h (right):

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/auto...
chrome/browser/autofill/autofill_metrics.h:71: DIALOG_NEW_USER_NOT_SIGNED_IN =
0,
is a user "new" if this is their *first* (and only first?) time running the
dialog?

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/auto...
chrome/browser/autofill/autofill_metrics.h:74: // User signed in, no Wallet
items, but 1+ verified Autofill profiles.
nit: if you could change "1+" to something that doesn't look like maths (i.e.
"User signed in, no Wallet items, has a verified Autofill profile." it'd be
easier for sleepy Dans to parse, IMO.

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1030: if
(!wallet_items_ && wallet_items) {
I don't really know how you'd make this harder to misread, but I definitely
thought this said

  if (!wallet_items && wallet_items)

which would be never true @_@ (just a _ difference is common, I guess... I'm
doing it below)

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1030: if
(!wallet_items_ && wallet_items) {
this currently assumes we don't re-request wallet items on the first error.  if
this ever changes this will break.

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1037:
suggested_billing_.GetItemCount() > 1;
what if there's a network error/wallet error/malformed response/this response
never comes?

[Ilya Sherman, 2013-03-09 01:58:56]

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/auto...
File chrome/browser/autofill/autofill_metrics.h (right):

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/auto...
chrome/browser/autofill/autofill_metrics.h:71: DIALOG_NEW_USER_NOT_SIGNED_IN =
0,
On 2013/03/07 17:16:50, Dan Beam wrote:
> is a user "new" if this is their *first* (and only first?) time running the
> dialog?

No, it's as the comment says.  Do you have a different name that you'd prefer
for this enum constant?

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/auto...
chrome/browser/autofill/autofill_metrics.h:74: // User signed in, no Wallet
items, but 1+ verified Autofill profiles.
On 2013/03/07 17:16:50, Dan Beam wrote:
> nit: if you could change "1+" to something that doesn't look like maths (i.e.
> "User signed in, no Wallet items, has a verified Autofill profile." it'd be
> easier for sleepy Dans to parse, IMO.

Done.

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1030: if
(!wallet_items_ && wallet_items) {
On 2013/03/07 17:16:50, Dan Beam wrote:
> this currently assumes we don't re-request wallet items on the first error. 
if
> this ever changes this will break.

Do you have a suggestion on how to better hook this?  Add a boolean that tracks
whether this is the first time reaching here?

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1037:
suggested_billing_.GetItemCount() > 1;
On 2013/03/07 17:16:50, Dan Beam wrote:
> what if there's a network error/wallet error/malformed response/this response
> never comes?

Then we won't get a stat for that user.  Since this should affect all users
roughly equally, I'm not too worried about that.

[Dan Beam, 2013-03-09 02:02:31]

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1030: if
(!wallet_items_ && wallet_items) {
On 2013/03/09 01:58:57, Ilya Sherman wrote:
> On 2013/03/07 17:16:50, Dan Beam wrote:
> > this currently assumes we don't re-request wallet items on the first error. 
> if
> > this ever changes this will break.
> 
> Do you have a suggestion on how to better hook this?  Add a boolean that
tracks
> whether this is the first time reaching here?

^ yes, something along these lines, or figure out a way to ask metric_logger_
whether it's already logged this type of metric.

https://chromiumcodereview.appspot.com/12588002/diff/5001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1037:
suggested_billing_.GetItemCount() > 1;
On 2013/03/09 01:58:57, Ilya Sherman wrote:
> On 2013/03/07 17:16:50, Dan Beam wrote:
> > what if there's a network error/wallet error/malformed response/this
response
> > never comes?
> 
> Then we won't get a stat for that user.  Since this should affect all users
> roughly equally, I'm not too worried about that.

how do you know this will affect users equally? can't you simply move this into
WalletRequestCompleted()?

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

https://codereview.chromium.org/12588002/diff/12001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12588002/diff/12001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1033: if
(!wallet_items_ && wallet_items) {
wallet_items will always be non-NULL here

https://codereview.chromium.org/12588002/diff/12001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1044: user_type =
AutofillMetrics::DIALOG_WALLET_USER_SIGNED_IN;
why are we not logging whether a user has both autofill and wallet?

https://codereview.chromium.org/12588002/diff/12001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1048: user_type =
AutofillMetrics::DIALOG_NEW_USER_SIGNED_IN;
I think this is a little strange to assume a user is new just because they have
no autofill profiles...

https://codereview.chromium.org/12588002/diff/12001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1055:
metric_logger_.LogDialogUserType(dialog_type_, user_type);
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.

[Ilya Sherman, 2013-03-12 08:13:44]

PTAL; I think I've rewritten this code to address all of your concerns.

[Dan Beam, 2013-03-12 14:59:32]

lgtm w/nit

https://chromiumcodereview.appspot.com/12588002/diff/21001/components/autofil...
File components/autofill/browser/autofill_metrics.cc (right):

https://chromiumcodereview.appspot.com/12588002/diff/21001/components/autofil...
components/autofill/browser/autofill_metrics.cc:301:
DialogInitialUserStateMetric user_type) const {
DCHECK_NE(UNKNOWN_TYPE, user_type);

[Ilya Sherman, 2013-03-12 20:51:27]

https://chromiumcodereview.appspot.com/12588002/diff/21001/components/autofil...
File components/autofill/browser/autofill_metrics.cc (right):

https://chromiumcodereview.appspot.com/12588002/diff/21001/components/autofil...
components/autofill/browser/autofill_metrics.cc:301:
DialogInitialUserStateMetric user_type) const {
On 2013/03/12 14:59:32, Dan Beam wrote:
> DCHECK_NE(UNKNOWN_TYPE, user_type);

This DCHECK wouldn't be valid, since it's possible that none of the Wallet
requests succeeded.

[commit-bot: I haz the power, 2013-03-13 02:24:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/12588002/21001

[commit-bot: I haz the power, 2013-03-13 02:24:38]

Failed to apply patch for components/autofill/browser/autofill_metrics.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file components/autofill/browser/autofill_metrics.cc
  Hunk #1 FAILED at 296.
  1 out of 1 hunk FAILED -- saving rejects to file
components/autofill/browser/autofill_metrics.cc.rej

Patch:       components/autofill/browser/autofill_metrics.cc
Index: components/autofill/browser/autofill_metrics.cc
diff --git a/components/autofill/browser/autofill_metrics.cc
b/components/autofill/browser/autofill_metrics.cc
index
11530a6ce54836a46a08ecfcc9f4fb38ac19788e..8154280af863167ddb73b8a6fe2e4b95c9ca558b
100644
--- a/components/autofill/browser/autofill_metrics.cc
+++ b/components/autofill/browser/autofill_metrics.cc
@@ -296,6 +296,14 @@ void
AutofillMetrics::LogCreditCardInfoBarMetric(InfoBarMetric metric) const {
                             NUM_INFO_BAR_METRICS);
 }
 
+void AutofillMetrics::LogDialogInitialUserState(
+    autofill::DialogType dialog_type,
+    DialogInitialUserStateMetric user_type) const {
+  std::string name = GetPrefixForDialogType(dialog_type) + ".InitialUserState";
+  LogUMAHistogramEnumeration(
+      name, user_type, NUM_DIALOG_INITIAL_USER_STATE_METRICS);
+}
+
 void AutofillMetrics::LogDialogSecurityMetric(
     autofill::DialogType dialog_type,
     DialogSecurityMetric metric) const {

[commit-bot: I haz the power, 2013-03-13 04:31:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/12588002/40001

[commit-bot: I haz the power, 2013-03-13 04:44:23]

Retried try job too often on linux_chromeos_clang for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-14 00:22:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/12588002/40001

[commit-bot: I haz the power, 2013-03-14 02:44:09]

Retried try job too often on mac_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-03-14 03:44:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/12588002/68001

[commit-bot: I haz the power, 2013-03-14 04:11:48]

Failed to trigger a try job on mac_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-03-14 04:12:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/12588002/76002

[commit-bot: I haz the power, 2013-03-14 09:53:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/12588002/85004

[commit-bot: I haz the power, 2013-03-14 19:54:01]

Change committed as 188163