[Autofill] Log the number of autofill profiles available at form submission

[Autofill] Log the number of autofill profiles available at form submission

BUG=None
TEST=AutofillMetricsTest

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

[Mathieu, 2015-01-28 19:19:56]

mathp@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2015-01-28 23:17:57]

Seems like a useful metric :)  Please remember to update histograms.xml in
addition to writing tests.

https://codereview.chromium.org/880353002/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/autofill_metrics.cc (right):

https://codereview.chromium.org/880353002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/autofill_metrics.cc:465:
UMA_HISTOGRAM_COUNTS("Autofill.StoredProfileCountAtSubmission", num_profiles);
nit: I'd make the name a little bit more explicit, i.e. instead of
"AtSubmission" I'd write "AtAutofillableFormSubmission", or something like that.

[Mathieu, 2015-01-28 23:18:24]

On 2015/01/28 23:17:57, Ilya Sherman wrote:
> Seems like a useful metric :)  Please remember to update histograms.xml in
> addition to writing tests.
> 
>
https://codereview.chromium.org/880353002/diff/1/components/autofill/core/bro...
> File components/autofill/core/browser/autofill_metrics.cc (right):
> 
>
https://codereview.chromium.org/880353002/diff/1/components/autofill/core/bro...
> components/autofill/core/browser/autofill_metrics.cc:465:
> UMA_HISTOGRAM_COUNTS("Autofill.StoredProfileCountAtSubmission", num_profiles);
> nit: I'd make the name a little bit more explicit, i.e. instead of
> "AtSubmission" I'd write "AtAutofillableFormSubmission", or something like
that.

Thanks, will clean it up!

[Evan Stade, 2015-01-28 23:24:59]

estade@chromium.org changed reviewers:
+ estade@chromium.org

[Evan Stade, 2015-01-28 23:25:00]

you should coordinate with waltercacau@, who has been looking at fleshing out
the autofill metrics.

[Mathieu, 2015-01-29 15:43:15]

mathp@chromium.org changed reviewers:
+ waltercacau@chromium.org

[Mathieu, 2015-01-29 18:28:40]

Hi Ilya, can you have a look?

[Ilya Sherman, 2015-01-29 20:32:38]

On 2015/01/29 18:28:40, Mathieu Perreault wrote:
> Hi Ilya, can you have a look?

Did you follow Evan's recommendation of coordinating with waltercacau@?

[Ilya Sherman, 2015-01-29 20:34:52]

In terms of the code:

https://codereview.chromium.org/880353002/diff/20001/components/autofill/core...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/880353002/diff/20001/components/autofill/core...
components/autofill/core/browser/autofill_manager_unittest.cc:2237:
"Autofill.StoredProfileCountAtAutofillableFormSubmission", 3, 1);
Please move this test to the autofill_metrics_unittest file, and please write
minimal tests to cover the possible cases.  At the least, I would suggest having
one test for when this histogram is emitted to, and one test verifying that it
is *not* emitted to when a non-autofillable form is submitted.

[Mathieu, 2015-01-29 20:37:11]

On 2015/01/29 20:32:38, Ilya Sherman wrote:
> On 2015/01/29 18:28:40, Mathieu Perreault wrote:
> > Hi Ilya, can you have a look?
> 
> Did you follow Evan's recommendation of coordinating with waltercacau@?

I did talk with Walter, yes.

[Walter Cacau, 2015-01-29 20:38:20]

On 2015/01/29 20:34:52, Ilya Sherman wrote:
> In terms of the code:
> 
>
https://codereview.chromium.org/880353002/diff/20001/components/autofill/core...
> File components/autofill/core/browser/autofill_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/880353002/diff/20001/components/autofill/core...
> components/autofill/core/browser/autofill_manager_unittest.cc:2237:
> "Autofill.StoredProfileCountAtAutofillableFormSubmission", 3, 1);
> Please move this test to the autofill_metrics_unittest file, and please write
> minimal tests to cover the possible cases.  At the least, I would suggest
having
> one test for when this histogram is emitted to, and one test verifying that it
> is *not* emitted to when a non-autofillable form is submitted.

Hi Ilya, we talked and it seems what we are doing is a bit orthogonal. He is
trying to get data regarding how many profiles are available in Local in order
to support an effort of increasing the quality of the local profile DB [please
correct if I am wrong].

I am trying to get data regarding the usage of autofill depending on local data
or server data being available and selected by the user. There may be some
overlapping metrics but we can cross check each other's CL IMHO. Agreed?

[Mathieu, 2015-01-29 20:41:45]

On 2015/01/29 20:38:20, waltercacau wrote:
> On 2015/01/29 20:34:52, Ilya Sherman wrote:
> > In terms of the code:
> > 
> >
>
https://codereview.chromium.org/880353002/diff/20001/components/autofill/core...
> > File components/autofill/core/browser/autofill_manager_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/880353002/diff/20001/components/autofill/core...
> > components/autofill/core/browser/autofill_manager_unittest.cc:2237:
> > "Autofill.StoredProfileCountAtAutofillableFormSubmission", 3, 1);
> > Please move this test to the autofill_metrics_unittest file, and please
write
> > minimal tests to cover the possible cases.  At the least, I would suggest
> having
> > one test for when this histogram is emitted to, and one test verifying that
it
> > is *not* emitted to when a non-autofillable form is submitted.
> 
> Hi Ilya, we talked and it seems what we are doing is a bit orthogonal. He is
> trying to get data regarding how many profiles are available in Local in order
> to support an effort of increasing the quality of the local profile DB [please
> correct if I am wrong].
> 
> I am trying to get data regarding the usage of autofill depending on local
data
> or server data being available and selected by the user. There may be some
> overlapping metrics but we can cross check each other's CL IMHO. Agreed?

Yes, by local and server data, in particular Walter is interested in the credit
card fields whereas I'll be looking at other aspects.

[Ilya Sherman, 2015-01-29 20:42:16]

On 2015/01/29 20:38:20, waltercacau wrote:
> Hi Ilya, we talked and it seems what we are doing is a bit orthogonal. He is
> trying to get data regarding how many profiles are available in Local in order
> to support an effort of increasing the quality of the local profile DB [please
> correct if I am wrong].
> 
> I am trying to get data regarding the usage of autofill depending on local
data
> or server data being available and selected by the user. There may be some
> overlapping metrics but we can cross check each other's CL IMHO. Agreed?

Yep, sounds good to me.  I just wanted to make sure to follow up on Evan's
comment on the thread :)

[Mathieu, 2015-01-30 14:36:46]

mathp@chromium.org changed reviewers:
- estade@chromium.org, waltercacau@chromium.org

[Mathieu, 2015-01-30 15:22:41]

Thanks, another look?

https://codereview.chromium.org/880353002/diff/20001/components/autofill/core...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/880353002/diff/20001/components/autofill/core...
components/autofill/core/browser/autofill_manager_unittest.cc:2237:
"Autofill.StoredProfileCountAtAutofillableFormSubmission", 3, 1);
On 2015/01/29 20:34:52, Ilya Sherman wrote:
> Please move this test to the autofill_metrics_unittest file, and please write
> minimal tests to cover the possible cases.  At the least, I would suggest
having
> one test for when this histogram is emitted to, and one test verifying that it
> is *not* emitted to when a non-autofillable form is submitted.

Done.

[Ilya Sherman, 2015-01-30 22:11:45]

https://codereview.chromium.org/880353002/diff/40001/components/autofill/core...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/880353002/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_metrics_unittest.cc:369:
"Autofill.StoredProfileCountAtAutofillableFormSubmission", 2, 1);
This isn't a quality metric.  Please write a separate TEST_F test case for this
histogram.

[Mathieu, 2015-01-31 18:25:52]

Thanks, another look?

https://codereview.chromium.org/880353002/diff/40001/components/autofill/core...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/880353002/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_metrics_unittest.cc:369:
"Autofill.StoredProfileCountAtAutofillableFormSubmission", 2, 1);
On 2015/01/30 22:11:44, Ilya Sherman wrote:
> This isn't a quality metric.  Please write a separate TEST_F test case for
this
> histogram.

Done. Thanks, cleaner now.

[Mathieu, 2015-01-31 18:27:30]

Patchset #4 (id:60001) has been deleted

[Ilya Sherman, 2015-02-02 22:37:16]

LGTM % a final nit.  Thanks!

https://codereview.chromium.org/880353002/diff/80001/components/autofill/core...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/880353002/diff/80001/components/autofill/core...
components/autofill/core/browser/autofill_metrics_unittest.cc:529: {
nit: No need for this scope, since the test is now simple enough to only be
testing a single thing :)  Ditto below.

[Mathieu, 2015-02-03 03:13:42]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2015-02-03 03:13:52]

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

[Mathieu, 2015-02-03 03:14:01]

Thanks, submitting

https://codereview.chromium.org/880353002/diff/80001/components/autofill/core...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/880353002/diff/80001/components/autofill/core...
components/autofill/core/browser/autofill_metrics_unittest.cc:529: {
On 2015/02/02 22:37:16, Ilya Sherman wrote:
> nit: No need for this scope, since the test is now simple enough to only be
> testing a single thing :)  Ditto below.

Done.

[commit-bot: I haz the power, 2015-02-03 04:53:18]

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

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

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2015-02-03 12:23:52]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2015-02-03 12:24:14]

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

[commit-bot: I haz the power, 2015-02-03 14:11:48]

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

[commit-bot: I haz the power, 2015-02-03 14:11:49]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2015-02-03 21:54:49]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2015-02-03 21:55:13]

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

[commit-bot: I haz the power, 2015-02-03 23:32:17]

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

[commit-bot: I haz the power, 2015-02-03 23:32:18]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2015-02-09 20:08:14]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2015-02-09 20:08:52]

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

[commit-bot: I haz the power, 2015-02-09 21:56:39]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-02-09 21:57:16]

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