Logs different SaveCardPrompt histogram names depending on if user

Logs different SaveCardPrompt histogram names depending on if user
accepted previous save card prompt.

BUG=715617
Review-Url: https://codereview.chromium.org/2839683002
Cr-Commit-Position: refs/heads/master@{#467864}
Committed: https://chromium.googlesource.com/chromium/src/+/3b669c732833d94c9b84974af7cf1b00eaeb0f32

[csashi, 2017-04-24 17:45:06]

csashi@google.com changed reviewers:
+ mathp@chromium.org

[csashi, 2017-04-24 17:45:07]

Hi Mathieu,
Please take a look. Feel free to reassign to load balance.

I have added a UMA because we want to distinguish between users who have denied
the previous prompt vs. accepted the previous prompt for both non signed-in
(i.e. local save) users as well as signed-in users.


I will follow up with 2 other changes (didn't want to combine in this change to
limit scope).

1. UKM so we can determine if accepting/denying prompt is dependent on the
domain.

2. Android metrics.

Thanks!
-sashi.

[csashi, 2017-04-24 17:45:21]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 17:46:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[csashi, 2017-04-24 18:21:19]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 18:21:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 19:35:45]

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

[commit-bot: I haz the power, 2017-04-24 19:35:46]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-04-24 20:06:46]

Description was changed from

==========
Logs different SaveCardPrompt histogram names depending on if user
accepted previous save card prompt.

BUG=
==========

to

==========
Logs different SaveCardPrompt histogram names depending on if user
accepted previous save card prompt.

BUG=
==========

[Mathieu, 2017-04-24 20:06:46]

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

[Mathieu, 2017-04-24 20:07:03]

would love Seb to have a first look, I'm a bit overloaded!

[csashi, 2017-04-24 20:18:51]

On 2017/04/24 20:07:03, Mathieu wrote:
> would love Seb to have a first look, I'm a bit overloaded!

Hi,
I have modified the mobile info bar metrics also to include user's previous
choice. I will update histograms when you accept the change.
Thanks,
-sashi.

[csashi, 2017-04-24 21:43:10]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 21:44:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 21:56:53]

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

[commit-bot: I haz the power, 2017-04-24 21:56:57]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[csashi, 2017-04-24 23:55:21]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 23:55:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 00:05:30]

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

[commit-bot: I haz the power, 2017-04-25 00:05:31]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[csashi, 2017-04-25 01:29:07]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 01:29:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 01:59:19]

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

[commit-bot: I haz the power, 2017-04-25 01:59:20]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[csashi, 2017-04-25 03:38:57]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 03:39:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 04:07:12]

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

[commit-bot: I haz the power, 2017-04-25 04:07:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[csashi, 2017-04-25 04:11:38]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 04:12:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 04:38:46]

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

[commit-bot: I haz the power, 2017-04-25 04:38:47]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sebsg, 2017-04-25 15:32:28]

Nice! Couple of comments

https://codereview.chromium.org/2839683002/diff/120001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2839683002/diff/120001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:323:
Bucket(AutofillMetrics::SAVE_CARD_PROMPT_END_ACCEPTED, 1)));
Can you also add one for the CreditCardInfoBar?

https://codereview.chromium.org/2839683002/diff/120001/components/autofill/co...
File
components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.cc
(right):

https://codereview.chromium.org/2839683002/diff/120001/components/autofill/co...
components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.cc:135:
if (user_action == AutofillMetrics::INFOBAR_ACCEPTED) {
could you just log like:

pref_service_->SetBoolean(prefs::kAutofillAccestSaveCreditCardPromptState,
user_action == AutofillMetrics::INFOBAR_ACCEPTED);

https://codereview.chromium.org/2839683002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2839683002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:118681: +<histogram_suffixes
name="AutofillCreditCardInfoBarSavePreviousUserDecision"
Question: Do you need two different suffixes? They seem to mean the same thing,
but I may be missing something :)

Thanks!

[csashi, 2017-04-25 16:13:00]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 16:13:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 16:39:12]

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

[commit-bot: I haz the power, 2017-04-25 16:39:12]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[csashi, 2017-04-25 17:22:52]

Hi Sebastien,
Please take a look. Thanks! -sashi.

https://codereview.chromium.org/2839683002/diff/120001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2839683002/diff/120001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:323:
Bucket(AutofillMetrics::SAVE_CARD_PROMPT_END_ACCEPTED, 1)));
On 2017/04/25 15:32:28, sebsg wrote:
> Can you also add one for the CreditCardInfoBar?

Please see
chrome/browser/autofill/autofill_save_card_infobar_delegate_mobile_unittest.cc

https://codereview.chromium.org/2839683002/diff/120001/components/autofill/co...
File
components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.cc
(right):

https://codereview.chromium.org/2839683002/diff/120001/components/autofill/co...
components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.cc:135:
if (user_action == AutofillMetrics::INFOBAR_ACCEPTED) {
On 2017/04/25 15:32:28, sebsg wrote:
> could you just log like:
> 
> pref_service_->SetBoolean(prefs::kAutofillAccestSaveCreditCardPromptState,
> user_action == AutofillMetrics::INFOBAR_ACCEPTED);
> 

Done. I wasn't sure if it would be expensive to set the preference if it has not
changed.

https://codereview.chromium.org/2839683002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2839683002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:118681: +<histogram_suffixes
name="AutofillCreditCardInfoBarSavePreviousUserDecision"
On 2017/04/25 15:32:28, sebsg wrote:
> Question: Do you need two different suffixes? They seem to mean the same
thing,
> but I may be missing something :)
> 
> Thanks!

Done.

[csashi, 2017-04-25 17:23:10]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 17:23:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sebsg, 2017-04-25 17:41:41]

Thanks! LGTM

[csashi, 2017-04-25 17:54:36]

csashi@google.com changed reviewers:
+ ericwilligers@chromium.org, jdonnelly@chromium.org

[csashi, 2017-04-25 17:54:37]

Hi Eric,
Please review tools/metrics/histograms/histograms.xml

Hi Justin,
Please review ios/chrome/browser/ui/autofill/autofill_client_ios.mm

Thanks,
-sashi.

[commit-bot: I haz the power, 2017-04-25 18:22:50]

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

[commit-bot: I haz the power, 2017-04-25 18:22:51]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Justin Donnelly, 2017-04-25 19:33:00]

ios/chrome/browser/ui/autofill/autofill_client_ios.mm lgtm

[csashi, 2017-04-25 21:26:30]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 21:27:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[csashi, 2017-04-25 21:28:18]

On 2017/04/24 20:07:03, Mathieu wrote:
> would love Seb to have a first look, I'm a bit overloaded!

Hi Mathieu,
Can you review for chrome/browser/ui/autofill (or reassign to Evan Stade if you
are overloaded). Thanks!
-sashi.

[commit-bot: I haz the power, 2017-04-25 23:02:55]

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

[commit-bot: I haz the power, 2017-04-25 23:02:56]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Eric Willigers, 2017-04-25 23:31:23]

ericwilligers@chromium.org changed reviewers:
+ mpearson@chromium.org
- ericwilligers@chromium.org

[Eric Willigers, 2017-04-25 23:31:24]

+mpearson for histogram review.

[Mathieu, 2017-04-26 14:31:17]

the code lgtm. One question about the metric

https://codereview.chromium.org/2839683002/diff/180001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/2839683002/diff/180001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:201: LOG(ERROR)
<< "OnBubbleClosed";
fix

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

https://codereview.chromium.org/2839683002/diff/180001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:287:
registry->RegisterBooleanPref(prefs::kAutofillAcceptSaveCreditCardPromptState,
you are starting with a default state that the user has accepted previously.
Isn't that going to skew your metrics one way?

[Mathieu, 2017-04-26 14:31:44]

On 2017/04/26 14:31:17, Mathieu wrote:
> the code lgtm. One question about the metric
> 
>
https://codereview.chromium.org/2839683002/diff/180001/chrome/browser/ui/auto...
> File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):
> 
>
https://codereview.chromium.org/2839683002/diff/180001/chrome/browser/ui/auto...
> chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:201: LOG(ERROR)
> << "OnBubbleClosed";
> fix
> 
>
https://codereview.chromium.org/2839683002/diff/180001/components/autofill/co...
> File components/autofill/core/browser/autofill_manager.cc (right):
> 
>
https://codereview.chromium.org/2839683002/diff/180001/components/autofill/co...
> components/autofill/core/browser/autofill_manager.cc:287:
> registry->RegisterBooleanPref(prefs::kAutofillAcceptSaveCreditCardPromptState,
> you are starting with a default state that the user has accepted previously.
> Isn't that going to skew your metrics one way?

Please add a bug to the CL description as well

[csashi, 2017-04-26 16:28:53]

Description was changed from

==========
Logs different SaveCardPrompt histogram names depending on if user
accepted previous save card prompt.

BUG=
==========

to

==========
Logs different SaveCardPrompt histogram names depending on if user
accepted previous save card prompt.

BUG=715617
==========

[csashi, 2017-04-26 17:58:31]

https://codereview.chromium.org/2839683002/diff/180001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/2839683002/diff/180001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:201: LOG(ERROR)
<< "OnBubbleClosed";
On 2017/04/26 14:31:16, Mathieu wrote:
> fix

Done.

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

https://codereview.chromium.org/2839683002/diff/180001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:287:
registry->RegisterBooleanPref(prefs::kAutofillAcceptSaveCreditCardPromptState,
On 2017/04/26 14:31:17, Mathieu wrote:
> you are starting with a default state that the user has accepted previously.
> Isn't that going to skew your metrics one way?

Done.

It seemed reasonable to assume that the bootstrap state is accept. I agree that
if we want to be more precise we should start with a default state of NULL and
have 3 metrics.

1. No suffix.
2. Previously accepted
3. Previously denied.

[csashi, 2017-04-26 17:58:55]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 17:59:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 18:49:16]

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

[commit-bot: I haz the power, 2017-04-26 18:49:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[csashi, 2017-04-26 19:02:52]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 19:04:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 20:23:08]

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

[commit-bot: I haz the power, 2017-04-26 20:23:09]

Dry run: This issue passed the CQ dry run.

[csashi, 2017-04-27 16:07:24]

On 2017/04/25 23:31:24, Eric Willigers wrote:
> +mpearson for histogram review.

Hi Mark,
Can you please take a look at histograms.xml? Thanks, -sashi.

[Mark P, 2017-04-27 22:52:58]

histograms.xml lgtm

--mark

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

https://codereview.chromium.org/2839683002/diff/220001/components/autofill/co...
components/autofill/core/browser/autofill_metrics.cc:94: return
previous_response;
optional nit while I'm here:
DCHECK_EQ(PREVIOUS_SAVE_CREDIT_CARD_PROMPT_USER_DECISION_NONE,
previous_save_credit_card_prompt_user_decision);

https://codereview.chromium.org/2839683002/diff/220001/components/autofill/co...
components/autofill/core/browser/autofill_metrics.cc:324:
PreviousSaveCreditCardPromptUserDecisionToString(
This structure makes is to there is no longer a single summary metric regardless
of a previous decision.  You only have three sliced metrics (previously
accepted, previously denied, nothing previous).  There is no way to view the
aggregate without manually adding up those separate histograms.  Please consider
whether viewing the aggregate would be useful.

(I imagine so, but I won't force you to do if you don't think so.)

Ditto with all changes below.

[csashi, 2017-04-27 23:37:51]

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

https://codereview.chromium.org/2839683002/diff/220001/components/autofill/co...
components/autofill/core/browser/autofill_metrics.cc:94: return
previous_response;
On 2017/04/27 22:52:58, Mark P wrote:
> optional nit while I'm here:
> DCHECK_EQ(PREVIOUS_SAVE_CREDIT_CARD_PROMPT_USER_DECISION_NONE,
> previous_save_credit_card_prompt_user_decision);

Done.

https://codereview.chromium.org/2839683002/diff/220001/components/autofill/co...
components/autofill/core/browser/autofill_metrics.cc:324:
PreviousSaveCreditCardPromptUserDecisionToString(
On 2017/04/27 22:52:58, Mark P wrote:
> This structure makes is to there is no longer a single summary metric
regardless
> of a previous decision.  You only have three sliced metrics (previously
> accepted, previously denied, nothing previous).  There is no way to view the
> aggregate without manually adding up those separate histograms.  Please
consider
> whether viewing the aggregate would be useful.
> 
> (I imagine so, but I won't force you to do if you don't think so.)
> 
> Ditto with all changes below.

It is useful to view the aggregate but I would rather burden the downstream
analysis tools rather than Chrome client. Also, it is consistent with other
stats I have seen. For example, FORM_EVENT_LOCAL_SUGGESTION_FILLED, 
FORM_EVENT_SERVER_SUGGESTION_FILLED and
FORM_EVENT_MASKED_SERVER_CARD_SUGGESTION_FILLED all refer to different types of
suggestion fills and we aggregate it on the analysis side to compute suggestion
usage rate.

[csashi, 2017-04-28 01:45:12]

The CQ bit was checked by csashi@google.com

[csashi, 2017-04-28 01:45:13]

The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org,
jdonnelly@chromium.org, mathp@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2839683002/#ps240001
(title: "Adds DCHECK for previous save card prompt user decision.")

[commit-bot: I haz the power, 2017-04-28 01:46:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 03:05:03]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1493343912756210,
"parent_rev": "2a80acc1457c86c20cff2ccbd2c17bc2a23643f8", "commit_rev":
"3b669c732833d94c9b84974af7cf1b00eaeb0f32"}

[commit-bot: I haz the power, 2017-04-28 03:05:26]

Description was changed from

==========
Logs different SaveCardPrompt histogram names depending on if user
accepted previous save card prompt.

BUG=715617
==========

to

==========
Logs different SaveCardPrompt histogram names depending on if user
accepted previous save card prompt.

BUG=715617

Review-Url: https://codereview.chromium.org/2839683002
Cr-Commit-Position: refs/heads/master@{#467864}
Committed:
https://chromium.googlesource.com/chromium/src/+/3b669c732833d94c9b84974af7cf...
==========

[commit-bot: I haz the power, 2017-04-28 03:05:27]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/3b669c732833d94c9b84974af7cf...