[Password Manager] Add UMA stats for submitted form type

[Password Manager] Add UMA stats for submitted form type

Add stats to determine how many forms are signup forms, login forms, etc.

BUG=

Committed: https://crrev.com/d231083176f2612a3f7541d0d9609599b44cef4c
Cr-Commit-Position: refs/heads/master@{#323798}

[Garrett Casto, 2015-03-30 21:13:21]

gcasto@chromium.org changed reviewers:
+ vabr@chromium.org

[Garrett Casto, 2015-03-30 21:13:21]

I debated a bit about how to get the submitted form. I'm not wild about having
this dependency between SetSubmittedForm() and IsIgnorableChangePasswordForm(),
but I didn't like any alternatives better. Suggestions welcome.

[vabr (Chromium), 2015-03-31 10:08:18]

Hi Garrett, LGTM with comments below.

An alternative to adding the dependency of IsIgnorableChangePasswordForm on
SetSubmittedForm would be to call the latter from the former. But that would
mean the additional PasswordForm argument for IsIgnorableChangePasswordForm.
Might make sense: the callsites are coupled anyway, and it would make it clear
that IsIgnorableChangePasswordForm uses the submitted form as opposed to the
observed form.

I very slightly prefer the above alternative to the separate calls, but both are
acceptable to me -- up to you which one you like better.

Cheers,
Vaclav

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:104:
submit_result_(kSubmitResultNotSubmitted) {
form_type_ needs an initial value as well.

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:448: return
is_ignorable_change_password_form_;
Add a DCHECK that form_type_ has been set?

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:96: // Update
with the |form| that was actually submitted. Used to determine
nit: Update what? The PasswordFormManager / |this| ?

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:101: // When
attempting to provisionally save |form|, check this to check if it is
optional nit: You are the native speaker here :), but does "check this to check"
really sounds better than "call this to check"?

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:112: bool
IsIgnorableChangePasswordForm();
nit: Let's change it to a simple is_ignorable_change_password_form() accessor
for is_ignorable_change_password_form_?

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:247:
kFormTypeLogin = 0,
Currently you do not assign this value anywhere. I suggest against making this
the default value, and instead suggest assigning it in SetSubmittedForm
explicitly (when appropriate).

For the default value you might want to create another value, to carry the
meaning: "no submitted form seen yet".

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:255:
kFormTypeMax,
optional nit:
Not putting the "," after kFormTypeMax might help prevent the case when somebody
adds a value past this one by mistake.

https://codereview.chromium.org/1049003002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1049003002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:24828: +    The type  (e.g. signup,
login, change password) of all submitted password
nit: two spaces after "type" -- intentional?

[Garrett Casto, 2015-04-01 22:34:49]

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:104:
submit_result_(kSubmitResultNotSubmitted) {
On 2015/03/31 10:08:17, vabr (Chromium) wrote:
> form_type_ needs an initial value as well.

Good catch, done.

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:448: return
is_ignorable_change_password_form_;
On 2015/03/31 10:08:17, vabr (Chromium) wrote:
> Add a DCHECK that form_type_ has been set?

Done.

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:96: // Update
with the |form| that was actually submitted. Used to determine
On 2015/03/31 10:08:17, vabr (Chromium) wrote:
> nit: Update what? The PasswordFormManager / |this| ?

Yeah, done.

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:101: // When
attempting to provisionally save |form|, check this to check if it is
On 2015/03/31 10:08:17, vabr (Chromium) wrote:
> optional nit: You are the native speaker here :), but does "check this to
check"
> really sounds better than "call this to check"?

My fault, leftover from a different change.

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:112: bool
IsIgnorableChangePasswordForm();
On 2015/03/31 10:08:17, vabr (Chromium) wrote:
> nit: Let's change it to a simple is_ignorable_change_password_form() accessor
> for is_ignorable_change_password_form_?

Yeah, I went back and forth about this. While this actually is an accessor right
now, it wasn't clear that this needed to be communicated to callers. Since you
seem to prefer it, I'll switch.

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:247:
kFormTypeLogin = 0,
On 2015/03/31 10:08:17, vabr (Chromium) wrote:
> Currently you do not assign this value anywhere. I suggest against making this
> the default value, and instead suggest assigning it in SetSubmittedForm
> explicitly (when appropriate).
> 

Done.

> For the default value you might want to create another value, to carry the
> meaning: "no submitted form seen yet".

Yeah, I sort of didn't want to do that because it added a value that we wouldn't
actually upload, but the clarity it gives probably outweighs any possible
confusion about it not being uploaded. I added a comment to clarify.

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:255:
kFormTypeMax,
On 2015/03/31 10:08:17, vabr (Chromium) wrote:
> optional nit:
> Not putting the "," after kFormTypeMax might help prevent the case when
somebody
> adds a value past this one by mistake.

Sure, done.

https://codereview.chromium.org/1049003002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1049003002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:24828: +    The type  (e.g. signup,
login, change password) of all submitted password
On 2015/03/31 10:08:18, vabr (Chromium) wrote:
> nit: two spaces after "type" -- intentional?

Nope, fixed.

[Garrett Casto, 2015-04-01 22:40:55]

gcasto@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Garrett Casto, 2015-04-01 22:40:56]

+asvitkine for histograms.xml changes.

[vabr (Chromium), 2015-04-02 08:05:53]

Thanks, Garrett. LGTM!
Vaclav

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1049003002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:247:
kFormTypeLogin = 0,
On 2015/04/01 22:34:48, Garrett Casto wrote:
> On 2015/03/31 10:08:17, vabr (Chromium) wrote:
> > Currently you do not assign this value anywhere. I suggest against making
this
> > the default value, and instead suggest assigning it in SetSubmittedForm
> > explicitly (when appropriate).
> > 
> 
> Done.
> 
> > For the default value you might want to create another value, to carry the
> > meaning: "no submitted form seen yet".
> 
> Yeah, I sort of didn't want to do that because it added a value that we
wouldn't
> actually upload, but the clarity it gives probably outweighs any possible
> confusion about it not being uploaded. I added a comment to clarify.

Acknowledged.

[Alexei Svitkine (slow), 2015-04-02 14:24:08]

lgtm

https://codereview.chromium.org/1049003002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1049003002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:24829: +    forms.
Nit: Please mention when this is logged.

[Garrett Casto, 2015-04-02 21:36:34]

The CQ bit was checked by gcasto@chromium.org

[Garrett Casto, 2015-04-02 21:36:34]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1049003002/#ps80001
(title: "Comments")

[commit-bot: I haz the power, 2015-04-02 21:39:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1049003002/80001

[commit-bot: I haz the power, 2015-04-02 21:55:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-02 21:55:09]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Garrett Casto, 2015-04-02 22:07:47]

The CQ bit was checked by gcasto@chromium.org

[Garrett Casto, 2015-04-02 22:07:47]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1049003002/#ps100001
(title: "Upload")

[Garrett Casto, 2015-04-03 00:16:33]

The CQ bit was unchecked by gcasto@chromium.org

[Garrett Casto, 2015-04-03 00:16:33]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2015-04-03 00:54:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1049003002/100001

[commit-bot: I haz the power, 2015-04-03 08:06:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-03 08:06:04]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Garrett Casto, 2015-04-03 18:43:28]

The CQ bit was checked by gcasto@chromium.org

[Garrett Casto, 2015-04-03 18:43:28]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1049003002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-03 18:44:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1049003002/120001

[commit-bot: I haz the power, 2015-04-03 19:55:02]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-04-03 20:40:15]

Patchset 7 (id:??) landed as
https://crrev.com/d231083176f2612a3f7541d0d9609599b44cef4c
Cr-Commit-Position: refs/heads/master@{#323798}