Add some basic success/failure UMA logging for autofill.

Add some basic success/failure UMA logging for autofill.

BUG=none
TEST=unit_tests --gtest_filter=AutoFillMetricsTest.*

[Ilya Sherman, 2010-12-10 09:07:42]

This only adds logging for the simple success and failure metrics David and I
talked about offline a couple of days ago.  We should decide exactly what UMA
logging we want to add (to start with) and then I'll ping jar@ to make sure that
what we're doing seems more or less sane.

http://codereview.chromium.org/5703002/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_metrics_unittest.cc (right):

http://codereview.chromium.org/5703002/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_metrics_unittest.cc:85: // A slightly hacky way
to verify that the right log calls are being made.
This is pretty hacky -- any thoughts on how to make it better?

[dhollowa, 2010-12-10 21:37:38]

http://codereview.chromium.org/5703002/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_download.h (right):

http://codereview.chromium.org/5703002/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_download.h:107: bool StartRequest(
The previous formatting is better.

http://codereview.chromium.org/5703002/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_metrics.h (right):

http://codereview.chromium.org/5703002/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_metrics.h:49: typedef void
(*LogQualityMetricFn)(QualityMetricType);
I think this all might be cleaner if we just change from |autofill_metrics| from
a namespace, to |AutoFillMetrics| as a class.  Then |AutoFillManager| can hold a
scoped-pointer to an instance of this class (supplied by the creator, for
dependency injection).  Similarly, with |AutoFillDownloadManager| and
|FormStructure|... they can hold their own scoped-pointers to an
|AutoFillMetrics| instance.

This will also let us use gmock in the tests more cleanly.

[Ilya Sherman, 2010-12-11 03:35:02]

In addition to the below changes, I changed the code around to no longer log for
<select> elements, since we can't detect success or failure for them quite
properly.  Better to have less data than wrong data =)

http://codereview.chromium.org/5703002/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_download.h (right):

http://codereview.chromium.org/5703002/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_download.h:107: bool StartRequest(
On 2010/12/10 21:37:38, dhollowa wrote:
> The previous formatting is better.

Oops, had already fixed that locally. Done.

http://codereview.chromium.org/5703002/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_metrics.h (right):

http://codereview.chromium.org/5703002/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_metrics.h:49: typedef void
(*LogQualityMetricFn)(QualityMetricType);
On 2010/12/10 21:37:38, dhollowa wrote:
> I think this all might be cleaner if we just change from |autofill_metrics|
from
> a namespace, to |AutoFillMetrics| as a class.  Then |AutoFillManager| can hold
a
> scoped-pointer to an instance of this class (supplied by the creator, for
> dependency injection).  Similarly, with |AutoFillDownloadManager| and
> |FormStructure|... they can hold their own scoped-pointers to an
> |AutoFillMetrics| instance.
> 
> This will also let us use gmock in the tests more cleanly.

I changed this around to use two classes, one for each type of log. 
|AutoFillManager| has a scoped-pointer, but |AutoFillDownlaodManager| and
|FormStructure| just take loggers by const-reference.  This seems to work fairly
nicely =)

[dhollowa, 2010-12-12 07:07:51]

http://codereview.chromium.org/5703002/diff/15001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_download.h (right):

http://codereview.chromium.org/5703002/diff/15001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_download.h:19: class
AutoFillServerQueryMetricLogger;
Wow, long name...  How about AutoFillMetrics?

http://codereview.chromium.org/5703002/diff/15001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_download_unittest.cc (right):

http://codereview.chromium.org/5703002/diff/15001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_download_unittest.cc:27: // TODO(isherman): Use
gmock instead and test that metrics are logged as we'd
Let's use gmock now.

http://codereview.chromium.org/5703002/diff/15001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_metrics.h (right):

http://codereview.chromium.org/5703002/diff/15001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_metrics.h:9: class
AutoFillServerQueryMetricLogger {
I'd prefer to combine these two classes into a single AutoFillMetrics class, and
make the metrics distinction at the |Log| method level.  I.e. either
Log(ServerMetrics) and Log(QualityMetrics), or LogServerMetrics(...) and
LogQualityMetrics(...).

[Ilya Sherman, 2010-12-13 22:13:51]

http://codereview.chromium.org/5703002/diff/15001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_download.h (right):

http://codereview.chromium.org/5703002/diff/15001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_download.h:19: class
AutoFillServerQueryMetricLogger;
On 2010/12/12 07:07:51, dhollowa wrote:
> Wow, long name...  How about AutoFillMetrics?

Done.

http://codereview.chromium.org/5703002/diff/15001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_download_unittest.cc (right):

http://codereview.chromium.org/5703002/diff/15001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_download_unittest.cc:27: // TODO(isherman): Use
gmock instead and test that metrics are logged as we'd
On 2010/12/12 07:07:51, dhollowa wrote:
> Let's use gmock now.

Done.

http://codereview.chromium.org/5703002/diff/15001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_metrics.h (right):

http://codereview.chromium.org/5703002/diff/15001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_metrics.h:9: class
AutoFillServerQueryMetricLogger {
On 2010/12/12 07:07:51, dhollowa wrote:
> I'd prefer to combine these two classes into a single AutoFillMetrics class,
and
> make the metrics distinction at the |Log| method level.  I.e. either
> Log(ServerMetrics) and Log(QualityMetrics), or LogServerMetrics(...) and
> LogQualityMetrics(...).

Done.

[dhollowa, 2010-12-14 00:01:55]

LGTM (with one small nit).  Let the data flow!

http://codereview.chromium.org/5703002/diff/23001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_metrics.h (right):

http://codereview.chromium.org/5703002/diff/23001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_metrics.h:54: };
nit: DISALLOW_COPY_AND_ASSIGN(AutoFillMetrics);