[Payments] Add information requested by merchant metrics to PR.

Log the different information that the merchant requires to complete a Payment Request.

BUG=631487
Committed: https://crrev.com/f261072421878405b0fa994856538250c2cf1f59
Cr-Commit-Position: refs/heads/master@{#409183}

[sebsg, 2016-07-26 15:40:37]

Description was changed from

==========
[Payments] Add information requested by merchant metrics to PR.

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

to

==========
Log the different information that the merchant requires to complete a Payment
Request.

BUG=631487
TEST=PaymentRequestTest
==========

[sebsg, 2016-07-26 15:51:15]

Patchset #1 (id:1) has been deleted

[sebsg, 2016-07-26 16:41:46]

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

[sebsg, 2016-07-26 16:41:46]

Hi Rouslan PTAL?

[please use gerrit instead, 2016-07-26 21:40:34]

As discussed on chat, let's add a PaymentRequestMetrics.java file that you can
unit test instead of PaymentRequestImpl directly. Also let's add integration
test cases for metrics in the existing PaymentRequest*Test.java files.

[sebsg, 2016-07-27 14:54:51]

Description was changed from

==========
Log the different information that the merchant requires to complete a Payment
Request.

BUG=631487
TEST=PaymentRequestTest
==========

to

==========
Log the different information that the merchant requires to complete a Payment
Request.

BUG=631487
==========

[sebsg, 2016-07-27 15:17:35]

Patchset #2 (id:40001) has been deleted

[sebsg, 2016-07-27 15:26:23]

Patchset #3 (id:80001) has been deleted

[sebsg, 2016-07-27 15:26:43]

Patchset #2 (id:60001) has been deleted

[sebsg, 2016-07-27 15:27:02]

Hi Rouslan, could you take another look? Thanks

[please use gerrit instead, 2016-07-27 16:24:40]

Great job on testing!

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java
(right):

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java:15:
public static final int REQUESTED_INFORMATION_NONE = 0;
Put @VisibleForTesting on all of these constants.

All public constants need Javadoc.

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java:28:
public static void recordRequestedInformationHistogram(boolean requestEmail,
All public functions need Javadoc.

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java:31:
if (requestEmail) {
The simpler way to map 3 booleans to a number: consider them bits. You will need
to re-order a little, however.

  shipping=4 phone=2 email=1
     0         0       0      NONE=0
     0         0       1      EMAIL=1
     0         1       0      PHONE=2
     0         1       1      PHONE_EMAIL=3
     1         0       0      SHIPPING=4
     1         0       1      SHIPPING_EMAIL=5
     1         1       0      SHIPPING_PHONE=6
     1         1       1      SHIPPING_PHONE_EMAIL=7

Then your function can be simpler:

int requestInformation =
        (requestEmail ? 1 : 0) +
        (requestPhone ? 2 : 0) +
        (requestShipping ? 4 : 0);

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestContactDetailsTest.java
(right):

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestContactDetailsTest.java:188:
assertEquals(1, RecordHistogram.getHistogramValueCountForTesting(
Here and elsewhere, save a few lines like this:

PaymentRequestMetrics.REQUESTED_INFORMATION_EMAIL_PHONE));
for (int i = 0; i < PaymentRequestMetrics.REQUESTED_INFORMATION_MAX; ++i) {
  assertEquals((i == PaymentRequestMetrics.REQUESTED_INFORMATION_EMAIL_PHONE ? 1
: 0),
           RecordHistogram.getHistogramValueCountForTesting(
                   "PaymentRequest.RequestedInformation", i));
}

(Indents might be different.)

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestMetricsUnitTest.java
(right):

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestMetricsUnitTest.java:28:
private static final int REQUESTED_INFORMATION_MAX = 8;
Probably no need to duplicate the constants here?

[sebsg, 2016-07-27 17:26:01]

Thanks for the suggestions! Another look?

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java
(right):

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java:15:
public static final int REQUESTED_INFORMATION_NONE = 0;
On 2016/07/27 16:24:40, rouslan wrote:
> Put @VisibleForTesting on all of these constants.
> 
> All public constants need Javadoc.

Done.

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java:28:
public static void recordRequestedInformationHistogram(boolean requestEmail,
On 2016/07/27 16:24:40, rouslan wrote:
> All public functions need Javadoc.

Done.

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java:31:
if (requestEmail) {
On 2016/07/27 16:24:40, rouslan wrote:
> The simpler way to map 3 booleans to a number: consider them bits. You will
need
> to re-order a little, however.
> 
>   shipping=4 phone=2 email=1
>      0         0       0      NONE=0
>      0         0       1      EMAIL=1
>      0         1       0      PHONE=2
>      0         1       1      PHONE_EMAIL=3
>      1         0       0      SHIPPING=4
>      1         0       1      SHIPPING_EMAIL=5
>      1         1       0      SHIPPING_PHONE=6
>      1         1       1      SHIPPING_PHONE_EMAIL=7
> 
> Then your function can be simpler:
> 
> int requestInformation =
>         (requestEmail ? 1 : 0) +
>         (requestPhone ? 2 : 0) +
>         (requestShipping ? 4 : 0);

Nice thanks!

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestContactDetailsTest.java
(right):

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestContactDetailsTest.java:188:
assertEquals(1, RecordHistogram.getHistogramValueCountForTesting(
On 2016/07/27 16:24:40, rouslan wrote:
> Here and elsewhere, save a few lines like this:
> 
> PaymentRequestMetrics.REQUESTED_INFORMATION_EMAIL_PHONE));
> for (int i = 0; i < PaymentRequestMetrics.REQUESTED_INFORMATION_MAX; ++i) {
>   assertEquals((i == PaymentRequestMetrics.REQUESTED_INFORMATION_EMAIL_PHONE ?
1
> : 0),
>            RecordHistogram.getHistogramValueCountForTesting(
>                    "PaymentRequest.RequestedInformation", i));
> }
> 
> (Indents might be different.)

Done.

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestMetricsUnitTest.java
(right):

https://codereview.chromium.org/2182123003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestMetricsUnitTest.java:28:
private static final int REQUESTED_INFORMATION_MAX = 8;
On 2016/07/27 16:24:40, rouslan wrote:
> Probably no need to duplicate the constants here?

Done.

[please use gerrit instead, 2016-07-28 01:49:25]

lgtm

[sebsg, 2016-07-28 02:47:33]

sebsg@chromium.org changed reviewers:
+ jwd@chromium.org, yfriedman@chromium.org

[sebsg, 2016-07-28 02:47:33]

yfriedman@chromium.org: Could you please review changes in chrome/android ?

jwd@chromium.org: Could you please review histograms.xml ?

Thanks!

[jwd, 2016-07-28 13:30:27]

lgtm

[sebsg, 2016-07-29 20:53:22]

sebsg@chromium.org changed reviewers:
+ bauerb@chromium.org
- yfriedman@chromium.org

[sebsg, 2016-07-29 20:53:23]

Hi bauerb@, could you please take a look at this CL? 

Thanks!

[Bernhard Bauer, 2016-08-01 09:20:02]

lgtm

https://codereview.chromium.org/2182123003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java
(right):

https://codereview.chromium.org/2182123003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java:17:
public static final int REQUESTED_INFORMATION_NONE = 0;
You could define three base values (REQUESTED_INFORMATION_EMAIL = 1 << 0,
REQUESTED_INFORMATION_PHONE = 1 << 1, REQUESTED_INFORMATION_SHIPPING = 1 << 2)
and construct the rest from those. Then you could also use them below to build
the bitfield.

https://codereview.chromium.org/2182123003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java:36:
private PaymentRequestMetrics() {};
No semicolon.

[sebsg, 2016-08-01 16:10:59]

Thanks for the comments!

https://codereview.chromium.org/2182123003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java
(right):

https://codereview.chromium.org/2182123003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java:17:
public static final int REQUESTED_INFORMATION_NONE = 0;
On 2016/08/01 09:20:02, Bernhard Bauer wrote:
> You could define three base values (REQUESTED_INFORMATION_EMAIL = 1 << 0,
> REQUESTED_INFORMATION_PHONE = 1 << 1, REQUESTED_INFORMATION_SHIPPING = 1 << 2)
> and construct the rest from those. Then you could also use them below to build
> the bitfield.

Done.

https://codereview.chromium.org/2182123003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java:36:
private PaymentRequestMetrics() {};
On 2016/08/01 09:20:02, Bernhard Bauer wrote:
> No semicolon.

Done.

[sebsg, 2016-08-01 16:11:05]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2016-08-01 16:11:05]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rouslan@chromium.org, jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2182123003/#ps140001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-08-01 16:11:22]

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-08-01 16:14:03]

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

[commit-bot: I haz the power, 2016-08-01 16:14:05]

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...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sebsg, 2016-08-01 20:50:09]

Patchset #5 (id:160001) has been deleted

[sebsg, 2016-08-02 12:30:22]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2016-08-02 12:30:22]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rouslan@chromium.org, jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2182123003/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-02 12:30:35]

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-08-02 13:47:47]

Description was changed from

==========
Log the different information that the merchant requires to complete a Payment
Request.

BUG=631487
==========

to

==========
Log the different information that the merchant requires to complete a Payment
Request.

BUG=631487
==========

[commit-bot: I haz the power, 2016-08-02 13:47:48]

Committed patchset #5 (id:180001)

[commit-bot: I haz the power, 2016-08-02 13:49:54]

Description was changed from

==========
Log the different information that the merchant requires to complete a Payment
Request.

BUG=631487
==========

to

==========
Log the different information that the merchant requires to complete a Payment
Request.

BUG=631487

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

[commit-bot: I haz the power, 2016-08-02 13:49:56]

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