Download web payment manifests.

Download web payment manifests.

The browser downloads web payment manifests and parses them in a utility
process. Then browser verifies the signatures of the installed payment
apps and shows only the apps with the matching SHA256 certificate
fingerprints to the user.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE

Flag (disabled by default): chrome://flags/#android-payment-apps

To understand how the feature works, let's see what happens for a
'https://bobpay.com' payment method. When the JavaScript PaymentRequest
object is instantiated, the browser first scans installed applications
for any app that can handle the 'https://bobpay.com' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:
  Link: <https://bobpay.com/payment-manifest.json>; rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:
  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:AB:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed Android app
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

BUG=683329
Review-Url: https://codereview.chromium.org/2645813006
Cr-Commit-Position: refs/heads/master@{#456934}
Committed: https://chromium.googlesource.com/chromium/src/+/8fdbfb2484873d09de132e051da87e580382f95b

[please use gerrit instead, 2017-01-20 20:30:19]

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

[commit-bot: I haz the power, 2017-01-20 20:30:46]

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-20 20:32:33]

Description was changed from

==========
Download web payment manifests.

Browser downloads web payment manifests and parses them in the renderer.
Then browser verifies the signature of the installed payment apps and
shows only the apps with the valid SHA256 certificate fingerprints to
the user.

Third-party native Android payment app integration is behind a flag:

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

To test:

1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE

2) Enable chrome://flags/#android-payment-apps.

3) Navigate to https://rsolomakhin.github.io/pr/bob/.

4) Click "Buy" button on the webpage.

This test webpage uses 'https://emerald-eon.appspot.com/bobpay' payment
method. Browser uses HTTP HEAD request for this URL to read the HTTP
Link header:

  Link: <https://rsolomakhin.github.io/bobpay/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents:

  {
    "android": [{
      "package": "com.bobpay",
      "version": 1,
      "sha256_cert_fingerprints": ["308201dd308201460201013..."]
    }]
  }

Finally, browser scans installed applications and matches the installed
BobPay.apk with 'https://emerald-eon.appspot.com/bobpay' payment method. Thus,
the owner of emerald-eon.appspot.com controls which payment apps can use this
URL as a payment method identifier.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

BUG=683329
==========

to

==========
Download web payment manifests.

Browser downloads web payment manifests and parses them in the browser
process. (Renderer process may not be available when the list of payment
apps is being displayed in Settings.) Then browser verifies the
signatures of the installed payment apps and shows only the apps with
the matching SHA256 certificate fingerprints to the user.

Third-party native Android payment app integration is behind a flag:

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

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

This test webpage uses 'https://emerald-eon.appspot.com/bobpay' payment
method. Browser uses HTTP HEAD request for this URL to read the HTTP
Link header:

  Link: <https://rsolomakhin.github.io/bobpay/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents:

  {
    "android": [{
      "package": "com.bobpay",
      "version": 1,
      "sha256_cert_fingerprints": ["308201dd308201460201013..."]
    }]
  }

Finally, browser scans installed applications and matches the installed
BobPay.apk with 'https://emerald-eon.appspot.com/bobpay' payment method. Thus,
the owner of emerald-eon.appspot.com controls which payment apps can use this
URL as a payment method identifier.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

BUG=683329
==========

[please use gerrit instead, 2017-01-20 20:34:39]

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

[commit-bot: I haz the power, 2017-01-20 20:35:10]

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-20 20:38:56]

Description was changed from

==========
Download web payment manifests.

Browser downloads web payment manifests and parses them in the browser
process. (Renderer process may not be available when the list of payment
apps is being displayed in Settings.) Then browser verifies the
signatures of the installed payment apps and shows only the apps with
the matching SHA256 certificate fingerprints to the user.

Third-party native Android payment app integration is behind a flag:

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

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

This test webpage uses 'https://emerald-eon.appspot.com/bobpay' payment
method. Browser uses HTTP HEAD request for this URL to read the HTTP
Link header:

  Link: <https://rsolomakhin.github.io/bobpay/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents:

  {
    "android": [{
      "package": "com.bobpay",
      "version": 1,
      "sha256_cert_fingerprints": ["308201dd308201460201013..."]
    }]
  }

Finally, browser scans installed applications and matches the installed
BobPay.apk with 'https://emerald-eon.appspot.com/bobpay' payment method. Thus,
the owner of emerald-eon.appspot.com controls which payment apps can use this
URL as a payment method identifier.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

BUG=683329
==========

to

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in the
browser process. (A renderer process may not be available when the list
of payment apps is being displayed in Settings.) Then browser verifies
the signatures of the installed payment apps and shows only the apps
with the matching SHA256 certificate fingerprints to the user.

Third-party native Android payment app integration is behind a flag:

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

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

This test webpage uses 'https://emerald-eon.appspot.com/bobpay' payment
method. When PaymentRequest object is instantiated, the browser first
scans installed applications for any that can handle this URL with
'org.chromium.intent.action.PAY' intent. If any application match is
found, then the browser uses HTTP HEAD request for this URL to read the
HTTP Link header:

  Link: <https://rsolomakhin.github.io/bobpay/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents:

  {
    "android": [{
      "package": "com.bobpay",
      "version": 1,
      "sha256_cert_fingerprints": ["308201dd308201460201013..."]
    }]
  }

Finally, browser compares the properties of the installed BobPay.apk
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier. (Caching will be implemented in a future patch.)

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

BUG=683329
==========

[please use gerrit instead, 2017-01-20 20:49:55]

rouslan@chromium.org changed reviewers:
+ gogerald@chromium.org, palmer@chromium.org, tedchoc@chromium.org,
xunjieli@chromium.org

[please use gerrit instead, 2017-01-20 20:49:55]

Ganggui, ptal PaymentAppFinder.find() in AndroidPaymentAppFactory.java. Please
make sure I did not regress the code that you so carefully crafted in there.

Chris, ptal ManifestParser.java. Unfortunately, I need to parse JSON in the
browser process. :-( Parser tests are upcoming. Also, ptal signature
verification in PaymentManifestVerifier.onManifestParseSuccess(manifests). I'm
checking all of the signatures of the installed APK, but ignoring the order.

Ted, ptal the overall design and querying of installed native Android apps. Do
you know whether Signature.toCharsString() is the SHA256 certificate
fingerprint? It seems way longer than I expected. Also, is there a good way to
mock installed native Android apps for integration testing?

Helen, ptal networking in ManifestDownloader.java. What's a good way to mock 
networking in Java for integration testing?

[commit-bot: I haz the power, 2017-01-20 21:45:03]

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

[commit-bot: I haz the power, 2017-01-20 21:45:04]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2017-01-20 21:51:08]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2017-01-20 22:04:57]

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

[commit-bot: I haz the power, 2017-01-20 22:05:31]

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-01-20 23:08:51]

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

[commit-bot: I haz the power, 2017-01-20 23:08:51]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-01-21 00:02:10]

On 2017/01/20 20:49:55, rouslan wrote:
> Ganggui, ptal PaymentAppFinder.find() in AndroidPaymentAppFactory.java. Please
> make sure I did not regress the code that you so carefully crafted in there.
> 
> Chris, ptal ManifestParser.java. Unfortunately, I need to parse JSON in the
> browser process. :-( Parser tests are upcoming. Also, ptal signature
> verification in PaymentManifestVerifier.onManifestParseSuccess(manifests). I'm
> checking all of the signatures of the installed APK, but ignoring the order.
> 
> Ted, ptal the overall design and querying of installed native Android apps. Do
> you know whether Signature.toCharsString() is the SHA256 certificate
> fingerprint? It seems way longer than I expected.

Hmm...no idea about that one sadly.

Also, is there a good way to
> mock installed native Android apps for integration testing?

The likely best way to mock apps is to extract a delegate or something that
provides an abstraction over packagemanager and context to give fake data.

Otherwise, you can look at ContextWrapper to try and achieve what you want,
but I suspect you'll find that breaks pretty down quickly.

> 
> Helen, ptal networking in ManifestDownloader.java. What's a good way to mock 
> networking in Java for integration testing?

FWIW, we really try to avoid using the Android net stack.  Is there any
reason that you're not using Chrome's?  To issue http requests, there is 
no requirement on a renderer being present (just native to be loaded).

That is my biggest first glance comment.  If this doesn't need to happen
pre-native (and if so, a big explanation of why), then it needs to move
to the chrome net stack.

[xunjieli, 2017-01-23 16:20:26]

On 2017/01/21 00:02:10, Ted C wrote:
> On 2017/01/20 20:49:55, rouslan wrote:
> > Ganggui, ptal PaymentAppFinder.find() in AndroidPaymentAppFactory.java.
Please
> > make sure I did not regress the code that you so carefully crafted in there.
> > 
> > Chris, ptal ManifestParser.java. Unfortunately, I need to parse JSON in the
> > browser process. :-( Parser tests are upcoming. Also, ptal signature
> > verification in PaymentManifestVerifier.onManifestParseSuccess(manifests).
I'm
> > checking all of the signatures of the installed APK, but ignoring the order.
> > 
> > Ted, ptal the overall design and querying of installed native Android apps.
Do
> > you know whether Signature.toCharsString() is the SHA256 certificate
> > fingerprint? It seems way longer than I expected.
> 
> Hmm...no idea about that one sadly.
> 
> Also, is there a good way to
> > mock installed native Android apps for integration testing?
> 
> The likely best way to mock apps is to extract a delegate or something that
> provides an abstraction over packagemanager and context to give fake data.
> 
> Otherwise, you can look at ContextWrapper to try and achieve what you want,
> but I suspect you'll find that breaks pretty down quickly.
> 
> > 
> > Helen, ptal networking in ManifestDownloader.java. What's a good way to mock

> > networking in Java for integration testing?
> 
> FWIW, we really try to avoid using the Android net stack.  Is there any
> reason that you're not using Chrome's?  To issue http requests, there is 
> no requirement on a renderer being present (just native to be loaded).
> 
> That is my biggest first glance comment.  If this doesn't need to happen
> pre-native (and if so, a big explanation of why), then it needs to move
> to the chrome net stack.

Sorry that I have led you on the wrong path. I somehow thought that you were
considering Cronet (a Java library that apps can include) vs HttpURLConnection
(what Android offers as a built-in networking API). If this in Chrome, going
through the Chrome network stack is the recommended way. Chrome network stack
doesn't have any Java hooks. If you can move the download part to the native
side, you can consider using the net::URLRequest API. Parsing and checking
signatures probably don't need to be on the Java side once you move download to
the native side.

[gogerald1, 2017-01-23 17:18:35]

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

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:53:
* The basic-card payment method name used by merchant and defined by W3C:
one more space for indentation

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:91:
mPendingApps.put(method, null);
nit: It looks a little more clear to me if we consider BASIC_CARD_PAYMENT_METHOD
completely separately (Do not put it to mPendingApps).

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:104:
if (!apps.isEmpty()) {
nits: positive logic first?

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:118:
if (apps.isEmpty()) continue;
record and remove these unsupported methods from mPendingApps later?

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:199:
new Intent(ACTION_IS_READY_TO_PAY), 0);
Could we do this when creating AndroidPaymentApp so as to be a little more
efficient. You can use resolveActivity with known package name at there.

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:205:
mCallback.onPaymentAppCreated(app);
Is this means that the app must support ACTION_IS_READY_TO_PAY intent action? I
remember is optional in spec.

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:280:
if ("*".equals(manifest.packageName)) {
add comments to explain * means all payment apps are allowed?

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

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ManifestParser.java:119:
if ("*".equals(manifest.packageName)) break;
Do you need result.add(manifest) here, otherwise result.isEmpty() could be true.

[please use gerrit instead, 2017-01-23 22:31:36]

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

[please use gerrit instead, 2017-01-23 22:32:43]

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

[commit-bot: I haz the power, 2017-01-23 22:32: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-23 22:33:57]

Patchset #3 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-01-23 23:48:33]

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

[commit-bot: I haz the power, 2017-01-23 23:48:34]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2017-01-25 20:59:13]

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

[commit-bot: I haz the power, 2017-01-25 21:00:02]

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-01-25 21:02:21]

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

[commit-bot: I haz the power, 2017-01-25 21:02:22]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[please use gerrit instead, 2017-01-26 19:50:08]

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

[commit-bot: I haz the power, 2017-01-26 19:50:58]

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

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

[commit-bot: I haz the power, 2017-01-26 20:01:49]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[please use gerrit instead, 2017-01-26 21:09:27]

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

[commit-bot: I haz the power, 2017-01-26 21:09:53]

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-01-26 21:17:38]

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

[commit-bot: I haz the power, 2017-01-26 21:17:39]

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

[palmer, 2017-01-27 00:11:47]

https://codereview.chromium.org/2645813006/diff/180001/components/payments/an...
File components/payments/android/payment_manifest_downloader_android.cc (right):

https://codereview.chromium.org/2645813006/diff/180001/components/payments/an...
components/payments/android/payment_manifest_downloader_android.cc:86:
base::android::ConvertJavaStringToUTF8(env, jmethod_name), delegate);
I think here might be a better place to convert the |method_name| string to a
URL? If possible, though, it would be best to declare it as (some kind of) URL
all up and down the Java/native stack. Is it possible? My goal is for bad inputs
to be rejected as early as possible.

https://codereview.chromium.org/2645813006/diff/180001/components/payments/pa...
File components/payments/payment_manifest_downloader.cc (right):

https://codereview.chromium.org/2645813006/diff/180001/components/payments/pa...
components/payments/payment_manifest_downloader.cc:37: fetcher_ =
net::URLFetcher::Create(0 /* id */, GURL(url), request_type, this);
I'd like to see a single point of validation, which validates at least the
following thingies:

* is it really a URL at all
* is it HTTPS
* is it to 1 of the handful of whitelisted hosts we trust payment manifests from

I have not yet read all the code in this CL; is there such a centralized
validation point?

https://codereview.chromium.org/2645813006/diff/180001/components/payments/pa...
components/payments/payment_manifest_downloader.cc:48: if
(source->GetResponseCode() != 200) {
Have 3xx redirects already been followed, at this point? (Apologies if so;
URLFetcher seems to depend on SetStopOnRedirect to learn what it should do; it
might be a good idea to set it explicitly just so you know for sure. Although
keep in mind that any URL validation requirements should apply to the new
redirected locations, too.)

[please use gerrit instead, 2017-01-27 21:38:34]

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

[commit-bot: I haz the power, 2017-01-27 21:39:19]

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-01-27 21:46:42]

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

[commit-bot: I haz the power, 2017-01-27 21:46:43]

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

[please use gerrit instead, 2017-01-28 01:00:47]

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

[commit-bot: I haz the power, 2017-01-28 01:01: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-01-28 01:08:51]

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

[commit-bot: I haz the power, 2017-01-28 01:08:52]

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

[please use gerrit instead, 2017-01-30 18:12:56]

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

[commit-bot: I haz the power, 2017-01-30 18:13:26]

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-01-30 18:21:03]

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

[commit-bot: I haz the power, 2017-01-30 18:21:04]

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

[please use gerrit instead, 2017-02-01 23:51:28]

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

[commit-bot: I haz the power, 2017-02-01 23:51:59]

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-02-01 23:57:34]

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

[commit-bot: I haz the power, 2017-02-01 23:57:35]

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

[please use gerrit instead, 2017-02-02 23:06:23]

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

[commit-bot: I haz the power, 2017-02-02 23:06: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-02-02 23:13:38]

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

[commit-bot: I haz the power, 2017-02-02 23:13:39]

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

[please use gerrit instead, 2017-02-02 23:26:33]

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

[commit-bot: I haz the power, 2017-02-02 23:27:10]

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-02-02 23:35:00]

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

[commit-bot: I haz the power, 2017-02-02 23:35:02]

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

[please use gerrit instead, 2017-02-14 19:40:29]

Patchset #3 (id:80001) has been deleted

[please use gerrit instead, 2017-02-14 19:41:01]

Patchset #3 (id:100001) has been deleted

[please use gerrit instead, 2017-02-14 19:45:04]

Patchset #3 (id:120001) has been deleted

[please use gerrit instead, 2017-02-14 19:45:20]

Patchset #3 (id:140001) has been deleted

[please use gerrit instead, 2017-02-14 19:45:44]

Patchset #3 (id:160001) has been deleted

[please use gerrit instead, 2017-02-14 19:46:13]

Patchset #4 (id:200001) has been deleted

[please use gerrit instead, 2017-02-14 19:46:50]

Patchset #4 (id:220001) has been deleted

[please use gerrit instead, 2017-02-14 19:47:03]

Patchset #4 (id:240001) has been deleted

[please use gerrit instead, 2017-02-14 19:47:45]

Patchset #4 (id:260001) has been deleted

[please use gerrit instead, 2017-02-14 19:47:57]

Patchset #4 (id:280001) has been deleted

[please use gerrit instead, 2017-02-14 19:48:10]

Patchset #4 (id:300001) has been deleted

[please use gerrit instead, 2017-02-14 19:48:23]

Patchset #4 (id:320001) has been deleted

[please use gerrit instead, 2017-02-14 19:49:44]

Description was changed from

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in the
browser process. (A renderer process may not be available when the list
of payment apps is being displayed in Settings.) Then browser verifies
the signatures of the installed payment apps and shows only the apps
with the matching SHA256 certificate fingerprints to the user.

Third-party native Android payment app integration is behind a flag:

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

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

This test webpage uses 'https://emerald-eon.appspot.com/bobpay' payment
method. When PaymentRequest object is instantiated, the browser first
scans installed applications for any that can handle this URL with
'org.chromium.intent.action.PAY' intent. If any application match is
found, then the browser uses HTTP HEAD request for this URL to read the
HTTP Link header:

  Link: <https://rsolomakhin.github.io/bobpay/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents:

  {
    "android": [{
      "package": "com.bobpay",
      "version": 1,
      "sha256_cert_fingerprints": ["308201dd308201460201013..."]
    }]
  }

Finally, browser compares the properties of the installed BobPay.apk
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier. (Caching will be implemented in a future patch.)

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

BUG=683329
==========

to

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in the
browser process. (A renderer process may not be available when the list
of payment apps is being displayed in Settings.) Then browser verifies
the signatures of the installed payment apps and shows only the apps
with the matching SHA256 certificate fingerprints to the user.

Third-party native Android payment app integration is behind a flag:

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

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

This test webpage uses 'https://emerald-eon.appspot.com/bobpay' payment
method. When PaymentRequest object is instantiated, the browser first
scans installed applications for any that can handle this URL with
'org.chromium.intent.action.PAY' intent. If any application match is
found, then the browser uses HTTP HEAD request for this URL to read the
HTTP Link header:

  Link: <https://rsolomakhin.github.io/bobpay/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents:

  {
    "android": [{
      "package": "com.bobpay",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:dd:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed BobPay.apk
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier. (Caching will be implemented in a future patch.)

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

BUG=683329
==========

[please use gerrit instead, 2017-02-14 19:50:25]

Description was changed from

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in the
browser process. (A renderer process may not be available when the list
of payment apps is being displayed in Settings.) Then browser verifies
the signatures of the installed payment apps and shows only the apps
with the matching SHA256 certificate fingerprints to the user.

Third-party native Android payment app integration is behind a flag:

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

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

This test webpage uses 'https://emerald-eon.appspot.com/bobpay' payment
method. When PaymentRequest object is instantiated, the browser first
scans installed applications for any that can handle this URL with
'org.chromium.intent.action.PAY' intent. If any application match is
found, then the browser uses HTTP HEAD request for this URL to read the
HTTP Link header:

  Link: <https://rsolomakhin.github.io/bobpay/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents:

  {
    "android": [{
      "package": "com.bobpay",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:dd:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed BobPay.apk
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier. (Caching will be implemented in a future patch.)

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

BUG=683329
==========

to

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in the
browser process. (A renderer process may not be available when the list
of payment apps is being displayed in Settings.) Then browser verifies
the signatures of the installed payment apps and shows only the apps
with the matching SHA256 certificate fingerprints to the user.

Third-party native Android payment app integration is behind a flag:

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

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

This test webpage uses 'https://emerald-eon.appspot.com/bobpay' payment
method. When PaymentRequest object is instantiated, the browser first
scans installed applications for any that can handle this URL with
'org.chromium.intent.action.PAY' intent. If any application match is
found, then the browser uses HTTP HEAD request for this URL to read the
HTTP Link header:

  Link: <https://rsolomakhin.github.io/bobpay/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents:

  {
    "android": [{
      "package": "com.bobpay",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:dd:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed BobPay.apk
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

BUG=683329
==========

[please use gerrit instead, 2017-02-14 19:50:39]

Description was changed from

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in the
browser process. (A renderer process may not be available when the list
of payment apps is being displayed in Settings.) Then browser verifies
the signatures of the installed payment apps and shows only the apps
with the matching SHA256 certificate fingerprints to the user.

Third-party native Android payment app integration is behind a flag:

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

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

This test webpage uses 'https://emerald-eon.appspot.com/bobpay' payment
method. When PaymentRequest object is instantiated, the browser first
scans installed applications for any that can handle this URL with
'org.chromium.intent.action.PAY' intent. If any application match is
found, then the browser uses HTTP HEAD request for this URL to read the
HTTP Link header:

  Link: <https://rsolomakhin.github.io/bobpay/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents:

  {
    "android": [{
      "package": "com.bobpay",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:dd:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed BobPay.apk
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

BUG=683329
==========

to

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in the
browser process. (A renderer process may not be available when the list
of payment apps is being displayed in Settings.) Then browser verifies
the signatures of the installed payment apps and shows only the apps
with the matching SHA256 certificate fingerprints to the user.

Third-party native Android payment app integration is behind a flag:

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

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

This test webpage uses 'https://emerald-eon.appspot.com/bobpay' payment
method. When PaymentRequest object is instantiated, the browser first
scans installed applications for any that can handle this URL with
'org.chromium.intent.action.PAY' intent. If any application match is
found, then the browser uses HTTP HEAD request for this URL to read the
HTTP Link header:

  Link: <https://rsolomakhin.github.io/bobpay/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents:

  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:dd:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed BobPay.apk
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

BUG=683329
==========

[please use gerrit instead, 2017-02-16 21:56:30]

Patchset #4 (id:340001) has been deleted

[please use gerrit instead, 2017-02-22 21:05:19]

Patchset #4 (id:360001) has been deleted

[please use gerrit instead, 2017-02-22 22:49:19]

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

[commit-bot: I haz the power, 2017-02-22 22:50:32]

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-02-22 22:54:47]

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

[commit-bot: I haz the power, 2017-02-22 22:54:48]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[please use gerrit instead, 2017-02-23 15:54:14]

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

[commit-bot: I haz the power, 2017-02-23 15:54:34]

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-02-23 16:02:54]

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

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

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 on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[please use gerrit instead, 2017-02-23 16:08:52]

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

[commit-bot: I haz the power, 2017-02-23 16:09:24]

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-02-23 16:10:29]

Patchset #4 (id:380001) has been deleted

[please use gerrit instead, 2017-02-23 16:10:47]

Patchset #4 (id:400001) has been deleted

[please use gerrit instead, 2017-02-23 16:11:12]

Patchset #4 (id:420001) has been deleted

[commit-bot: I haz the power, 2017-02-23 16:16:50]

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

[commit-bot: I haz the power, 2017-02-23 16:16:51]

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

[please use gerrit instead, 2017-02-23 16:30:32]

Description was changed from

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in the
browser process. (A renderer process may not be available when the list
of payment apps is being displayed in Settings.) Then browser verifies
the signatures of the installed payment apps and shows only the apps
with the matching SHA256 certificate fingerprints to the user.

Third-party native Android payment app integration is behind a flag:

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

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

This test webpage uses 'https://emerald-eon.appspot.com/bobpay' payment
method. When PaymentRequest object is instantiated, the browser first
scans installed applications for any that can handle this URL with
'org.chromium.intent.action.PAY' intent. If any application match is
found, then the browser uses HTTP HEAD request for this URL to read the
HTTP Link header:

  Link: <https://rsolomakhin.github.io/bobpay/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents:

  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:dd:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed BobPay.apk
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

BUG=683329
==========

to

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in the
renderer process. Then browser verifies the signatures of the installed
payment apps and shows only the apps with the matching SHA256
certificate fingerprints to the user.

Third-party native Android payment app integration is behind a flag:

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

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

This test webpage uses 'https://emerald-eon.appspot.com/bobpay' payment
method name. When the JavaScript PaymentRequest object is instantiated,
the browser first scans installed applications for any that can handle
the 'https://emerald-eon.appspot.com/bobpay' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:

  Link: <https://rsolomakhin.github.io/bobpay/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:

  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:dd:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed BobPay.apk
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

BUG=683329
==========

[sdefresne, 2017-02-23 17:11:04]

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

[sdefresne, 2017-02-23 17:11:05]

https://codereview.chromium.org/2645813006/diff/440001/components/payments/BU...
File components/payments/BUILD.gn (right):

https://codereview.chromium.org/2645813006/diff/440001/components/payments/BU...
components/payments/BUILD.gn:98: "//content/test:test_support",
I think the iOS build failures are due to this line.

When gn parse a BUILD.gn file, all targets are evaluated and their dependencies
loaded, even if the target itself is never used on iOS. However, loading
//content is not allowed on iOS, so this cause the failure.

As //components/payments is build and used on iOS, it cannot depends on
//content. You should try to remove this dependency on //content. One option is
to re-implement the helper you use from //content. Another is to turn your
component into a layered component
(https://www.chromium.org/developers/design-documents/layered-components-design).

https://codereview.chromium.org/2645813006/diff/440001/components/payments/DEPS
File components/payments/DEPS (left):

https://codereview.chromium.org/2645813006/diff/440001/components/payments/DE...
components/payments/DEPS:3: # TODO(crbug.com/679381): Move this to
components/payments/content.
This component is used on iOS so cannot depends on //content so I think you need
to address this bug before landing your CL.

[please use gerrit instead, 2017-02-23 18:31:10]

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

[commit-bot: I haz the power, 2017-02-23 18:32:25]

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-02-23 18:41:20]

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

[commit-bot: I haz the power, 2017-02-23 18:41:22]

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

[please use gerrit instead, 2017-02-23 19:55:14]

Patchset #5 (id:460001) has been deleted

[please use gerrit instead, 2017-02-23 19:55:40]

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

[commit-bot: I haz the power, 2017-02-23 19:56:24]

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-02-23 19:57:48]

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

[please use gerrit instead, 2017-02-23 19:57:51]

Reviewers, please take a look at patch 5. This patch has grown from 300 to 1300
lines of code. I personally prefer to review features as a whole, so I see the
large picture. However, I can split out individual changes into smaller patches,
if you like that more. Please let me know your preference.

The major updates since the last iteration are:

 1) JSON parsing has moved to the renderer process.
 2) URL validation happens as soon as possible.
 3) URL object is used all up and down the Java and native stacks.
 4) HTTP 300 redirects are prohibited.
 5) Download has moved to Chrome network stack.
 6) Both downloader and JSON parser have extensive tests.

The individual areas for review are:

GANGGUI, ptal AndroidPaymentAppFactory.java and PaymentManifestVerifier.java.

CHRIS, ptal mojom and overall security.
 * URL validation happens in AndroidPaymentAppFactory.java.
 * File download happens in PaymentManifestDownloader.java,
payment_manifest_downloader_android.{h,cc}, and
payment_manifest_downloader.{h,cc}.
 * Manifest parsing happens in PaymentRequest.cpp.

TED, ptal the overall design of Java. I've figured out that I need to take a
SHA-256 hash of Android's signature to get the fingerprint as per the design.

HELEN, ptal networking in payment_manifest_downloader.cc.

+MARIJN, ptal WebKit. I have to use ScriptState::forMainWorld() in
PaymentRequest::ParsePaymentManifest(), because that's called from the browser
process. Is that an OK use of the main world?

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

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:53:
* The basic-card payment method name used by merchant and defined by W3C:
On 2017/01/23 17:18:35, gogerald1 wrote:
> one more space for indentation

Done.

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:91:
mPendingApps.put(method, null);
On 2017/01/23 17:18:35, gogerald1 wrote:
> nit: It looks a little more clear to me if we consider
BASIC_CARD_PAYMENT_METHOD
> completely separately (Do not put it to mPendingApps).

Done.

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:104:
if (!apps.isEmpty()) {
On 2017/01/23 17:18:35, gogerald1 wrote:
> nits: positive logic first?

Done.

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:118:
if (apps.isEmpty()) continue;
On 2017/01/23 17:18:34, gogerald1 wrote:
> record and remove these unsupported methods from mPendingApps later?

I don't see the advantage of this. Please explain.

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:199:
new Intent(ACTION_IS_READY_TO_PAY), 0);
On 2017/01/23 17:18:34, gogerald1 wrote:
> Could we do this when creating AndroidPaymentApp so as to be a little more
> efficient. You can use resolveActivity with known package name at there.

I don't see how that would improve efficiency. Please explain.

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:205:
mCallback.onPaymentAppCreated(app);
On 2017/01/23 17:18:34, gogerald1 wrote:
> Is this means that the app must support ACTION_IS_READY_TO_PAY intent action?
I
> remember is optional in spec.

Fixed. It should be optional.

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:280:
if ("*".equals(manifest.packageName)) {
On 2017/01/23 17:18:35, gogerald1 wrote:
> add comments to explain * means all payment apps are allowed?

Done.

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

https://codereview.chromium.org/2645813006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ManifestParser.java:119:
if ("*".equals(manifest.packageName)) break;
On 2017/01/23 17:18:35, gogerald1 wrote:
> Do you need result.add(manifest) here, otherwise result.isEmpty() could be
true.

Done. Great catch!

https://codereview.chromium.org/2645813006/diff/180001/components/payments/an...
File components/payments/android/payment_manifest_downloader_android.cc (right):

https://codereview.chromium.org/2645813006/diff/180001/components/payments/an...
components/payments/android/payment_manifest_downloader_android.cc:86:
base::android::ConvertJavaStringToUTF8(env, jmethod_name), delegate);
On 2017/01/27 00:11:47, palmer wrote:
> I think here might be a better place to convert the |method_name| string to a
> URL? If possible, though, it would be best to declare it as (some kind of) URL
> all up and down the Java/native stack. Is it possible? My goal is for bad
inputs
> to be rejected as early as possible.

The code now uses a URL all up and down the Java/native stack. The URL parsing
and validation happens in Java as the first step in payment app search.

https://codereview.chromium.org/2645813006/diff/180001/components/payments/pa...
File components/payments/payment_manifest_downloader.cc (right):

https://codereview.chromium.org/2645813006/diff/180001/components/payments/pa...
components/payments/payment_manifest_downloader.cc:37: fetcher_ =
net::URLFetcher::Create(0 /* id */, GURL(url), request_type, this);
On 2017/01/27 00:11:47, palmer wrote:
> I'd like to see a single point of validation, which validates at least the
> following thingies:
> 
> * is it really a URL at all

AndroidPaymentAppFactory.java:115

> * is it HTTPS

AndroidPaymentAppFactory.java:112

> * is it to 1 of the handful of whitelisted hosts we trust payment manifests
from

I've moved the parser into the renderer with extensive unit test coverage. Is
this sufficient to alleviate your concerns? Standards bodies generally frown
upon whitelisting.

https://codereview.chromium.org/2645813006/diff/180001/components/payments/pa...
components/payments/payment_manifest_downloader.cc:48: if
(source->GetResponseCode() != 200) {
On 2017/01/27 00:11:47, palmer wrote:
> Have 3xx redirects already been followed, at this point? (Apologies if so;
> URLFetcher seems to depend on SetStopOnRedirect to learn what it should do; it
> might be a good idea to set it explicitly just so you know for sure. Although
> keep in mind that any URL validation requirements should apply to the new
> redirected locations, too.)

I've prohibited 3xx redirects in the latest patch. See
payment_manifest_downloader.cc:58.

https://codereview.chromium.org/2645813006/diff/440001/components/payments/BU...
File components/payments/BUILD.gn (right):

https://codereview.chromium.org/2645813006/diff/440001/components/payments/BU...
components/payments/BUILD.gn:98: "//content/test:test_support",
On 2017/02/23 17:11:05, sdefresne wrote:
> I think the iOS build failures are due to this line.
> 
> When gn parse a BUILD.gn file, all targets are evaluated and their
dependencies
> loaded, even if the target itself is never used on iOS. However, loading
> //content is not allowed on iOS, so this cause the failure.
> 
> As //components/payments is build and used on iOS, it cannot depends on
> //content. You should try to remove this dependency on //content. One option
is
> to re-implement the helper you use from //content. Another is to turn your
> component into a layered component
>
(https://www.chromium.org/developers/design-documents/layered-components-design).
> 

Started on splitting up the component into layers in a separate patch, which
will need to land first.

https://codereview.chromium.org/2645813006/diff/440001/components/payments/DEPS
File components/payments/DEPS (left):

https://codereview.chromium.org/2645813006/diff/440001/components/payments/DE...
components/payments/DEPS:3: # TODO(crbug.com/679381): Move this to
components/payments/content.
On 2017/02/23 17:11:05, sdefresne wrote:
> This component is used on iOS so cannot depends on //content so I think you
need
> to address this bug before landing your CL.

I've started work on https://crbug.com/679381. It will be fixed before this
patch lands.

[commit-bot: I haz the power, 2017-02-23 20:05:49]

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

[commit-bot: I haz the power, 2017-02-23 20:05:50]

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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Marijn Kruisselbrink, 2017-02-23 20:07:06]

https://codereview.chromium.org/2645813006/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right):

https://codereview.chromium.org/2645813006/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:1088: ScriptState*
state = ScriptState::forMainWorld(frame());
I don't think using ScriptState::forMainWorld (or really any script state) here
is okay. Parsing JSON this way can have observable side effects in the world
you're passing it, and certainly can return different results depending on the
state of that world. Probably what you want to do is not involve any v8 code at
all, and instead use blink::parseJSON (from Source/platform/json/JSONParser.h)
to parse it?

[please use gerrit instead, 2017-02-23 20:09:27]

On 2017/02/23 20:07:06, Marijn Kruisselbrink wrote:
>
https://codereview.chromium.org/2645813006/diff/480001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right):
> 
>
https://codereview.chromium.org/2645813006/diff/480001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:1088:
ScriptState*
> state = ScriptState::forMainWorld(frame());
> I don't think using ScriptState::forMainWorld (or really any script state)
here
> is okay. Parsing JSON this way can have observable side effects in the world
> you're passing it, and certainly can return different results depending on the
> state of that world. Probably what you want to do is not involve any v8 code
at
> all, and instead use blink::parseJSON (from Source/platform/json/JSONParser.h)
> to parse it?

That would remove the advantages for WebIDL type-checking. Is the trade-off
worth it?

[Marijn Kruisselbrink, 2017-02-23 20:24:00]

On 2017/02/23 at 20:09:27, rouslan wrote:
> On 2017/02/23 20:07:06, Marijn Kruisselbrink wrote:
> >
https://codereview.chromium.org/2645813006/diff/480001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right):
> > 
> >
https://codereview.chromium.org/2645813006/diff/480001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:1088:
ScriptState*
> > state = ScriptState::forMainWorld(frame());
> > I don't think using ScriptState::forMainWorld (or really any script state)
here
> > is okay. Parsing JSON this way can have observable side effects in the world
> > you're passing it, and certainly can return different results depending on
the
> > state of that world. Probably what you want to do is not involve any v8 code
at
> > all, and instead use blink::parseJSON (from
Source/platform/json/JSONParser.h)
> > to parse it?
> 
> That would remove the advantages for WebIDL type-checking. Is the trade-off
worth it?

The trade-off appears to be either spin-up an entire new v8 isolate/blink
scriptstate, and use that to parse the manifest (similar to what I did in
https://codereview.chromium.org/924983002/ a while back for a completely
different usecase where I needed to serialize/deserialize v8 values outside of
script execution), which comes with pretty significant overhead, vs writing your
own parsing/validation code. I think that trade off is probably worth it
(one-time cost of writing the code, vs performance/memory overhead everytime the
code is executed, even though I guess this is probably not something that will
be hit over and over again).

[Marijn Kruisselbrink, 2017-02-23 20:55:22]

https://codereview.chromium.org/2645813006/diff/480001/components/payments/pa...
File components/payments/payment_manifest_downloader.cc (right):

https://codereview.chromium.org/2645813006/diff/480001/components/payments/pa...
components/payments/payment_manifest_downloader.cc:87: break;
Should this be continue rather than break? For example should one invalid link
header cause all later link headers to be ignored?

https://codereview.chromium.org/2645813006/diff/480001/components/payments/pa...
components/payments/payment_manifest_downloader.cc:92: rel->second.value_or("")
== "payment-method-manifest") {
This doesn't seem entirely spec compliant.
https://tools.ietf.org/html/rfc5988#section-5 defines the value of the rel param
as possibly multiple space seperated relation types, so you probably want to
pass the value of the rel param through something like
base::SplitStringPiece(rel, HTTP_LWS, base::TRIM_WHITESPACE,
base::SPLIT_WANT_NON_EMPTY)

(it's of course unlikely to be useful to use the same URL both as
payment-method-manifest and some other link relation type, but still better to
be spec compliant).

[xunjieli, 2017-02-24 18:18:30]

https://codereview.chromium.org/2645813006/diff/480001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
(right):

https://codereview.chromium.org/2645813006/diff/480001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:115:
url = new URL(method);
Is it possible to avoid using java.net.URL?
This does a synchronous DNS lookup in its constructor.

https://codereview.chromium.org/2645813006/diff/480001/components/payments/an...
File components/payments/android/payment_manifest_downloader_android.cc (right):

https://codereview.chromium.org/2645813006/diff/480001/components/payments/an...
components/payments/android/payment_manifest_downloader_android.cc:71: const
base::android::JavaParamRef<jobject>& jmethod_name,
Suggest passing the string directly. 
java.net.URL constructor does a synchronous DNS lookup. We should avoid using it
if we are using the native network stack.

https://codereview.chromium.org/2645813006/diff/480001/components/payments/pa...
File components/payments/payment_manifest_downloader.cc (right):

https://codereview.chromium.org/2645813006/diff/480001/components/payments/pa...
components/payments/payment_manifest_downloader.cc:7: #include <unordered_map>
nit: not used.

https://codereview.chromium.org/2645813006/diff/480001/components/payments/pa...
components/payments/payment_manifest_downloader.cc:65: if
(source->GetResponseCode() != 200) {
nit: net::HTTP_OK

[please use gerrit instead, 2017-03-01 22:09:48]

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

[commit-bot: I haz the power, 2017-03-01 22:10:42]

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-03-01 22:10:47]

Patchset #6 (id:500001) has been deleted

[commit-bot: I haz the power, 2017-03-01 22:28:25]

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

[commit-bot: I haz the power, 2017-03-01 22:28:27]

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

[please use gerrit instead, 2017-03-01 22:57:06]

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

[commit-bot: I haz the power, 2017-03-01 22:58:23]

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-03-02 00:17:16]

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

[commit-bot: I haz the power, 2017-03-02 00:17:18]

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

[please use gerrit instead, 2017-03-02 15:52:21]

Patchset #6 (id:520001) has been deleted

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

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

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

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-03-03 02:33:29]

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

[commit-bot: I haz the power, 2017-03-03 02:33:30]

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

[please use gerrit instead, 2017-03-03 03:05:14]

Description was changed from

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in the
renderer process. Then browser verifies the signatures of the installed
payment apps and shows only the apps with the matching SHA256
certificate fingerprints to the user.

Third-party native Android payment app integration is behind a flag:

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

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

This test webpage uses 'https://emerald-eon.appspot.com/bobpay' payment
method name. When the JavaScript PaymentRequest object is instantiated,
the browser first scans installed applications for any that can handle
the 'https://emerald-eon.appspot.com/bobpay' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:

  Link: <https://rsolomakhin.github.io/bobpay/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:

  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:dd:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed BobPay.apk
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

BUG=683329
==========

to

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in a
utility process. Then browser verifies the signatures of the installed
payment apps and shows only the apps with the matching SHA256
certificate fingerprints to the user.

Third-party native Android payment app integration is behind a flag:

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

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

This test webpage uses 'https://emerald-eon.appspot.com/bobpay' payment
method name. When the JavaScript PaymentRequest object is instantiated,
the browser first scans installed applications for any that can handle
the 'https://emerald-eon.appspot.com/bobpay' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:

  Link: <https://rsolomakhin.github.io/bobpay/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:

  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:dd:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed BobPay.apk
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

BUG=683329
==========

[please use gerrit instead, 2017-03-03 03:06:53]

Patchset #7 (id:560001) has been deleted

[please use gerrit instead, 2017-03-03 03:07:09]

Patchset #6 (id:540001) has been deleted

[please use gerrit instead, 2017-03-03 03:11:16]

rouslan@chromium.org changed reviewers:
- mek@chromium.org

[please use gerrit instead, 2017-03-03 03:11:17]

PTAL patch 6.

Ganggui, ptal PaymentRequestImpl.java, AndroidPaymentAppFactory.java, and
PaymentManifestVerifier.java.

Chris, ptal mojom and overall security. I've moved JSON parsing to a utility
process. I'm using java.net.URI to avoid synchronous DNS lookups in java.net.URL
constructor.

Ted, ptal the overall design of Java.

Helen, ptal networking in payment_manifest_downloader.cc. I'm using java.net.URI
instead of java.net.URL now. It's better than using a plain String from security
standpoint.

Marijn, I'ved moved JSON parsing to a utility process, so I'm moving you to CC.

https://codereview.chromium.org/2645813006/diff/480001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
(right):

https://codereview.chromium.org/2645813006/diff/480001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:115:
url = new URL(method);
On 2017/02/24 18:18:30, xunjieli wrote:
> Is it possible to avoid using java.net.URL?
> This does a synchronous DNS lookup in its constructor.

Done.

https://codereview.chromium.org/2645813006/diff/480001/components/payments/an...
File components/payments/android/payment_manifest_downloader_android.cc (right):

https://codereview.chromium.org/2645813006/diff/480001/components/payments/an...
components/payments/android/payment_manifest_downloader_android.cc:71: const
base::android::JavaParamRef<jobject>& jmethod_name,
On 2017/02/24 18:18:30, xunjieli wrote:
> Suggest passing the string directly. 
> java.net.URL constructor does a synchronous DNS lookup. We should avoid using
it
> if we are using the native network stack.

Using java.net.URI instead.

https://codereview.chromium.org/2645813006/diff/480001/components/payments/pa...
File components/payments/payment_manifest_downloader.cc (right):

https://codereview.chromium.org/2645813006/diff/480001/components/payments/pa...
components/payments/payment_manifest_downloader.cc:7: #include <unordered_map>
On 2017/02/24 18:18:30, xunjieli wrote:
> nit: not used.

Used on line 83:
std::unordered_map<std::string, base::Optional<std::string>> params;

https://codereview.chromium.org/2645813006/diff/480001/components/payments/pa...
components/payments/payment_manifest_downloader.cc:65: if
(source->GetResponseCode() != 200) {
On 2017/02/24 18:18:30, xunjieli wrote:
> nit: net::HTTP_OK

Done.

https://codereview.chromium.org/2645813006/diff/480001/components/payments/pa...
components/payments/payment_manifest_downloader.cc:87: break;
On 2017/02/23 20:55:22, Marijn Kruisselbrink wrote:
> Should this be continue rather than break? For example should one invalid link
> header cause all later link headers to be ignored?

Done.

https://codereview.chromium.org/2645813006/diff/480001/components/payments/pa...
components/payments/payment_manifest_downloader.cc:92: rel->second.value_or("")
== "payment-method-manifest") {
On 2017/02/23 20:55:21, Marijn Kruisselbrink wrote:
> This doesn't seem entirely spec compliant.
> https://tools.ietf.org/html/rfc5988#section-5 defines the value of the rel
param
> as possibly multiple space seperated relation types, so you probably want to
> pass the value of the rel param through something like
> base::SplitStringPiece(rel, HTTP_LWS, base::TRIM_WHITESPACE,
> base::SPLIT_WANT_NON_EMPTY)
> 
> (it's of course unlikely to be useful to use the same URL both as
> payment-method-manifest and some other link relation type, but still better to
> be spec compliant).

Done.

[please use gerrit instead, 2017-03-03 03:59:50]

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

[commit-bot: I haz the power, 2017-03-03 04:00:08]

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-03-03 05:19:12]

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

[commit-bot: I haz the power, 2017-03-03 05:19:13]

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-03-03 14:59:43]

Description was changed from

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in a
utility process. Then browser verifies the signatures of the installed
payment apps and shows only the apps with the matching SHA256
certificate fingerprints to the user.

Third-party native Android payment app integration is behind a flag:

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

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

This test webpage uses 'https://emerald-eon.appspot.com/bobpay' payment
method name. When the JavaScript PaymentRequest object is instantiated,
the browser first scans installed applications for any that can handle
the 'https://emerald-eon.appspot.com/bobpay' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:

  Link: <https://rsolomakhin.github.io/bobpay/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:

  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:dd:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed BobPay.apk
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

BUG=683329
==========

to

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in a utility
process. Then browser verifies the signatures of the installed payment
apps and shows only the apps with the matching SHA256 certificate
fingerprints to the user.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

Flag (disabled by default): chrome://flags/#android-payment-apps

To understand how the feature works, let's see what happens for a
'https://bobpay.com' payment method. When the JavaScript PaymentRequest
object is instantiated, the browser first scans installed applications
for any app that can handle the 'https://bobpay.com' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:
  Link: <https://bobpay.com/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:

  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:dd:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed Android app
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

BUG=683329
==========

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

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

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

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-03-03 15:02:18]

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

[please use gerrit instead, 2017-03-03 15:02:19]

Mathieu, can you take a look at the overall design please?

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

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

[please use gerrit instead, 2017-03-03 15:10:11]

Nico, ptal chrome/utility/chrome_content_utility_client.cc

[please use gerrit instead, 2017-03-03 15:12:04]

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

[please use gerrit instead, 2017-03-03 15:12:05]

Kentaro, ptal histograms.xml

[Nico, 2017-03-03 15:20:13]

chrome_content_utility_client.cc lgtm

[xunjieli, 2017-03-03 15:59:40]

payment_manifest_downloader.cc lgtm!

[xunjieli, 2017-03-03 16:00:22]

payment_manifest_downloader.cc lgtm!

https://codereview.chromium.org/2645813006/diff/620001/components/payments/co...
File components/payments/content/android/payment_manifest_downloader_android.cc
(right):

https://codereview.chromium.org/2645813006/diff/620001/components/payments/co...
components/payments/content/android/payment_manifest_downloader_android.cc:69:
const base::android::JavaParamRef<jobject>& jmethod_name,
nit: could pass a jstring directly here instead of the jobject to avoid the JNI
round trip.

[please use gerrit instead, 2017-03-03 16:02:45]

Thank you!

https://codereview.chromium.org/2645813006/diff/620001/components/payments/co...
File components/payments/content/android/payment_manifest_downloader_android.cc
(right):

https://codereview.chromium.org/2645813006/diff/620001/components/payments/co...
components/payments/content/android/payment_manifest_downloader_android.cc:69:
const base::android::JavaParamRef<jobject>& jmethod_name,
On 2017/03/03 16:00:22, xunjieli wrote:
> nit: could pass a jstring directly here instead of the jobject to avoid the
JNI
> round trip.

Acknowledged. Let's see what Chris thinks about it.

[commit-bot: I haz the power, 2017-03-03 16:08:29]

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

[commit-bot: I haz the power, 2017-03-03 16:08:31]

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

[please use gerrit instead, 2017-03-03 16:16:16]

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

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

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-03-03 16:23:19]

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

[commit-bot: I haz the power, 2017-03-03 16:23:39]

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-03-03 16:47:04]

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

[commit-bot: I haz the power, 2017-03-03 16:47:06]

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

[please use gerrit instead, 2017-03-03 16:55:42]

Patchset #10 (id:660001) has been deleted

[please use gerrit instead, 2017-03-03 17:20:13]

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

[commit-bot: I haz the power, 2017-03-03 17:20:40]

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

[Ted C, 2017-03-03 17:44:45]

The main thing that looks weird to me is that the native code is in the
component but the java code is in chrome/.  Looks like there are some cases that
do that, but that seems like a layering violation to me.

Any reason the java can't be in the component as well?

Also, tests for PaymentAppFinder and PaymentManifestVerifier would be good.

You'll likely need to provide some abstraction for interacting with
PackageManager, but there is enough handling there that some coverage would be
very good to have.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
(right):

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:35:
new PaymentAppFinder(webContents, methods, callback).find();
A slightly nicer pattern IMO would just be to have a static method find in
PaymentAppFinder that internally constructs an object.  newing something to just
throw it away looks odd to me.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:44:
private static class PaymentAppFinder implements ManifestVerifyCallback {
I'd pull this out to a separate class (package protected) just for the sake of
brevity of this file.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:69:
* A map of payment method names to the list of unverified (yet) Android apps
that claim to
nit of nits, I would put the (yet) before unverified

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:102:
if (method.startsWith(UrlConstants.HTTPS_URL_PREFIX)) {
I would do

if (!method.startsWith...) continue;

Then reduce intent.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:127:
List<ResolveInfo> apps = pm.queryIntentActivities(payIntent, 0);
you might need a strictmode exemption for this (crbug.com/613977), see
ExternalNavigationDelegateImpl#queryIntentActivities

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:132:
mParser.startUtilityProcess();
might be covered later, but do you do anything to handle when the utility
process is killed?  I suspect it has the likelihood to be killed like a
background renderer, but I'm not sure.

Just want to make sure this won't hang in that case or anything.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:138:
if (verifiers.size() == MAX_NUMBER_OF_MANIFESTS) break;
maybe log something here that we've exceeded our max allowed.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:172:
CharSequence label = resolveInfo.loadLabel(pm);
any reason we don't force people to have a valid label?

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:209:

for extra precaution, i'd add assert mPendingApps.isEmpty() here.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java
(right):

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:117:
// Intentionally ignore.
should we log anything here or where it marks it as invalid because this is null
below?

if we need to diagnose why this isn't working on a device, that might be good

[commit-bot: I haz the power, 2017-03-03 18:16:12]

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

[commit-bot: I haz the power, 2017-03-03 18:16:14]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-03-03 19:24:44]

histograms LGTM

[gogerald1, 2017-03-03 19:40:28]

lgtm with comments,

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
(right):

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:101:
assert method != null;
TextUtils.isEmpty(method) might be better

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:111:
if (uri.isAbsolute() && UrlConstants.HTTPS_SCHEME.equals(uri.getScheme())) {
Is it possible that the scheme is not 'https' after checking
method.startsWith(UrlConstants.HTTPS_URL_PREFIX)?

assert might be enough?

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:168:
PackageManager pm = ContextUtils.getApplicationContext().getPackageManager();
move inside if (app == null)

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:631:
query.notifyObserversOfResponse(false);
How about add this logic to disconnectFromClientWithDebugMessage so as to work
for other situations?

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1388:
if (query != null && query.matchesPaymentMethods(mMethodData)) {
We should no need matchesPaymentMethods if notifyObserversOfResponse is called
to clear observers at the end of PR. Assume the merchant website has no way to
change payment methods in current PR.

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1416:
if (!isFinishedQueryingPaymentApps() || !mIsCurrentPaymentRequestShowing) return
false;
This looks has been broken here
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr....

I can try to fix it in another CL,

[Mathieu, 2017-03-03 19:42:09]

lgtm

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
(right):

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:111:
if (uri.isAbsolute() && UrlConstants.HTTPS_SCHEME.equals(uri.getScheme())) {
in what case are  UrlConstants.HTTPS_SCHEME.equals(uri.getScheme()) and
method.startsWith(UrlConstants.HTTPS_URL_PREFIX) (line 102) different?

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:132:
mParser.startUtilityProcess();
Add a comment that we start the utility process as soon as possible.

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:140:

Add a comment that we do not verify app manifests for basic-card support

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:142:
payIntent.setAction(ACTION_PAY_BASIC_CARD);
reusing the same intent is prone to bugs (what if some fields get set above and
we forget to change them here?). Is it expensive to create a new Intent object?

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:151:
onSearchFinished();
would something like mCallback.onAllPaymentAppsCreated(); be good here?

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:156:
verifiers.get(i).verify();
would it be better to start verifying as soon as the verifier is added to
|verifiers|?

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:187:
private void removePendingApp(URI methodName, ResolveInfo resolveInfo) {
shouldn't private methods be at the bottom, or perhaps I'm wrong/outdated on the
style

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java
(right):

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:29:
* Verifies that the discovered native Android payment apps have the sufficient
privileges
Is there a public doc we can link to for this manifest format?

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
File components/payments/content/android/payment_manifest_downloader_test.cc
(right):

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
components/payments/content/android/payment_manifest_downloader_test.cc:1: //
Copyright 2017 The Chromium Authors. All rights reserved.
if it's a unittest the file name should end with _unittest.cc?

[please use gerrit instead, 2017-03-03 19:50:46]

rouslan@chromium.org changed reviewers:
- sdefresne@chromium.org

[please use gerrit instead, 2017-03-03 19:50:47]

Sylvian, the iOS layering was done in a separate patch, so I'm moving you to CC.

https://codereview.chromium.org/2645813006/diff/440001/components/payments/BU...
File components/payments/BUILD.gn (right):

https://codereview.chromium.org/2645813006/diff/440001/components/payments/BU...
components/payments/BUILD.gn:98: "//content/test:test_support",
On 2017/02/23 19:57:50, ಠ_ಠ wrote:
> On 2017/02/23 17:11:05, sdefresne wrote:
> > I think the iOS build failures are due to this line.
> > 
> > When gn parse a BUILD.gn file, all targets are evaluated and their
> dependencies
> > loaded, even if the target itself is never used on iOS. However, loading
> > //content is not allowed on iOS, so this cause the failure.
> > 
> > As //components/payments is build and used on iOS, it cannot depends on
> > //content. You should try to remove this dependency on //content. One option
> is
> > to re-implement the helper you use from //content. Another is to turn your
> > component into a layered component
> >
>
(https://www.chromium.org/developers/design-documents/layered-components-design).
> > 
> 
> Started on splitting up the component into layers in a separate patch, which
> will need to land first.

The split was done in a previous patch. This patch is rebased on top.

[please use gerrit instead, 2017-03-03 19:51:07]

Description was changed from

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in a utility
process. Then browser verifies the signatures of the installed payment
apps and shows only the apps with the matching SHA256 certificate
fingerprints to the user.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

Flag (disabled by default): chrome://flags/#android-payment-apps

To understand how the feature works, let's see what happens for a
'https://bobpay.com' payment method. When the JavaScript PaymentRequest
object is instantiated, the browser first scans installed applications
for any app that can handle the 'https://bobpay.com' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:
  Link: <https://bobpay.com/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:

  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:dd:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed Android app
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

BUG=683329
==========

to

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in a utility
process. Then browser verifies the signatures of the installed payment
apps and shows only the apps with the matching SHA256 certificate
fingerprints to the user.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

Flag (disabled by default): chrome://flags/#android-payment-apps

To understand how the feature works, let's see what happens for a
'https://bobpay.com' payment method. When the JavaScript PaymentRequest
object is instantiated, the browser first scans installed applications
for any app that can handle the 'https://bobpay.com' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:
  Link: <https://bobpay.com/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:
  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:dd:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed Android app
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

BUG=683329
==========

[palmer, 2017-03-03 22:56:31]

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
File components/payments/content/android/payment_manifest_parser.mojom (right):

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
components/payments/content/android/payment_manifest_parser.mojom:12:
array<string> sha256_cert_fingerprints;
Is it possible to express a tighter type-fit, e.g.

  array<uint8[64]> sha256_cert_fingerprints;

?

Also, for my info, are these fingerprints of the cert, fingerprints from in the
cert, or the result of sha256(cert.getPublicKeyBytes()) ?

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
File components/payments/content/android/utility/payment_manifest_parser.cc
(right):

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
components/payments/content/android/utility/payment_manifest_parser.cc:64:
output.clear();
If I understand this correctly, the first time we see a package named "*", we
get rid of everything previously parsed and return just the * section? Any *
section would thus override a more-specifically-named section, it would seem. Is
that safe and intended?

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
File components/payments/content/android/utility/payment_manifest_parser_test.cc
(right):

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
components/payments/content/android/utility/payment_manifest_parser_test.cc:92: 
Maybe add a test for an input that has a "com.bobpay" package, and also a *
package? I'd also add tests for duplicate keys, inconsistent capitalization, the
correct and incorrect lexical structure for SHA-256 fingerprints (all hex
digits? right # of hex digits? colons or not? et c.).

Also, what about embedded NUL characters, e.g.

  "package": "com.bobpay.app\0com.evil.app"

[palmer, 2017-03-03 23:18:54]

>   array<uint8[64]> sha256_cert_fingerprints;

Looks like the syntax is array<array<uint8, 64>>.

[please use gerrit instead, 2017-03-08 16:36:46]

On 2017/03/03 23:18:54, palmer wrote:
> >   array<uint8[64]> sha256_cert_fingerprints;
> 
> Looks like the syntax is array<array<uint8, 64>>.

sha256 output should be 32 bytes, right?

[palmer, 2017-03-08 20:18:57]

> sha256 output should be 32 bytes, right?

Right, sorry. :)

[please use gerrit instead, 2017-03-09 18:02:06]

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

[commit-bot: I haz the power, 2017-03-09 18:02:42]

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-03-09 18:05:34]

Ted, ptal patch 11. I've separated out the AndroidPaymentAppFinder.java class
and added tests for it, as well as tests for PaymentManifestVerifier.java. Also
moved the Java files that interact with C++ into components/payments to adhere
to the layering rules.

Chris, ptal patch 11. I've tightened the type of the fingerprint to be 32 bytes
instead of a free-form string. Also wrote the additional parser tests that
you've requested.

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
(right):

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:101:
assert method != null;
On 2017/03/03 19:40:27, gogerald1 wrote:
> TextUtils.isEmpty(method) might be better

Done.

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:111:
if (uri.isAbsolute() && UrlConstants.HTTPS_SCHEME.equals(uri.getScheme())) {
On 2017/03/03 19:40:27, gogerald1 wrote:
> Is it possible that the scheme is not 'https' after checking
> method.startsWith(UrlConstants.HTTPS_URL_PREFIX)?
> 
> assert might be enough?

Done.

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:168:
PackageManager pm = ContextUtils.getApplicationContext().getPackageManager();
On 2017/03/03 19:40:27, gogerald1 wrote:
> move inside if (app == null)

No longer applicable after abstracting away PackageManager.

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:631:
query.notifyObserversOfResponse(false);
On 2017/03/03 19:40:27, gogerald1 wrote:
> How about add this logic to disconnectFromClientWithDebugMessage so as to work
> for other situations?

Closing the connection to the current instance of PaymentRequest should not
necessarily trigger rejection of all .canMakePayment() queries in flight,
because disconnectFromClientWithDebugMessage() can be called when user closes
the UI, for example. However, other instances of PaymentRequest could still be
around with pending .canMakePayment() queries that should be answered
accurately, rather than with "false".

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1388:
if (query != null && query.matchesPaymentMethods(mMethodData)) {
On 2017/03/03 19:40:27, gogerald1 wrote:
> We should no need matchesPaymentMethods if notifyObserversOfResponse is called
> to clear observers at the end of PR. Assume the merchant website has no way to
> change payment methods in current PR.

This is important for the merchant that has created multiple PaymentRequest
objects with different payment methods and called .canMakePayment() on all of
them at the same time. Only one of them should be answered with "true" or
"false". The rest should be answered with QuotaExceeedError. All of the requests
are shared among instances of PaymentRequest in the static variable
sCanMakePaymentQueries. That's why we need matchesPaymentMethods() here. Sorry
about the confusion. I should avoid static variables like the plague, but in
this case not sure how to improve the code readability and reliability without
sacrificing functionality.

https://codereview.chromium.org/2645813006/diff/620001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1416:
if (!isFinishedQueryingPaymentApps() || !mIsCurrentPaymentRequestShowing) return
false;
On 2017/03/03 19:40:27, gogerald1 wrote:
> This looks has been broken here
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr....
> 
> I can try to fix it in another CL,

I don't follow. Please explain more.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
(right):

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:35:
new PaymentAppFinder(webContents, methods, callback).find();
On 2017/03/03 17:44:45, Ted C wrote:
> A slightly nicer pattern IMO would just be to have a static method find in
> PaymentAppFinder that internally constructs an object.  newing something to
just
> throw it away looks odd to me.

Done.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:44:
private static class PaymentAppFinder implements ManifestVerifyCallback {
On 2017/03/03 17:44:45, Ted C wrote:
> I'd pull this out to a separate class (package protected) just for the sake of
> brevity of this file.

Done.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:69:
* A map of payment method names to the list of unverified (yet) Android apps
that claim to
On 2017/03/03 17:44:45, Ted C wrote:
> nit of nits, I would put the (yet) before unverified

Done.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:102:
if (method.startsWith(UrlConstants.HTTPS_URL_PREFIX)) {
On 2017/03/03 17:44:45, Ted C wrote:
> I would do
> 
> if (!method.startsWith...) continue;
> 
> Then reduce intent.

Done.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:127:
List<ResolveInfo> apps = pm.queryIntentActivities(payIntent, 0);
On 2017/03/03 17:44:45, Ted C wrote:
> you might need a strictmode exemption for this (crbug.com/613977), see
> ExternalNavigationDelegateImpl#queryIntentActivities

Moved to async task.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:132:
mParser.startUtilityProcess();
On 2017/03/03 17:44:45, Ted C wrote:
> might be covered later, but do you do anything to handle when the utility
> process is killed?  I suspect it has the likelihood to be killed like a
> background renderer, but I'm not sure.
> 
> Just want to make sure this won't hang in that case or anything.

Yep, dead parser process causes all consequent requests to parse data to invoke
the onParseFailure() callback, which is properly handled in the
PaymentManifestVerifier.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:138:
if (verifiers.size() == MAX_NUMBER_OF_MANIFESTS) break;
On 2017/03/03 17:44:45, Ted C wrote:
> maybe log something here that we've exceeded our max allowed.

Done.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:172:
CharSequence label = resolveInfo.loadLabel(pm);
On 2017/03/03 17:44:45, Ted C wrote:
> any reason we don't force people to have a valid label?

Forcing now.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:209:

On 2017/03/03 17:44:45, Ted C wrote:
> for extra precaution, i'd add assert mPendingApps.isEmpty() here.

Done.

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java
(right):

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:117:
// Intentionally ignore.
On 2017/03/03 17:44:45, Ted C wrote:
> should we log anything here or where it marks it as invalid because this is
null
> below?
> 
> if we need to diagnose why this isn't working on a device, that might be good

Done.

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
(right):

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:111:
if (uri.isAbsolute() && UrlConstants.HTTPS_SCHEME.equals(uri.getScheme())) {
On 2017/03/03 19:42:09, Mathieu Perreault wrote:
> in what case are  UrlConstants.HTTPS_SCHEME.equals(uri.getScheme()) and
> method.startsWith(UrlConstants.HTTPS_URL_PREFIX) (line 102) different?

Good point. Using assert instead, as Ganggui recommends.

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:132:
mParser.startUtilityProcess();
On 2017/03/03 19:42:09, Mathieu Perreault wrote:
> Add a comment that we start the utility process as soon as possible.

Done.

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:140:

On 2017/03/03 19:42:09, Mathieu Perreault wrote:
> Add a comment that we do not verify app manifests for basic-card support

Done.

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:142:
payIntent.setAction(ACTION_PAY_BASIC_CARD);
On 2017/03/03 19:42:09, Mathieu Perreault wrote:
> reusing the same intent is prone to bugs (what if some fields get set above
and
> we forget to change them here?). Is it expensive to create a new Intent
object?

Creating new objects is expensive on Android, so I would not do it inside of a
loop. However, one more Intent object should have small impact and sizable
maintainability improvement, so following your advice here.

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:151:
onSearchFinished();
On 2017/03/03 19:42:09, Mathieu Perreault wrote:
> would something like mCallback.onAllPaymentAppsCreated(); be good here?

onSearchFinished() is better because it checks for READY_TO_PAY services in
"basic-card" apps as well.

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:156:
verifiers.get(i).verify();
On 2017/03/03 19:42:09, Mathieu Perreault wrote:
> would it be better to start verifying as soon as the verifier is added to
> |verifiers|?

That would be prone to a race condition. Suppose you have 2 payment manifests
that should be verified: https://alicepay.com and https://bobpay.com. Chrome
adds https://alicepay.com verifier to the list of verifiers and the list of
pending apps and starts verification. Then suppose for whatever reason the
verification was extremely fast and finished even before the verifier for
https://bobpay.com was created. The code in removePendingApp() removes
https://alicepay.com from the list of pending apps and sees that the list of
pending apps is now empty. This indicates that all verifiers finished. So
onSearchFinished() is called. However, the code still has not started verifying
https://bobpay.com yet. 


Granted, this probably will not happen because verification is asynchronous,
while creation of verifiers is synchronous, it's a common enough practice to
follow the pattern in this code that I'd rather prefer to be safe than sorry :-)

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:187:
private void removePendingApp(URI methodName, ResolveInfo resolveInfo) {
On 2017/03/03 19:42:09, Mathieu Perreault wrote:
> shouldn't private methods be at the bottom, or perhaps I'm wrong/outdated on
the
> style

Java methods follow "natural" method ordering. That's kind of vague, but I take
it to mean "place utility methods immediately below the last method where
they're used." onInvalidPaymentApp() is the last method to call
removePendingApp(), hence I placed removePendingApp() immediately below
onInvalidPaymentApp().

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java
(right):

https://codereview.chromium.org/2645813006/diff/680001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:29:
* Verifies that the discovered native Android payment apps have the sufficient
privileges
On 2017/03/03 19:42:09, Mathieu Perreault wrote:
> Is there a public doc we can link to for this manifest format?

Linking.

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
File components/payments/content/android/payment_manifest_downloader_test.cc
(right):

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
components/payments/content/android/payment_manifest_downloader_test.cc:1: //
Copyright 2017 The Chromium Authors. All rights reserved.
On 2017/03/03 19:42:09, Mathieu Perreault wrote:
> if it's a unittest the file name should end with _unittest.cc?

Done.

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
File components/payments/content/android/payment_manifest_parser.mojom (right):

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
components/payments/content/android/payment_manifest_parser.mojom:12:
array<string> sha256_cert_fingerprints;
On 2017/03/03 22:56:31, palmer wrote:
> Is it possible to express a tighter type-fit, e.g.
> 
>   array<uint8[64]> sha256_cert_fingerprints;
> 
> ?
> 
> Also, for my info, are these fingerprints of the cert, fingerprints from in
the
> cert, or the result of sha256(cert.getPublicKeyBytes()) ?

Tightened the type. Added a comment on what this actually is.

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
File components/payments/content/android/utility/payment_manifest_parser.cc
(right):

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
components/payments/content/android/utility/payment_manifest_parser.cc:64:
output.clear();
On 2017/03/03 22:56:31, palmer wrote:
> If I understand this correctly, the first time we see a package named "*", we
> get rid of everything previously parsed and return just the * section? Any *
> section would thus override a more-specifically-named section, it would seem.
Is
> that safe and intended?

That is intentional. We want to specify as tight of an interface as possible. If
there's a section with 'package': '*', then it must be the only section and it
should not have 'version' or 'sha256_cert_fingerprints'. Any deviations from a
correct format should cause the full file to be rejected. Added a comment.

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
File components/payments/content/android/utility/payment_manifest_parser_test.cc
(right):

https://codereview.chromium.org/2645813006/diff/680001/components/payments/co...
components/payments/content/android/utility/payment_manifest_parser_test.cc:92: 
On 2017/03/03 22:56:31, palmer wrote:
> Maybe add a test for an input that has a "com.bobpay" package, and also a *
> package? I'd also add tests for duplicate keys, inconsistent capitalization,
the
> correct and incorrect lexical structure for SHA-256 fingerprints (all hex
> digits? right # of hex digits? colons or not? et c.).
> 
> Also, what about embedded NUL characters, e.g.
> 
>   "package": "com.bobpay.app\0com.evil.app"

Done.

[commit-bot: I haz the power, 2017-03-09 18:06:32]

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

[commit-bot: I haz the power, 2017-03-09 18:06:34]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

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

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
(right):

https://codereview.chromium.org/2645813006/diff/640001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:127:
List<ResolveInfo> apps = pm.queryIntentActivities(payIntent, 0);
On 2017/03/09 18:05:32, ಠ_ಠ wrote:
> On 2017/03/03 17:44:45, Ted C wrote:
> > you might need a strictmode exemption for this (crbug.com/613977), see
> > ExternalNavigationDelegateImpl#queryIntentActivities
> 
> Moved to async task.

Eh, I did not, actually. Going to add an exemption in patch 12.

[please use gerrit instead, 2017-03-09 18:33:43]

Description was changed from

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in a utility
process. Then browser verifies the signatures of the installed payment
apps and shows only the apps with the matching SHA256 certificate
fingerprints to the user.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

Flag (disabled by default): chrome://flags/#android-payment-apps

To understand how the feature works, let's see what happens for a
'https://bobpay.com' payment method. When the JavaScript PaymentRequest
object is instantiated, the browser first scans installed applications
for any app that can handle the 'https://bobpay.com' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:
  Link: <https://bobpay.com/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:
  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:dd:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed Android app
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

BUG=683329
==========

to

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in a utility
process. Then browser verifies the signatures of the installed payment
apps and shows only the apps with the matching SHA256 certificate
fingerprints to the user.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

Flag (disabled by default): chrome://flags/#android-payment-apps

To understand how the feature works, let's see what happens for a
'https://bobpay.com' payment method. When the JavaScript PaymentRequest
object is instantiated, the browser first scans installed applications
for any app that can handle the 'https://bobpay.com' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:
  Link: <https://bobpay.com/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:
  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:AB:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed Android app
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

BUG=683329
==========

[please use gerrit instead, 2017-03-09 18:41:49]

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

[commit-bot: I haz the power, 2017-03-09 18:42:26]

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-03-09 20:00:03]

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

[commit-bot: I haz the power, 2017-03-09 20:00:05]

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

[please use gerrit instead, 2017-03-09 20:44:19]

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

[commit-bot: I haz the power, 2017-03-09 20:44:49]

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-03-09 22:11:02]

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

[commit-bot: I haz the power, 2017-03-09 22:11:04]

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

[please use gerrit instead, 2017-03-10 16:09:18]

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

[commit-bot: I haz the power, 2017-03-10 16:09:54]

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-03-10 16:48:18]

Description was changed from

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in a utility
process. Then browser verifies the signatures of the installed payment
apps and shows only the apps with the matching SHA256 certificate
fingerprints to the user.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ...

Flag (disabled by default): chrome://flags/#android-payment-apps

To understand how the feature works, let's see what happens for a
'https://bobpay.com' payment method. When the JavaScript PaymentRequest
object is instantiated, the browser first scans installed applications
for any app that can handle the 'https://bobpay.com' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:
  Link: <https://bobpay.com/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:
  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:AB:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed Android app
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

BUG=683329
==========

to

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in a utility
process. Then browser verifies the signatures of the installed payment
apps and shows only the apps with the matching SHA256 certificate
fingerprints to the user.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE

Flag (disabled by default): chrome://flags/#android-payment-apps

To understand how the feature works, let's see what happens for a
'https://bobpay.com' payment method. When the JavaScript PaymentRequest
object is instantiated, the browser first scans installed applications
for any app that can handle the 'https://bobpay.com' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:
  Link: <https://bobpay.com/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:
  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:AB:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed Android app
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

BUG=683329
==========

[commit-bot: I haz the power, 2017-03-10 17:40:16]

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

[commit-bot: I haz the power, 2017-03-10 17:40:18]

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

[please use gerrit instead, 2017-03-10 20:53:25]

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

[commit-bot: I haz the power, 2017-03-10 20:53:53]

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-03-10 22:49:30]

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

[commit-bot: I haz the power, 2017-03-10 22:49:32]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2017-03-13 13:51:40]

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

[commit-bot: I haz the power, 2017-03-13 13:51: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-03-13 15:18:12]

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

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

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

[Ted C, 2017-03-13 21:13:34]

java - lgtm

https://codereview.chromium.org/2645813006/diff/800001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java
(right):

https://codereview.chromium.org/2645813006/diff/800001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java:206:
if (TextUtils.isEmpty(label)) return;
I'd add logging here too that we skipped the package name due to incomplete
data.

https://codereview.chromium.org/2645813006/diff/800001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PackageManagerDelegate.java
(right):

https://codereview.chromium.org/2645813006/diff/800001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PackageManagerDelegate.java:21:
public class PackageManagerDelegate {
Potentially out of scope of this change, but I wonder if we should build out
better testing helpers to do this for us.

In particular, we can use ContextUtils.initApplicationContextForTests with our
own Context class.  We could either extend our AdvancedMockContext class, use a
ContextWrapper, "or something" that would return a Dummy/Mockable implementation
of getPackageManager.

Ideally, we'd find a way to use this without each small package needing to build
this out.

But again, that might be larger of an effort, but I wanted to bring it up to see
your thoughts.

And I guess it gets more difficult for things that load the label or icon using
the packagemanager.

https://codereview.chromium.org/2645813006/diff/800001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderTest.java
(right):

https://codereview.chromium.org/2645813006/diff/800001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderTest.java:63:
new Intent("org.chromium.intent.action.PAY_BASIC_CARD")))
can you make this string package protected in the other file so we can share it?

https://codereview.chromium.org/2645813006/diff/800001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderTest.java:66:
methodNames.add("basic-card");
same throughout where we can share strings

[please use gerrit instead, 2017-03-13 22:16:22]

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

[commit-bot: I haz the power, 2017-03-13 22:17:32]

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-03-13 22:19:28]

Description was changed from

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in a utility
process. Then browser verifies the signatures of the installed payment
apps and shows only the apps with the matching SHA256 certificate
fingerprints to the user.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE

Flag (disabled by default): chrome://flags/#android-payment-apps

To understand how the feature works, let's see what happens for a
'https://bobpay.com' payment method. When the JavaScript PaymentRequest
object is instantiated, the browser first scans installed applications
for any app that can handle the 'https://bobpay.com' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:
  Link: <https://bobpay.com/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:
  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:AB:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed Android app
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

BUG=683329
==========

to

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in a utility
process. Then browser verifies the signatures of the installed payment
apps and shows only the apps with the matching SHA256 certificate
fingerprints to the user.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE

Flag (enabled by default): chrome://flags/#android-payment-apps

To understand how the feature works, let's see what happens for a
'https://bobpay.com' payment method. When the JavaScript PaymentRequest
object is instantiated, the browser first scans installed applications
for any app that can handle the 'https://bobpay.com' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:
  Link: <https://bobpay.com/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:
  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:AB:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed Android app
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

BUG=683329
==========

[please use gerrit instead, 2017-03-13 22:19:54]

https://codereview.chromium.org/2645813006/diff/800001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java
(right):

https://codereview.chromium.org/2645813006/diff/800001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java:206:
if (TextUtils.isEmpty(label)) return;
On 2017/03/13 21:13:34, Ted C wrote:
> I'd add logging here too that we skipped the package name due to incomplete
> data.

Done.

https://codereview.chromium.org/2645813006/diff/800001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PackageManagerDelegate.java
(right):

https://codereview.chromium.org/2645813006/diff/800001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PackageManagerDelegate.java:21:
public class PackageManagerDelegate {
On 2017/03/13 21:13:34, Ted C wrote:
> Potentially out of scope of this change, but I wonder if we should build out
> better testing helpers to do this for us.
> 
> In particular, we can use ContextUtils.initApplicationContextForTests with our
> own Context class.  We could either extend our AdvancedMockContext class, use
a
> ContextWrapper, "or something" that would return a Dummy/Mockable
implementation
> of getPackageManager.
> 
> Ideally, we'd find a way to use this without each small package needing to
build
> this out.
> 
> But again, that might be larger of an effort, but I wanted to bring it up to
see
> your thoughts.
> 
> And I guess it gets more difficult for things that load the label or icon
using
> the packagemanager.

That sounds like a good plan in general for a future refactor. Are there other
places that need to abstract away the package manager like this? I can only
think of the "Share..." button.

https://codereview.chromium.org/2645813006/diff/800001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderTest.java
(right):

https://codereview.chromium.org/2645813006/diff/800001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderTest.java:63:
new Intent("org.chromium.intent.action.PAY_BASIC_CARD")))
On 2017/03/13 21:13:34, Ted C wrote:
> can you make this string package protected in the other file so we can share
it?

Done.

https://codereview.chromium.org/2645813006/diff/800001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderTest.java:66:
methodNames.add("basic-card");
On 2017/03/13 21:13:34, Ted C wrote:
> same throughout where we can share strings

Done. (Sharing "basic-card" and "READY_TO_PAY".)

[palmer, 2017-03-13 23:17:30]

LGTM. Thanks for adding the tests and typing the IPC more tightly. :)

https://codereview.chromium.org/2645813006/diff/820001/components/payments/co...
File
components/payments/content/android/utility/payment_manifest_parser_unittest.cc
(right):

https://codereview.chromium.org/2645813006/diff/820001/components/payments/co...
components/payments/content/android/utility/payment_manifest_parser_unittest.cc:137:
TEST(PaymentManifestParserTest, FingerprintsShouldBeUpperCase) {
Note: If you ever need to be more lenient on this, that would obviously be fine
from a security perspective.

[please use gerrit instead, 2017-03-13 23:26:50]

The CQ bit was unchecked by rouslan@chromium.org

[please use gerrit instead, 2017-03-13 23:26:56]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2017-03-13 23:26:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org, thakis@chromium.org, mathp@chromium.org,
gogerald@chromium.org, haraken@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2645813006/#ps820001
(title: "Ted's comments")

[commit-bot: I haz the power, 2017-03-13 23:27:41]

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-03-13 23:38:25]

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

[commit-bot: I haz the power, 2017-03-13 23:38:28]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[please use gerrit instead, 2017-03-14 00:34:18]

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

[please use gerrit instead, 2017-03-14 00:34:21]

jochen@, deps owner total for components/data_use_measurement.

[jochen (gone - plz use gerrit), 2017-03-14 20:00:19]

Hey,

thanks for the detailed design doc! the components change lgtm. However, I'd
argue that this is a web-exposed change (it implements part of the payment
request API). As such this should come with an intent to ship, and we should
only turn this on by default once it is approved.

Can you please either make sure that the feature is not enabled by default
before the intent to ship was approved, or delay this CL until after it was
approved?

thanks
-jochen

[please use gerrit instead, 2017-03-14 20:10:09]

On 2017/03/14 20:00:19, jochen wrote:
> Hey,
> 
> thanks for the detailed design doc! the components change lgtm. However, I'd
> argue that this is a web-exposed change (it implements part of the payment
> request API). As such this should come with an intent to ship, and we should
> only turn this on by default once it is approved.
> 
> Can you please either make sure that the feature is not enabled by default
> before the intent to ship was approved, or delay this CL until after it was
> approved?
> 
> thanks
> -jochen

Fair point about the intent to ship. I've started the revert process in
http://crrev.com/2749023003 in order to disable the feature by default. Will be
sending out the intent to ship shortly. As for this particular patch, I am
sending it to CQ now.

[please use gerrit instead, 2017-03-14 20:12:58]

Description was changed from

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in a utility
process. Then browser verifies the signatures of the installed payment
apps and shows only the apps with the matching SHA256 certificate
fingerprints to the user.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE

Flag (enabled by default): chrome://flags/#android-payment-apps

To understand how the feature works, let's see what happens for a
'https://bobpay.com' payment method. When the JavaScript PaymentRequest
object is instantiated, the browser first scans installed applications
for any app that can handle the 'https://bobpay.com' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:
  Link: <https://bobpay.com/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:
  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:AB:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed Android app
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

BUG=683329
==========

to

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in a utility
process. Then browser verifies the signatures of the installed payment
apps and shows only the apps with the matching SHA256 certificate
fingerprints to the user.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE

Flag (disabled by default): chrome://flags/#android-payment-apps

To understand how the feature works, let's see what happens for a
'https://bobpay.com' payment method. When the JavaScript PaymentRequest
object is instantiated, the browser first scans installed applications
for any app that can handle the 'https://bobpay.com' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:
  Link: <https://bobpay.com/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:
  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:AB:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed Android app
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

BUG=683329
==========

[please use gerrit instead, 2017-03-14 20:14:08]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2017-03-14 20:14:49]

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-03-14 20:24:24]

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

[commit-bot: I haz the power, 2017-03-14 20:24:27]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[please use gerrit instead, 2017-03-14 20:56:18]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2017-03-14 20:56:19]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mathp@chromium.org, gogerald@chromium.org, xunjieli@chromium.org,
tedchoc@chromium.org, haraken@chromium.org, thakis@chromium.org,
palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/2645813006/#ps840001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-14 20:57:02]

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-03-14 23:13:48]

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

[commit-bot: I haz the power, 2017-03-14 23:13:52]

Failed to apply patch for
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:
While running git apply --index -p1;
  error: patch failed:
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:27
  error:
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java:
patch does not apply

Patch:      
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
Index:
chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
b/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
index
ac44d119dbc56f3cfaeeda3e985e5d6176d90f83..58f37e7b2671173202eb6c800a5fb7edd4a39639
100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFactory.java
@@ -4,20 +4,17 @@
 
 package org.chromium.chrome.browser.payments;
 
-import android.content.Context;
 import android.content.Intent;
-import android.content.pm.PackageManager;
 import android.content.pm.ResolveInfo;
 import android.graphics.drawable.Drawable;
-import android.net.Uri;
+import android.text.TextUtils;
 import android.util.Pair;
 
-import org.chromium.base.ContextUtils;
-import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.ChromeFeatureList;
-import org.chromium.chrome.browser.UrlConstants;
 import
org.chromium.chrome.browser.payments.PaymentAppFactory.PaymentAppCreatedCallback;
 import
org.chromium.chrome.browser.payments.PaymentAppFactory.PaymentAppFactoryAddition;
+import org.chromium.components.payments.PaymentManifestDownloader;
+import org.chromium.components.payments.PaymentManifestParser;
 import org.chromium.content_public.browser.WebContents;
 
 import java.util.HashMap;
@@ -27,78 +24,13 @@ import java.util.Set;
 
 /** Builds instances of payment apps based on installed third party Android
payment apps. */
 public class AndroidPaymentAppFactory implements PaymentAppFactoryAddition {
-    private static final String ACTION_IS_READY_TO_PAY =
-            "org.chromium.intent.action.IS_READY_TO_PAY";
-
-    /** The action name for the Pay Basic-card Intent. */
-    private static final String ACTION_PAY_BASIC_CARD =
"org.chromium.intent.action.PAY_BASIC_CARD";
-
-    /**
-     * The basic-card payment method name used by merchant and defined by W3C:
-     * https://w3c.github.io/webpayments-methods-card/#method-id
-     */
-    private static final String BASIC_CARD_PAYMENT_METHOD = "basic-card";
-
     @Override
-    public void create(WebContents webContents, Set<String> methods,
-            PaymentAppCreatedCallback callback) {
-        Context context = ChromeActivity.fromWebContents(webContents);
-        if (context == null) {
-            callback.onAllPaymentAppsCreated();
-            return;
-        }
-
-        Map<String, AndroidPaymentApp> installedApps = new HashMap<>();
-        PackageManager pm = context.getPackageManager();
-        Intent payIntent = new Intent();
-
-        boolean paymentAppsFilterEnabled =
-               
ChromeFeatureList.isEnabled(ChromeFeatureList.ANDROID_PAYMENT_APPS_FILTER);
-        Intent filterIntent = new Intent(AndroidPaymentApp.ACTION_PAY);
-
-        for (String methodName : methods) {
-            if (methodName.startsWith(UrlConstants.HTTPS_URL_PREFIX)) {
-                payIntent.setAction(AndroidPaymentApp.ACTION_PAY);
-                payIntent.setData(Uri.parse(methodName));
-            } else if (methodName.equals(BASIC_CARD_PAYMENT_METHOD)) {
-                payIntent.setAction(ACTION_PAY_BASIC_CARD);
-                payIntent.setData(null);
-            } else {
-                continue;
-            }
-
-            List<ResolveInfo> matches = pm.queryIntentActivities(payIntent, 0);
-            for (int i = 0; i < matches.size(); i++) {
-                ResolveInfo match = matches.get(i);
-                String packageName = match.activityInfo.packageName;
-                if (paymentAppsFilterEnabled) {
-                    filterIntent.setPackage(packageName);
-                    if (pm.resolveActivity(filterIntent, 0) == null) continue;
-                }
-                // Do not recommend disabled apps.
-                if
(!PaymentPreferencesUtil.isAndroidPaymentAppEnabled(packageName)) continue;
-                AndroidPaymentApp installedApp =
installedApps.get(packageName);
-                if (installedApp == null) {
-                    CharSequence label = match.loadLabel(pm);
-                    installedApp =
-                            new AndroidPaymentApp(webContents, packageName,
match.activityInfo.name,
-                                    label == null ? "" : label.toString(),
match.loadIcon(pm));
-                    callback.onPaymentAppCreated(installedApp);
-                    installedApps.put(packageName, installedApp);
-                }
-                installedApp.addMethodName(methodName);
-            }
-        }
-
-        List<ResolveInfo> matches = pm.queryIntentServices(new
Intent(ACTION_IS_READY_TO_PAY), 0);
-        for (int i = 0; i < matches.size(); i++) {
-            ResolveInfo match = matches.get(i);
-            String packageName = match.serviceInfo.packageName;
-            AndroidPaymentApp installedApp = installedApps.get(packageName);
-            if (installedApp != null)
installedApp.setIsReadyToPayAction(match.serviceInfo.name);
-        }
-
-        callback.onAllPaymentAppsCreated();
+    public void create(
+            WebContents webContents, Set<String> methods,
PaymentAppCreatedCallback callback) {
+        AndroidPaymentAppFinder.find(webContents, methods,
+               
ChromeFeatureList.isEnabled(ChromeFeatureList.ANDROID_PAYMENT_APPS_FILTER),
+                new PaymentManifestDownloader(webContents), new
PaymentManifestParser(),
+                new PackageManagerDelegate(), callback);
     }
 
     /**
@@ -107,11 +39,11 @@ public class AndroidPaymentAppFactory implements
PaymentAppFactoryAddition {
      * @return True if there are Android payment apps on device.
      */
     public static boolean hasAndroidPaymentApps() {
-        PackageManager pm =
ContextUtils.getApplicationContext().getPackageManager();
+        PackageManagerDelegate packageManagerDelegate = new
PackageManagerDelegate();
         // Note that all Android payment apps must support
org.chromium.intent.action.PAY action
         // without additional data to be detected.
         Intent payIntent = new Intent(AndroidPaymentApp.ACTION_PAY);
-        return !pm.queryIntentActivities(payIntent, 0).isEmpty();
+        return
!packageManagerDelegate.getActivitiesThatCanRespondToIntent(payIntent).isEmpty();
     }
 
     /**
@@ -123,14 +55,17 @@ public class AndroidPaymentAppFactory implements
PaymentAppFactoryAddition {
     public static Map<String, Pair<String, Drawable>>
getAndroidPaymentAppsInfo() {
         Map<String, Pair<String, Drawable>> paymentAppsInfo = new HashMap<>();
 
-        PackageManager pm =
ContextUtils.getApplicationContext().getPackageManager();
+        PackageManagerDelegate packageManagerDelegate = new
PackageManagerDelegate();
         Intent payIntent = new Intent(AndroidPaymentApp.ACTION_PAY);
-        List<ResolveInfo> matches = pm.queryIntentActivities(payIntent, 0);
+        List<ResolveInfo> matches =
+               
packageManagerDelegate.getActivitiesThatCanRespondToIntent(payIntent);
         if (matches.isEmpty()) return paymentAppsInfo;
 
         for (ResolveInfo match : matches) {
+            CharSequence label = packageManagerDelegate.getAppLabel(match);
+            if (TextUtils.isEmpty(label)) continue;
             Pair<String, Drawable> appInfo =
-                    new Pair<>(match.loadLabel(pm).toString(),
match.loadIcon(pm));
+                    new Pair<>(label.toString(),
packageManagerDelegate.getAppIcon(match));
             paymentAppsInfo.put(match.activityInfo.packageName, appInfo);
         }

[please use gerrit instead, 2017-03-14 23:32:14]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2017-03-14 23:32:21]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mathp@chromium.org, gogerald@chromium.org, xunjieli@chromium.org,
tedchoc@chromium.org, haraken@chromium.org, thakis@chromium.org,
palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/2645813006/#ps860001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-14 23:33:45]

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-03-15 01:12:55]

CQ is committing da patch.

Bot data: {"patchset_id": 860001, "attempt_start_ts": 1489534334685030,
"parent_rev": "18a3165ddb7326845a6028b1e3f93f7d347bde95", "commit_rev":
"8fdbfb2484873d09de132e051da87e580382f95b"}

[commit-bot: I haz the power, 2017-03-15 01:13:46]

Description was changed from

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in a utility
process. Then browser verifies the signatures of the installed payment
apps and shows only the apps with the matching SHA256 certificate
fingerprints to the user.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE

Flag (disabled by default): chrome://flags/#android-payment-apps

To understand how the feature works, let's see what happens for a
'https://bobpay.com' payment method. When the JavaScript PaymentRequest
object is instantiated, the browser first scans installed applications
for any app that can handle the 'https://bobpay.com' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:
  Link: <https://bobpay.com/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:
  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:AB:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed Android app
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

BUG=683329
==========

to

==========
Download web payment manifests.

The browser downloads web payment manifests and parses them in a utility
process. Then browser verifies the signatures of the installed payment
apps and shows only the apps with the matching SHA256 certificate
fingerprints to the user.

Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE

Flag (disabled by default): chrome://flags/#android-payment-apps

To understand how the feature works, let's see what happens for a
'https://bobpay.com' payment method. When the JavaScript PaymentRequest
object is instantiated, the browser first scans installed applications
for any app that can handle the 'https://bobpay.com' URL with
'org.chromium.intent.action.PAY' intent. If a matching app is found,
then the browser uses HTTP HEAD request for this URL to read the HTTP
Link header.

Example HTTP link header:
  Link: <https://bobpay.com/payment-manifest.json>;
rel="payment-method-manifest"

Then browser uses HTTP GET request for the manifest file to retrieve its
contents. Both HEAD and GET requests do not follow 300 redirects and
require https:// scheme.

Example manifest contents:
  {
    "android": [{
      "package": "com.bobpay.app",
      "version": 1,
      "sha256_cert_fingerprints": ["30:82:01:AB:30:82:01:46:02:01:01"]
    }]
  }

Finally, browser compares the properties of the installed Android app
with the contents of the manifest. Thus, the owner of the payment method
URL controls which payment apps can use this URL as a payment method
identifier.

To test:
1) Install BobPay.apk from
   https://drive.google.com/open?id=0B9_TYWUgXNVFQ1pUb21PQkZ3VVE
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.

BUG=683329

Review-Url: https://codereview.chromium.org/2645813006
Cr-Commit-Position: refs/heads/master@{#456934}
Committed:
https://chromium.googlesource.com/chromium/src/+/8fdbfb2484873d09de132e051da8...
==========

[commit-bot: I haz the power, 2017-03-15 01:13:50]

Committed patchset #19 (id:860001) as
https://chromium.googlesource.com/chromium/src/+/8fdbfb2484873d09de132e051da8...