[Autofill] Separate Autofill.CreditCardInfoBar into .Local and .Server

Separate Autofill.CreditCardInfoBar into .Local and .Server histograms.  Deprecate the original Autofill.CreditCardInfoBar histogram at the same time, since all traffic should go to one of the two sub-histograms.

BUG=654747
TEST=AutofillSaveCardInfoBarDelegateMobileTest (unit_tests_apk)

Committed: https://crrev.com/89f4c2373a8b7a4fa4f7029b34b4e1e8f3a2ad83
Cr-Commit-Position: refs/heads/master@{#426550}

[Mathieu, 2016-10-19 19:05:50]

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

[Mathieu, 2016-10-19 19:05:50]

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

https://codereview.chromium.org/2434543003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_metrics.cc:265:
UMA_HISTOGRAM_ENUMERATION("Autofill.CreditCardInfoBar" + destination, metric,
This macro is caching at the call site (if you follow the implementation of
UMA_HISTOGRAM_ENUMERATION) so it will "remember" the first version of the
histogram that you logged.

To fix this, just do:

if (is_uploading) {
  UMA_HISTOGRAM_ENUMERATION(...Server);
} else {
  UMA_HISTOGRAM_ENUMERATION(...Local);
}

You will see this pattern in the autofill_metrics.cc file as well. It's just a
weird limitation of the metrics macro.

[Jared Saul, 2016-10-19 21:00:13]

Patchset #1 (id:1) has been deleted

[Jared Saul, 2016-10-19 21:03:34]

Description was changed from

==========
Separate Autofill.CreditCardInfoBar into .Local and .Server

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

to

==========
Separate Autofill.CreditCardInfoBar into .Local and .Server

BUG=654747
TEST=Autofill (unit_tests_apk)
==========

[Jared Saul, 2016-10-19 21:19:16]

Hi mathp, please review the whole patch.

Thanks!

[Jared Saul, 2016-10-19 21:20:52]

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

[commit-bot: I haz the power, 2016-10-19 21:21:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2016-10-19 21:27:36]

Description was changed from

==========
Separate Autofill.CreditCardInfoBar into .Local and .Server

BUG=654747
TEST=Autofill (unit_tests_apk)
==========

to

==========
Separate Autofill.CreditCardInfoBar into .Local and .Server

BUG=654747
TEST=AutofillSaveCardInfoBarDelegateMobileTest (unit_tests_apk)
==========

[Mathieu, 2016-10-19 21:28:12]

lgtm,

Please also flesh our the change description to mention that
Autofill.CreditCardInfoBar is deprecated.

[Jared Saul, 2016-10-19 21:31:43]

Description was changed from

==========
Separate Autofill.CreditCardInfoBar into .Local and .Server

BUG=654747
TEST=AutofillSaveCardInfoBarDelegateMobileTest (unit_tests_apk)
==========

to

==========
Separate Autofill.CreditCardInfoBar into .Local and .Server histograms. 
Deprecate the original Autofill.CreditCardInfoBar histogram at the same time,
since all traffic should go to one of the two sub-histograms.

BUG=654747
TEST=AutofillSaveCardInfoBarDelegateMobileTest (unit_tests_apk)
==========

[Jared Saul, 2016-10-19 21:31:43]

jsaul@google.com changed reviewers:
+ isherman@chromium.org

[Jared Saul, 2016-10-19 21:32:02]

On 2016/10/19 21:28:12, Mathieu Perreault wrote:
> lgtm,
> 
> Please also flesh our the change description to mention that
> Autofill.CreditCardInfoBar is deprecated.

Done, thanks.

[Jared Saul, 2016-10-19 21:33:09]

Hi isherman@, please review this patch for the change to
tools/metrics/histograms/histograms.xml.  Thanks!

[Ilya Sherman, 2016-10-19 22:04:16]

https://codereview.chromium.org/2434543003/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2434543003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:3042: +</histogram>
Could you use a histogram_suffixes element, and mark the original histogram as a
"base" histogram?

[Jared Saul, 2016-10-19 22:51:51]

https://codereview.chromium.org/2434543003/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2434543003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:3042: +</histogram>
On 2016/10/19 22:04:16, Ilya Sherman wrote:
> Could you use a histogram_suffixes element, and mark the original histogram as
a
> "base" histogram?

Done, how's this?

[Ilya Sherman, 2016-10-19 23:34:08]

https://codereview.chromium.org/2434543003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2434543003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:3021: </histogram>
Please add a base="true" attribute for this histogram (it's a brand new
attribute, so there aren't a lot of examples yet, but there are a couple in this
file).

https://codereview.chromium.org/2434543003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2434543003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:3019: +    credit card info bar prompt. 
Converted to a base histogram as of 10/2016.
nit: This "converted to a base histogram ..." text will show up for the suffixed
histograms as well.  I'd just omit it.  I think that possibly you can list an
empty suffix in histogram_suffixes and mark that suffix as obsolete with the
given date, but I don't think it's super worth it.

[Jared Saul, 2016-10-20 00:07:06]

https://codereview.chromium.org/2434543003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2434543003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:3021: </histogram>
On 2016/10/19 23:34:08, Ilya Sherman wrote:
> Please add a base="true" attribute for this histogram (it's a brand new
> attribute, so there aren't a lot of examples yet, but there are a couple in
this
> file).

Apologies, but could you point out an example?  I couldn't find one when I was
looking earlier.  I also just tried to do this, and presubmit checks fail with:
ERROR:root:Unrecognized attribute "base" in tag "histogram"

https://codereview.chromium.org/2434543003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2434543003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:3019: +    credit card info bar prompt. 
Converted to a base histogram as of 10/2016.
On 2016/10/19 23:34:08, Ilya Sherman wrote:
> nit: This "converted to a base histogram ..." text will show up for the
suffixed
> histograms as well.  I'd just omit it.  I think that possibly you can list an
> empty suffix in histogram_suffixes and mark that suffix as obsolete with the
> given date, but I don't think it's super worth it.

Good point; removed.

[Ilya Sherman, 2016-10-20 00:08:45]

https://codereview.chromium.org/2434543003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2434543003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:3021: </histogram>
On 2016/10/20 00:07:06, jsaul wrote:
> On 2016/10/19 23:34:08, Ilya Sherman wrote:
> > Please add a base="true" attribute for this histogram (it's a brand new
> > attribute, so there aren't a lot of examples yet, but there are a couple in
> this
> > file).
> 
> Apologies, but could you point out an example?  I couldn't find one when I was
> looking earlier.  I also just tried to do this, and presubmit checks fail
with:
> ERROR:root:Unrecognized attribute "base" in tag "histogram"

You might need to sync your client to pick up the change:
https://codereview.chromium.org/2426473006/

[Jared Saul, 2016-10-20 00:17:18]

https://codereview.chromium.org/2434543003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2434543003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:3021: </histogram>
On 2016/10/20 00:08:45, Ilya Sherman wrote:
> On 2016/10/20 00:07:06, jsaul wrote:
> > On 2016/10/19 23:34:08, Ilya Sherman wrote:
> > > Please add a base="true" attribute for this histogram (it's a brand new
> > > attribute, so there aren't a lot of examples yet, but there are a couple
in
> > this
> > > file).
> > 
> > Apologies, but could you point out an example?  I couldn't find one when I
was
> > looking earlier.  I also just tried to do this, and presubmit checks fail
> with:
> > ERROR:root:Unrecognized attribute "base" in tag "histogram"
> 
> You might need to sync your client to pick up the change:
> https://codereview.chromium.org/2426473006/

Hadn't synced since yesterday--that did the trick, thanks!  Done.

[Ilya Sherman, 2016-10-20 00:52:16]

metrics lgtm, thanks

[Jared Saul, 2016-10-20 17:31:07]

The CQ bit was checked by jsaul@google.com

[Jared Saul, 2016-10-20 17:31:08]

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/2434543003/#ps70001
(title: "Add base=true value to histogram")

[commit-bot: I haz the power, 2016-10-20 17:32:11]

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, 2016-10-20 18:57:40]

Description was changed from

==========
Separate Autofill.CreditCardInfoBar into .Local and .Server histograms. 
Deprecate the original Autofill.CreditCardInfoBar histogram at the same time,
since all traffic should go to one of the two sub-histograms.

BUG=654747
TEST=AutofillSaveCardInfoBarDelegateMobileTest (unit_tests_apk)
==========

to

==========
Separate Autofill.CreditCardInfoBar into .Local and .Server histograms. 
Deprecate the original Autofill.CreditCardInfoBar histogram at the same time,
since all traffic should go to one of the two sub-histograms.

BUG=654747
TEST=AutofillSaveCardInfoBarDelegateMobileTest (unit_tests_apk)
==========

[commit-bot: I haz the power, 2016-10-20 18:57:42]

Committed patchset #4 (id:70001)

[commit-bot: I haz the power, 2016-10-21 13:20:54]

Description was changed from

==========
Separate Autofill.CreditCardInfoBar into .Local and .Server histograms. 
Deprecate the original Autofill.CreditCardInfoBar histogram at the same time,
since all traffic should go to one of the two sub-histograms.

BUG=654747
TEST=AutofillSaveCardInfoBarDelegateMobileTest (unit_tests_apk)
==========

to

==========
Separate Autofill.CreditCardInfoBar into .Local and .Server histograms. 
Deprecate the original Autofill.CreditCardInfoBar histogram at the same time,
since all traffic should go to one of the two sub-histograms.

BUG=654747
TEST=AutofillSaveCardInfoBarDelegateMobileTest (unit_tests_apk)

Committed: https://crrev.com/89f4c2373a8b7a4fa4f7029b34b4e1e8f3a2ad83
Cr-Commit-Position: refs/heads/master@{#426550}
==========

[commit-bot: I haz the power, 2016-10-21 13:20:55]

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