[Autofill] Add user actions for checkout flow.

Add user action metrics to analyze to most important drop off points of the Autofill checkout flow.

BUG=602292
TEST=AutofillMetricTest

Committed: https://crrev.com/4c4d78ac9ba779825e952fdba0eed619854dbd87
Cr-Commit-Position: refs/heads/master@{#387111}

[sebsg, 2016-04-10 19:03:18]

Patchset #2 (id:20001) has been deleted

[sebsg, 2016-04-10 19:08:13]

Patchset #1 (id:1) has been deleted

[sebsg, 2016-04-10 19:13:44]

Patchset #1 (id:40001) has been deleted

[sebsg, 2016-04-10 19:20:33]

Patchset #1 (id:60001) has been deleted

[sebsg, 2016-04-11 15:40:20]

Description was changed from

==========
[Autofill] Add user actions for checkout flow (WIP).
==========

to

==========
Add user action metrics to analyse to most important drop off points of the
Autofill checkout flow.
==========

[sebsg, 2016-04-11 15:43:25]

Description was changed from

==========
Add user action metrics to analyse to most important drop off points of the
Autofill checkout flow.
==========

to

==========
Add user action metrics to analyse to most important drop off points of the
Autofill checkout flow.

BUG=602292
TEST=AutofillMetricTest
==========

[sebsg, 2016-04-11 16:01:49]

Patchset #1 (id:80001) has been deleted

[sebsg, 2016-04-11 16:04:47]

Patchset #1 (id:100001) has been deleted

[sebsg, 2016-04-11 16:10:31]

Patchset #1 (id:120001) has been deleted

[sebsg, 2016-04-11 16:11:12]

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

[sebsg, 2016-04-11 16:11:12]

Hi Math could you please take a look? Thanks!

[sebsg, 2016-04-11 16:11:42]

sebsg@chromium.org changed reviewers:
+ rkaplow@chromium.org

[sebsg, 2016-04-11 16:11:54]

sebsg@chromium.org changed reviewers:
- rkaplow@chromium.org

[Mathieu, 2016-04-11 16:42:56]

Description was changed from

==========
Add user action metrics to analyse to most important drop off points of the
Autofill checkout flow.

BUG=602292
TEST=AutofillMetricTest
==========

to

==========
Add user action metrics to analyze to most important drop off points of the
Autofill checkout flow.

BUG=602292
TEST=AutofillMetricTest
==========

[Mathieu, 2016-04-11 16:45:46]

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:715: if
(autofill_field->Type().group() == CREDIT_CARD)
we will need brackets if we have multiple lines in the block, no?

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:716:
credit_card_form_event_logger_->OnDidShowSuggestions(
FormEventLogger is constructed with |is_for_credit_card|, so you don't need
this?

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_metrics.cc (right):

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_metrics.cc:734: if
(!has_logged_suggestions_shown_) {
I'm wondering if the action log should be put in the if block. Since we are
looking to log a sequence of actions, I think we don't want to miss an action in
the sequence...

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_metrics.cc:829: }
Can you add OnWillSubmitForm action too? Not all form submissions call
FormSubmitted. There is a goal to merge both OnWillSubmitForm and FormSubmitted
soon in the manager but we're not there yet.

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_metrics_unittest.cc:1632: EXPECT_EQ(1,
user_action_tester.GetActionCount("Autofill_FormSubmitted"));
if you also log OnWillSubmitForm it's ok to check for it here alongside
FormSubmitted

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:686:
base::UserMetricsAction("Autofill_PolledProfileSuggestions"));
would prefer a static method into AutofillMetrics

https://codereview.chromium.org/1874493002/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1874493002/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:906: <owner>chrome-autofill@google.com</owner>
put your chromium email here

[sebsg, 2016-04-12 14:40:59]

Thanks for reviewing. Could you take another look? Thanks!

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:715: if
(autofill_field->Type().group() == CREDIT_CARD)
On 2016/04/11 16:45:45, Mathieu Perreault wrote:
> we will need brackets if we have multiple lines in the block, no?

Done.

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:716:
credit_card_form_event_logger_->OnDidShowSuggestions(
On 2016/04/11 16:45:45, Mathieu Perreault wrote:
> FormEventLogger is constructed with |is_for_credit_card|, so you don't need
> this?

Done.

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_metrics.cc (right):

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_metrics.cc:734: if
(!has_logged_suggestions_shown_) {
On 2016/04/11 16:45:45, Mathieu Perreault wrote:
> I'm wondering if the action log should be put in the if block. Since we are
> looking to log a sequence of actions, I think we don't want to miss an action
in
> the sequence...

Done.

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_metrics.cc:829: }
On 2016/04/11 16:45:46, Mathieu Perreault wrote:
> Can you add OnWillSubmitForm action too? Not all form submissions call
> FormSubmitted. There is a goal to merge both OnWillSubmitForm and
FormSubmitted
> soon in the manager but we're not there yet.

Done.

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_metrics_unittest.cc:1632: EXPECT_EQ(1,
user_action_tester.GetActionCount("Autofill_FormSubmitted"));
On 2016/04/11 16:45:46, Mathieu Perreault wrote:
> if you also log OnWillSubmitForm it's ok to check for it here alongside
> FormSubmitted

Done.

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/1874493002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:686:
base::UserMetricsAction("Autofill_PolledProfileSuggestions"));
On 2016/04/11 16:45:46, Mathieu Perreault wrote:
> would prefer a static method into AutofillMetrics

Done.

https://codereview.chromium.org/1874493002/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1874493002/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:906: <owner>chrome-autofill@google.com</owner>
On 2016/04/11 16:45:46, Mathieu Perreault wrote:
> put your chromium email here

Done.

[Mathieu, 2016-04-12 14:44:32]

Thanks, lgtm

https://codereview.chromium.org/1874493002/diff/160001/components/autofill/co...
File components/autofill/core/browser/autofill_metrics.h (right):

https://codereview.chromium.org/1874493002/diff/160001/components/autofill/co...
components/autofill/core/browser/autofill_metrics.h:710: void
OnDidPollSuggestions();
nit: I would move this after OnDidInteract to respect the order of things

[sebsg, 2016-04-12 15:07:47]

sebsg@chromium.org changed reviewers:
+ rkaplow@chromium.org

[sebsg, 2016-04-12 15:07:47]

Hi Robert, could you please review tools/metrics/actions/actions.xml ?

Thanks!

https://codereview.chromium.org/1874493002/diff/160001/components/autofill/co...
File components/autofill/core/browser/autofill_metrics.h (right):

https://codereview.chromium.org/1874493002/diff/160001/components/autofill/co...
components/autofill/core/browser/autofill_metrics.h:710: void
OnDidPollSuggestions();
On 2016/04/12 14:44:32, Mathieu Perreault wrote:
> nit: I would move this after OnDidInteract to respect the order of things

Done.

[rkaplow, 2016-04-13 20:42:21]

lgtm

oops, sorry for delay

[sebsg, 2016-04-13 20:43:22]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2016-04-13 20:43:23]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/1874493002/#ps180001
(title: "Nits")

[commit-bot: I haz the power, 2016-04-13 20:43:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1874493002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1874493002/180001

[commit-bot: I haz the power, 2016-04-13 21:52:04]

Description was changed from

==========
Add user action metrics to analyze to most important drop off points of the
Autofill checkout flow.

BUG=602292
TEST=AutofillMetricTest
==========

to

==========
Add user action metrics to analyze to most important drop off points of the
Autofill checkout flow.

BUG=602292
TEST=AutofillMetricTest
==========

[commit-bot: I haz the power, 2016-04-13 21:52:05]

Committed patchset #3 (id:180001)

[commit-bot: I haz the power, 2016-04-13 21:53:49]

Description was changed from

==========
Add user action metrics to analyze to most important drop off points of the
Autofill checkout flow.

BUG=602292
TEST=AutofillMetricTest
==========

to

==========
Add user action metrics to analyze to most important drop off points of the
Autofill checkout flow.

BUG=602292
TEST=AutofillMetricTest

Committed: https://crrev.com/4c4d78ac9ba779825e952fdba0eed619854dbd87
Cr-Commit-Position: refs/heads/master@{#387111}
==========

[commit-bot: I haz the power, 2016-04-13 21:53:50]

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