Detect locally installed native Android payment apps.

Detect locally installed native Android payment apps.

In this patch:

  1) Query locally installed native Android payment apps.
  2) Show the name and icon of these apps in UI.
  3) Invoke the apps with matching payment method data.
  4) Return the results back to the merchant website.

Intent to implement:

  https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/fsslHD1Gf88/K2KpikS6BwAJ

The feature is disabled by default. It can be enabled via a flag:

  chrome://flags/#android-payment-apps

To test the new functionality:

  1) Install BobPay:
     https://drive.google.com/open?id=0B9_TYWUgXNVFLXpRRkthd1lRbjg
  2) Enable chrome://flags/#android-payment-apps.
  3) Open https://rsolomakhin.github.io/pr/bob/.
  4) Click [Buy] button.

BUG=620173
Review-Url: https://codereview.chromium.org/2604193002
Cr-Commit-Position: refs/heads/master@{#442033}
Committed: https://chromium.googlesource.com/chromium/src/+/03f7edd7976c0c96aff58cdb2a85b9cb775168e5

[please use gerrit instead, 2016-12-29 21:40:18]

The CQ bit was checked by rouslan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 21:40:40]

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, 2016-12-29 23:17:13]

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

[commit-bot: I haz the power, 2016-12-29 23:17:14]

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_...)

[please use gerrit instead, 2017-01-03 15:18:15]

The CQ bit was checked by rouslan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 15:18:29]

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

[please use gerrit instead, 2017-01-03 15:42:31]

The CQ bit was checked by rouslan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 15:42:40]

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

[please use gerrit instead, 2017-01-03 15:45:00]

Description was changed from

==========
Detect locally installed native Android payment apps.

In this patch:
1) Query locally installed native Android payment apps.
2) Show the name and icon of these apps in UI.
3) Invoke the apps with matching payment method data.
4) Return the results back to the merchant website.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/fsslHD1Gf88...

The feature is disabled by default. It can be enabled via a flag:
chrome://flags#android-payment-apps

BUG=620173
==========

to

==========
Detect locally installed native Android payment apps.

In this patch:

  1) Query locally installed native Android payment apps.
  2) Show the name and icon of these apps in UI.
  3) Invoke the apps with matching payment method data.
  4) Return the results back to the merchant website.

Intent to implement:

 
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/fsslHD1Gf88...

The feature is disabled by default. It can be enabled via a flag:

  chrome://flags/#android-payment-apps

To test the new functionality:

  1) Install BobPay:
     https://drive.google.com/open?id=0B9_TYWUgXNVFLXpRRkthd1lRbjg
  2) Enable chrome://flags/#android-payment-apps.
  3) Open https://rsolomakhin.github.io/pr/bob/.
  4) Click [Buy] button.

BUG=620173
==========

[please use gerrit instead, 2017-01-03 15:46:47]

rouslan@chromium.org changed reviewers:
+ dfalcantara@chromium.org, isherman@chromium.org

[please use gerrit instead, 2017-01-03 15:46:47]

Dan, ptal android bits.
Ilya, ptal histograms.xml.

[please use gerrit instead, 2017-01-03 15:48:35]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2017-01-03 15:48:41]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-01-03 16:43:40]

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

[commit-bot: I haz the power, 2017-01-03 16:43:41]

Dry run: This issue passed the CQ dry run.

[rwlbuis, 2017-01-03 17:08:28]

rob.buis@samsung.com changed reviewers:
+ rob.buis@samsung.com

[rwlbuis, 2017-01-03 17:08:29]

https://codereview.chromium.org/2604193002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java
(right):

https://codereview.chromium.org/2604193002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:161:
private static void serialziePaymentItem(JsonWriter json, PaymentItem item)
throws IOException {
Typo: serialziePaymentItem.

[please use gerrit instead, 2017-01-03 17:53:05]

The CQ bit was checked by rouslan@chromium.org to run a CQ dry run

[please use gerrit instead, 2017-01-03 17:53:05]

Thank you, Rob!

https://codereview.chromium.org/2604193002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java
(right):

https://codereview.chromium.org/2604193002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:161:
private static void serialziePaymentItem(JsonWriter json, PaymentItem item)
throws IOException {
On 2017/01/03 17:08:29, rwlbuis wrote:
> Typo: serialziePaymentItem.

Done.

[gone, 2017-01-03 19:20:03]

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java
(right):

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
Happy new year!

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:28:
/** A locally installed payment app. */
nit: Maybe "interfaces with a locally installed payment app"?  It's a little
weird to say that this class is an app.

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:31:
private static final String METHOD_NAME_EXTRA = "methodName";
nit: Consider prefixing these with EXTRA_ instead of suffixing them.  It's a
more common pattern in the codebase and groups them together more clearly.

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:55:
public void addMethodName(String methodName) {
Javadocs for public methods.  Findbugs might complain otherwise.

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:96:
return Collections.unmodifiableSet(mMethodNames);
Is this function's return value always going to match getAppMethodNames?  Should
one call the other?

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
(right):

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:27:
private static final String IS_READY_TO_PAY_INTENT_ACTION =
Prefix ACTION instead of suffixing; that's how Intent.ACTION_* is standardized.

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:34:
WindowAndroid window =
ContentViewCore.fromWebContents(webContents).getWindowAndroid();
1) getWindowAndroid can return null, which doesn't seem to be accounted for.

2) It's much safer to cache the WebContents instead of the WindowAndroid to deal
with tab reparenting (e.g.), which results in getWindowAndroid() being null.

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java
(right):

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:115:
context, webContents, methods, new PaymentAppCreatedCallback() {
Indentation would be less wonky if you created the PaymentAppCreatedCallback
outside of the function call.

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:2763: Unable to launch
payment app
Should specify in the description why this doesn't have a period, or add one.  I
don't think snackbars or toast messages are discouraged from having periods,
despite other usages of the WindowAndroid method.

[please use gerrit instead, 2017-01-04 17:18:14]

The CQ bit was checked by rouslan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 17:18:27]

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

[please use gerrit instead, 2017-01-04 17:21:27]

The CQ bit was checked by rouslan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 17:21:43]

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

[please use gerrit instead, 2017-01-04 17:27:29]

Patchset #3 (id:80001) has been deleted

[please use gerrit instead, 2017-01-04 17:27:53]

Dan, ptal patch 3.

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java
(right):

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/03 19:20:03, dfalcantara (check queue) wrote:
> Happy new year!

You too!

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:28:
/** A locally installed payment app. */
On 2017/01/03 19:20:03, dfalcantara (check queue) wrote:
> nit: Maybe "interfaces with a locally installed payment app"?  It's a little
> weird to say that this class is an app.

Done.

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:31:
private static final String METHOD_NAME_EXTRA = "methodName";
On 2017/01/03 19:20:03, dfalcantara (check queue) wrote:
> nit: Consider prefixing these with EXTRA_ instead of suffixing them.  It's a
> more common pattern in the codebase and groups them together more clearly.

Done.

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:55:
public void addMethodName(String methodName) {
On 2017/01/03 19:20:03, dfalcantara (check queue) wrote:
> Javadocs for public methods.  Findbugs might complain otherwise.

Done.

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:96:
return Collections.unmodifiableSet(mMethodNames);
On 2017/01/03 19:20:03, dfalcantara (check queue) wrote:
> Is this function's return value always going to match getAppMethodNames? 
Should
> one call the other?

Done.

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
(right):

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:27:
private static final String IS_READY_TO_PAY_INTENT_ACTION =
On 2017/01/03 19:20:03, dfalcantara (check queue) wrote:
> Prefix ACTION instead of suffixing; that's how Intent.ACTION_* is
standardized.

Done.

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:34:
WindowAndroid window =
ContentViewCore.fromWebContents(webContents).getWindowAndroid();
On 2017/01/03 19:20:03, dfalcantara (check queue) wrote:
> 1) getWindowAndroid can return null, which doesn't seem to be accounted for.

Checking getWindowAndroid() for null now. Thank you.

Can ContentViewCore.fromWebContents(webContents) return null?

> 2) It's much safer to cache the WebContents instead of the WindowAndroid to
deal
> with tab reparenting (e.g.), which results in getWindowAndroid() being null.

Caching WebContents instead.

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java
(right):

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:115:
context, webContents, methods, new PaymentAppCreatedCallback() {
On 2017/01/03 19:20:03, dfalcantara (check queue) wrote:
> Indentation would be less wonky if you created the PaymentAppCreatedCallback
> outside of the function call.

Done.

Not super happy about having more than one callback reference of the same type
in the function, but can't think of a better way to do it.

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:2763: Unable to launch
payment app
On 2017/01/03 19:20:03, dfalcantara (check queue) wrote:
> Should specify in the description why this doesn't have a period, or add one. 
I
> don't think snackbars or toast messages are discouraged from having periods,
> despite other usages of the WindowAndroid method.

Done.

[commit-bot: I haz the power, 2017-01-04 18:33:57]

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

[commit-bot: I haz the power, 2017-01-04 18:33:58]

Dry run: This issue passed the CQ dry run.

[gone, 2017-01-04 19:40:57]

lgtm % adding that null check

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
(right):

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:34:
WindowAndroid window =
ContentViewCore.fromWebContents(webContents).getWindowAndroid();
On 2017/01/04 17:27:53, rouslan wrote:
> Can ContentViewCore.fromWebContents(webContents) return null?

Javadoc suggests so, yeah.  Should probably null check that where you moved it
to.

[please use gerrit instead, 2017-01-04 19:58:53]

The CQ bit was checked by rouslan@chromium.org to run a CQ dry run

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

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

[please use gerrit instead, 2017-01-04 20:02:51]

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
(right):

https://codereview.chromium.org/2604193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:34:
WindowAndroid window =
ContentViewCore.fromWebContents(webContents).getWindowAndroid();
On 2017/01/04 19:40:57, dfalcantara (check queue) wrote:
> On 2017/01/04 17:27:53, rouslan wrote:
> > Can ContentViewCore.fromWebContents(webContents) return null?
> 
> Javadoc suggests so, yeah.  Should probably null check that where you moved it
> to.

Done.

[commit-bot: I haz the power, 2017-01-04 20:57:43]

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

[commit-bot: I haz the power, 2017-01-04 20:57:44]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2017-01-06 17:31:50]

rouslan@chromium.org changed reviewers:
+ rkaplow@chromium.org
- isherman@chromium.org, rob.buis@samsung.com

[please use gerrit instead, 2017-01-06 17:31:51]

rkaplow, owners ptal histograms.xml

[rkaplow, 2017-01-06 19:07:13]

lgtm

[please use gerrit instead, 2017-01-06 19:10:33]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2017-01-06 19:10:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2604193002/#ps120001
(title: "null-check")

[commit-bot: I haz the power, 2017-01-06 19:11:16]

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-01-06 20:36:26]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483729833721500,
"parent_rev": "f09d5dbfa4a7bc684693e5888071eec5a7ea2414", "commit_rev":
"03f7edd7976c0c96aff58cdb2a85b9cb775168e5"}

[commit-bot: I haz the power, 2017-01-06 20:36:57]

Description was changed from

==========
Detect locally installed native Android payment apps.

In this patch:

  1) Query locally installed native Android payment apps.
  2) Show the name and icon of these apps in UI.
  3) Invoke the apps with matching payment method data.
  4) Return the results back to the merchant website.

Intent to implement:

 
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/fsslHD1Gf88...

The feature is disabled by default. It can be enabled via a flag:

  chrome://flags/#android-payment-apps

To test the new functionality:

  1) Install BobPay:
     https://drive.google.com/open?id=0B9_TYWUgXNVFLXpRRkthd1lRbjg
  2) Enable chrome://flags/#android-payment-apps.
  3) Open https://rsolomakhin.github.io/pr/bob/.
  4) Click [Buy] button.

BUG=620173
==========

to

==========
Detect locally installed native Android payment apps.

In this patch:

  1) Query locally installed native Android payment apps.
  2) Show the name and icon of these apps in UI.
  3) Invoke the apps with matching payment method data.
  4) Return the results back to the merchant website.

Intent to implement:

 
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/fsslHD1Gf88...

The feature is disabled by default. It can be enabled via a flag:

  chrome://flags/#android-payment-apps

To test the new functionality:

  1) Install BobPay:
     https://drive.google.com/open?id=0B9_TYWUgXNVFLXpRRkthd1lRbjg
  2) Enable chrome://flags/#android-payment-apps.
  3) Open https://rsolomakhin.github.io/pr/bob/.
  4) Click [Buy] button.

BUG=620173

Review-Url: https://codereview.chromium.org/2604193002
Cr-Commit-Position: refs/heads/master@{#442033}
Committed:
https://chromium.googlesource.com/chromium/src/+/03f7edd7976c0c96aff58cdb2a85...
==========

[commit-bot: I haz the power, 2017-01-06 20:36:59]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/03f7edd7976c0c96aff58cdb2a85...