|
|
Created:
3 years, 8 months ago by gogerald1 Modified:
3 years, 8 months ago Reviewers:
please use gerrit instead CC:
chromium-reviews, mahmadi+paymentswatch_chromium.org, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, gogerald+paymentswatch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse <meta-data> tag instead of intent filter data to detect supported payment methods.
Design doc:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE
To test:
1) Install BobPayWithMetaData.apk from
https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNSllxOWtTU0U0TDQ/view?usp=sharing
2) Enable chrome://flags/#android-payment-apps.
3) Navigate to https://rsolomakhin.github.io/pr/bob/.
4) Click "Buy" button on the webpage.
BUG=707246
Review-Url: https://codereview.chromium.org/2793573002
Cr-Commit-Position: refs/heads/master@{#461274}
Committed: https://chromium.googlesource.com/chromium/src/+/7bb0a7ba88c589910ed35f74f7892ef93fbd0eda
Patch Set 1 #
Total comments: 17
Patch Set 2 : address comments #Patch Set 3 : remove unecessary check #Patch Set 4 : move <meta-data> to pay activity #
Messages
Total messages: 43 (31 generated)
Description was changed from ========== Use <meta-data> tag instead of intent filter data to detect supported payment methods. BUG= ========== to ========== Use <meta-data> tag instead of intent filter data to detect supported payment methods. BUG= ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use <meta-data> tag instead of intent filter data to detect supported payment methods. BUG= ========== to ========== Use <meta-data> tag instead of intent filter data to detect supported payment methods. BUG=707246 ==========
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Use <meta-data> tag instead of intent filter data to detect supported payment methods. BUG=707246 ========== to ========== Use <meta-data> tag instead of intent filter data to detect supported payment methods. Design doc: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE BUG=707246To test: 1) Install BobPay.apk from https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNQzcxUm9zXzBPbEE/vie... 2) Enable chrome://flags/#android-payment-apps. 3) Navigate to https://rsolomakhin.github.io/pr/bob/. 4) Click "Buy" button on the webpage. ==========
Description was changed from ========== Use <meta-data> tag instead of intent filter data to detect supported payment methods. Design doc: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE BUG=707246To test: 1) Install BobPay.apk from https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNQzcxUm9zXzBPbEE/vie... 2) Enable chrome://flags/#android-payment-apps. 3) Navigate to https://rsolomakhin.github.io/pr/bob/. 4) Click "Buy" button on the webpage. ========== to ========== Use <meta-data> tag instead of intent filter data to detect supported payment methods. Design doc: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE To test: 1) Install BobPay.apk from https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNQzcxUm9zXzBPbEE/vie... 2) Enable chrome://flags/#android-payment-apps. 3) Navigate to https://rsolomakhin.github.io/pr/bob/. 4) Click "Buy" button on the webpage. BUG=707246 ==========
Description was changed from ========== Use <meta-data> tag instead of intent filter data to detect supported payment methods. Design doc: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE To test: 1) Install BobPay.apk from https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNQzcxUm9zXzBPbEE/vie... 2) Enable chrome://flags/#android-payment-apps. 3) Navigate to https://rsolomakhin.github.io/pr/bob/. 4) Click "Buy" button on the webpage. BUG=707246 ========== to ========== Use <meta-data> tag instead of intent filter data to detect supported payment methods. Design doc: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE To test: 1) Install BobPayWithMetaData.apk from https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNQzcxUm9zXzBPbEE/vie... 2) Enable chrome://flags/#android-payment-apps. 3) Navigate to https://rsolomakhin.github.io/pr/bob/. 4) Click "Buy" button on the webpage. BUG=707246 ==========
gogerald@chromium.org changed reviewers: + rouslan@chromium.org
Hi rouslan@, ptal,
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java (right): https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java:183: if (!mPaymentMethods.isEmpty()) { This check seems redundant now. Let's remove it. https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java:223: private List<ResolveInfo> findOutAppsSupportTheGivenPaymentMethod(List<ResolveInfo> apps, static? https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java:223: private List<ResolveInfo> findOutAppsSupportTheGivenPaymentMethod(List<ResolveInfo> apps, filterAppsByMethodName(). https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java:224: List<String[]> appsMethods, List<String> appsDefaultMethods, String targetMethodName) { Let's pass URI data type in as many places as possible: "URI targetMethodName". https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java:232: if (targetMethodName.equalsIgnoreCase(appsDefaultMethods.get(i))) { Don't ignore case. Let's be strict. https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PackageManagerDelegate.java (right): https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PackageManagerDelegate.java:123: } catch (NameNotFoundException e) { Let's wrap in try-catch only the function that throws this exception: getApplicationInfo(). https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PackageManagerDelegate.java:145: } catch (NameNotFoundException e) { Ditto.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, please take another look, https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java (right): https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java:183: if (!mPaymentMethods.isEmpty()) { On 2017/03/31 15:47:08, ಠ_ಠ wrote: > This check seems redundant now. Let's remove it. Why? mPaymentMethods is merchant queried methods which haven't been checked before. https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java:223: private List<ResolveInfo> findOutAppsSupportTheGivenPaymentMethod(List<ResolveInfo> apps, On 2017/03/31 15:47:08, ಠ_ಠ wrote: > static? Done. https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java:223: private List<ResolveInfo> findOutAppsSupportTheGivenPaymentMethod(List<ResolveInfo> apps, On 2017/03/31 15:47:08, ಠ_ಠ wrote: > filterAppsByMethodName(). Done. https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java:224: List<String[]> appsMethods, List<String> appsDefaultMethods, String targetMethodName) { On 2017/03/31 15:47:08, ಠ_ಠ wrote: > Let's pass URI data type in as many places as possible: "URI targetMethodName". Looks not good since this interface is also used for basic-card https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java:232: if (targetMethodName.equalsIgnoreCase(appsDefaultMethods.get(i))) { On 2017/03/31 15:47:08, ಠ_ಠ wrote: > Don't ignore case. Let's be strict. Done. I am worried that the merchant might use upper case (like the first character 'H'), is it allowed in the spec? or do we lower cased them somewhere? https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PackageManagerDelegate.java (right): https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PackageManagerDelegate.java:123: } catch (NameNotFoundException e) { On 2017/03/31 15:47:08, ಠ_ಠ wrote: > Let's wrap in try-catch only the function that throws this exception: > getApplicationInfo(). Done. getResourcesForApplication might also through that exception https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PackageManagerDelegate.java:145: } catch (NameNotFoundException e) { On 2017/03/31 15:47:08, ಠ_ಠ wrote: > Ditto. Done. Ditto
LGTM % comment. Please remove the redundant mPaymentMethods.isEmpty() check. https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java (right): https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java:183: if (!mPaymentMethods.isEmpty()) { On 2017/03/31 16:18:43, gogerald1 wrote: > On 2017/03/31 15:47:08, ಠ_ಠ wrote: > > This check seems redundant now. Let's remove it. > > Why? mPaymentMethods is merchant queried methods which haven't been checked > before. It's redundant because the for loop on the next line is a no-op for empty mPaymentMethods. https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java:232: if (targetMethodName.equalsIgnoreCase(appsDefaultMethods.get(i))) { On 2017/03/31 16:18:43, gogerald1 wrote: > On 2017/03/31 15:47:08, ಠ_ಠ wrote: > > Don't ignore case. Let's be strict. > > Done. I am worried that the merchant might use upper case (like the first > character 'H'), is it allowed in the spec? or do we lower cased them somewhere? We prohibit variations in the spec as much as possible. Variations breed security holes and noncompatible implementaitons. See http://queue.acm.org/detail.cfm?id=1999945
https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java (right): https://codereview.chromium.org/2793573002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java:183: if (!mPaymentMethods.isEmpty()) { On 2017/03/31 16:26:30, ಠ_ಠ wrote: > On 2017/03/31 16:18:43, gogerald1 wrote: > > On 2017/03/31 15:47:08, ಠ_ಠ wrote: > > > This check seems redundant now. Let's remove it. > > > > Why? mPaymentMethods is merchant queried methods which haven't been checked > > before. > > It's redundant because the for loop on the next line is a no-op for empty > mPaymentMethods. yes, my bad, updated in the new patch
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2793573002/#ps100001 (title: "remove unecessary check")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by gogerald@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by gogerald@chromium.org
Patchset #4 (id:120001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi rouslan@, please take another look, update the CL to match the spec,
Description was changed from ========== Use <meta-data> tag instead of intent filter data to detect supported payment methods. Design doc: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE To test: 1) Install BobPayWithMetaData.apk from https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNQzcxUm9zXzBPbEE/vie... 2) Enable chrome://flags/#android-payment-apps. 3) Navigate to https://rsolomakhin.github.io/pr/bob/. 4) Click "Buy" button on the webpage. BUG=707246 ========== to ========== Use <meta-data> tag instead of intent filter data to detect supported payment methods. Design doc: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE To test: 1) Install BobPayWithMetaData.apk from https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNSllxOWtTU0U0TDQ/vie... 2) Enable chrome://flags/#android-payment-apps. 3) Navigate to https://rsolomakhin.github.io/pr/bob/. 4) Click "Buy" button on the webpage. BUG=707246 ==========
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gogerald@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491003367129360, "parent_rev": "df2f72edebddff3b7ded81b9022af5c9ae368607", "commit_rev": "7bb0a7ba88c589910ed35f74f7892ef93fbd0eda"}
Message was sent while issue was closed.
Description was changed from ========== Use <meta-data> tag instead of intent filter data to detect supported payment methods. Design doc: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE To test: 1) Install BobPayWithMetaData.apk from https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNSllxOWtTU0U0TDQ/vie... 2) Enable chrome://flags/#android-payment-apps. 3) Navigate to https://rsolomakhin.github.io/pr/bob/. 4) Click "Buy" button on the webpage. BUG=707246 ========== to ========== Use <meta-data> tag instead of intent filter data to detect supported payment methods. Design doc: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE To test: 1) Install BobPayWithMetaData.apk from https://drive.google.com/a/google.com/file/d/0B3ISiXgGE1MNSllxOWtTU0U0TDQ/vie... 2) Enable chrome://flags/#android-payment-apps. 3) Navigate to https://rsolomakhin.github.io/pr/bob/. 4) Click "Buy" button on the webpage. BUG=707246 Review-Url: https://codereview.chromium.org/2793573002 Cr-Commit-Position: refs/heads/master@{#461274} Committed: https://chromium.googlesource.com/chromium/src/+/7bb0a7ba88c589910ed35f74f789... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001) as https://chromium.googlesource.com/chromium/src/+/7bb0a7ba88c589910ed35f74f789... |