[Autofill] Log RAPPOR metrics around credit card upload failures

[Autofill] Log RAPPOR metrics around credit card upload failures

Covers all cases we are interested in: conflicting names, no name, no cvc, no address.

BUG=605230
TEST=AutofillManagerTest*UploadCreditCard*

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

[Mathieu, 2016-06-21 14:46:30]

mathp@chromium.org changed reviewers:
+ holte@chromium.org, jdonnelly@chromium.org

[Mathieu, 2016-06-21 14:46:31]

PTAL

jdonnelly: components/autofill
holte: rappor.xml

Thanks!

[Justin Donnelly, 2016-06-21 15:04:54]

lgtm with suggestion

https://codereview.chromium.org/2082483005/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2082483005/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1080: if
(submitted_form.source_url().is_valid() &&
This is a complicated bit of code that's difficult to read inline. Maybe use a
function to do this? So each case instead looks like:

AutofillMetrics::LogCardUploadDecisionMetric(
    AutofillMetrics::UPLOAD_NOT_OFFERED_NO_CVC);
CollectRapportSample(submitted_form.source_url(),
    "Autofill.CardUploadNotOfferedNoCvc");

or maybe the function does both?

ReportFailureCase(
    AutofillMetrics::UPLOAD_NOT_OFFERED_NO_CVC,
    "Autofill.CardUploadNotOfferedNoCvc",
    submitted_form.source_url());

[Mathieu, 2016-06-21 17:04:10]

Patchset #3 (id:40001) has been deleted

[Mathieu, 2016-06-21 17:05:08]

PTAL now?

https://codereview.chromium.org/2082483005/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2082483005/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1080: if
(submitted_form.source_url().is_valid() &&
On 2016/06/21 at 15:04:54, Justin Donnelly wrote:
> This is a complicated bit of code that's difficult to read inline. Maybe use a
function to do this? So each case instead looks like:
> 
> AutofillMetrics::LogCardUploadDecisionMetric(
>     AutofillMetrics::UPLOAD_NOT_OFFERED_NO_CVC);
> CollectRapportSample(submitted_form.source_url(),
>     "Autofill.CardUploadNotOfferedNoCvc");
> 
> or maybe the function does both?
> 
> ReportFailureCase(
>     AutofillMetrics::UPLOAD_NOT_OFFERED_NO_CVC,
>     "Autofill.CardUploadNotOfferedNoCvc",
>     submitted_form.source_url());

Sure, I prefer the first version.

[Justin Donnelly, 2016-06-21 17:29:53]

lgtm

[Steven Holte, 2016-06-21 19:59:39]

lgtm

[Mathieu, 2016-06-21 20:00:28]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2016-06-21 20:00:50]

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

[commit-bot: I haz the power, 2016-06-21 20:07:49]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-06-21 20:14:18]

Description was changed from

==========
[Autofill] Log RAPPOR metrics around credit card upload failures

Covers all cases we are interested in: conflicting names, no name, no cvc, no
address.

BUG=605230
TEST=AutofillManagerTest*UploadCreditCard*
==========

to

==========
[Autofill] Log RAPPOR metrics around credit card upload failures

Covers all cases we are interested in: conflicting names, no name, no cvc, no
address.

BUG=605230
TEST=AutofillManagerTest*UploadCreditCard*

Committed: https://crrev.com/a84beb5f2f64fed2628b958c45ffa36c953b327b
Cr-Commit-Position: refs/heads/master@{#401087}
==========

[commit-bot: I haz the power, 2016-06-21 20:14:20]

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