[Payments] Present web payment app instead of instruments to user

Present web payment app instead of instruments to user

Test video:
https://drive.google.com/file/d/0B4yF5XJG4VQAMEFaNzV1Y2t4ODQ/view?usp=sharing

This CL is based on:
https://codereview.chromium.org/2958333002/

BUG=735063
Review-Url: https://codereview.chromium.org/2969713002
Cr-Commit-Position: refs/heads/master@{#484368}
Committed: https://chromium.googlesource.com/chromium/src/+/ad5b9e54353ce83b4805f4d3e9ba5cc5fec90102

[gogerald1, 2017-06-30 18:19:21]

Patchset #1 (id:1) has been deleted

[gogerald1, 2017-06-30 18:20:17]

Description was changed from

==========
present payment handler draft

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

to

==========
Present web payment app instead of instruments to user

BUG=735063
==========

[gogerald1, 2017-06-30 18:36:41]

Patchset #1 (id:20001) has been deleted

[gogerald1, 2017-06-30 18:36:47]

Patchset #1 (id:40001) has been deleted

[gogerald1, 2017-06-30 20:25:58]

Description was changed from

==========
Present web payment app instead of instruments to user

BUG=735063
==========

to

==========
Present web payment app instead of instruments to user

Test video:
https://drive.google.com/file/d/0B4yF5XJG4VQAMEFaNzV1Y2t4ODQ/view?usp=sharing

BUG=735063
==========

[gogerald1, 2017-06-30 20:26:06]

Patchset #1 (id:60001) has been deleted

[gogerald1, 2017-06-30 20:28:36]

Description was changed from

==========
Present web payment app instead of instruments to user

Test video:
https://drive.google.com/file/d/0B4yF5XJG4VQAMEFaNzV1Y2t4ODQ/view?usp=sharing

BUG=735063
==========

to

==========
Present web payment app instead of instruments to user

Test video:
https://drive.google.com/file/d/0B4yF5XJG4VQAMEFaNzV1Y2t4ODQ/view?usp=sharing

This CL is based on:
https://codereview.chromium.org/2958333002/

BUG=735063
==========

[gogerald1, 2017-06-30 20:28:56]

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

[gogerald1, 2017-06-30 20:28:57]

Hi rouslan@, ptal,

[please use gerrit instead, 2017-06-30 20:30:22]

Can you use the origin of the service worker as the sub-label for the payment
app?

[gogerald1, 2017-06-30 20:36:08]

On 2017/06/30 20:30:22, rouslan wrote:
> Can you use the origin of the service worker as the sub-label for the payment
> app?

I was planning to do it in a separate CL since we have to consider when the web
manifest is not available. We might could simply compare label and sublabel to
distinguish them, but I preferred to do it separately carefully, do you think is
that good?

[please use gerrit instead, 2017-06-30 20:37:25]

All service worker payment apps should display origin as sub-label, regardless
of availability of the manifest. You can do this in a follow up CL.

[gogerald1, 2017-07-05 11:41:35]

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

[commit-bot: I haz the power, 2017-07-05 11:41:44]

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-07-05 11:43:54]

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

[commit-bot: I haz the power, 2017-07-05 11:43:55]

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

[gogerald1, 2017-07-05 12:00:51]

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

[commit-bot: I haz the power, 2017-07-05 12:00:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gogerald1, 2017-07-05 12:02:38]

On 2017/06/30 20:37:25, rouslan wrote:
> All service worker payment apps should display origin as sub-label, regardless
> of availability of the manifest. You can do this in a follow up CL.

Yes, but do you think is it good to display the origin two times?

BTW, you can review this CL now, the depending CL has been landed,

[commit-bot: I haz the power, 2017-07-05 12:14:28]

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

[commit-bot: I haz the power, 2017-07-05 12:14:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_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-07-05 12:58:18]

On 2017/07/05 12:02:38, gogerald1 wrote:
> do you think is it good to display the origin two times?

No, the origin should be displayed only once. Something like this:

boolean hasAppName = !TextUtils.isEmpty(appName);
String label = hasAppName ? appName : origin;
String subLabel = hasAppName ? origin : null;

[please use gerrit instead, 2017-07-05 13:14:22]

Good job!

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

https://codereview.chromium.org/2969713002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java:34:
private final long mSWRegistrationId;
This is a service worker, so the "SW" part of the variable name is redundant.

https://codereview.chromium.org/2969713002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java:57:
for (String method : methodNames) {
Use int loops. Foreach loops are not efficient.

https://codereview.chromium.org/2969713002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java:84:
return true;
While you're here, please replace these two lines with something like this:

        Set<String> methodNames = new HashSet<>(methodsAndData.keySet());
        methodNames.retainAll(mMethodNames);
        return !methodNames.isEmpty();

https://codereview.chromium.org/2969713002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java:89:
return "Chrome_Service_Worker_Payment_App";
This is used for frecency comparison. Must be swRegistrationId.

https://codereview.chromium.org/2969713002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java:109:
new HashSet<>(modifiers.values()), "", callback);
Add a comment about what "" is.

https://codereview.chromium.org/2969713002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_app_database.cc (right):

https://codereview.chromium.org/2969713002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_database.cc:373:
apps[origin]->enabled_methods.push_back(method);
That's very strange. Shouldn't service workers be keyed on "scope" [1], not
"origin"? The same "origin" can actually have multiple service workers, as far
as I know.

[1]
https://developer.mozilla.org/en-US/docs/Web/API/Service_Worker_API/Using_Ser...

https://codereview.chromium.org/2969713002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_app_info_fetcher.cc (right):

https://codereview.chromium.org/2969713002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_info_fetcher.cc:25: const int
kPaymentAppIdealIconSize = 0xFFFF;
Why jump from 64 to 65535?

[gogerald1, 2017-07-05 19:37:17]

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

[commit-bot: I haz the power, 2017-07-05 19:37:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gogerald1, 2017-07-05 19:48:07]

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

[gogerald1, 2017-07-05 19:48:12]

Patchset #3 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-07-05 19:48:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gogerald1, 2017-07-05 19:49:39]

another look?

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

https://codereview.chromium.org/2969713002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java:34:
private final long mSWRegistrationId;
On 2017/07/05 13:14:21, rouslan wrote:
> This is a service worker, so the "SW" part of the variable name is redundant.

Done.

https://codereview.chromium.org/2969713002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java:57:
for (String method : methodNames) {
On 2017/07/05 13:14:21, rouslan wrote:
> Use int loops. Foreach loops are not efficient.

Done.

https://codereview.chromium.org/2969713002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java:84:
return true;
On 2017/07/05 13:14:22, rouslan wrote:
> While you're here, please replace these two lines with something like this:
> 
>         Set<String> methodNames = new HashSet<>(methodsAndData.keySet());
>         methodNames.retainAll(mMethodNames);
>         return !methodNames.isEmpty();

Done.

https://codereview.chromium.org/2969713002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java:89:
return "Chrome_Service_Worker_Payment_App";
On 2017/07/05 13:14:21, rouslan wrote:
> This is used for frecency comparison. Must be swRegistrationId.

I think registration Id is not associated with a payment app when users can
register and unregister service worker.

I will make it as label + sublabel (name + origin) in the next CL.

https://codereview.chromium.org/2969713002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java:109:
new HashSet<>(modifiers.values()), "", callback);
On 2017/07/05 13:14:21, rouslan wrote:
> Add a comment about what "" is.

Done. Removed this unused variable

https://codereview.chromium.org/2969713002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_app_database.cc (right):

https://codereview.chromium.org/2969713002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_database.cc:373:
apps[origin]->enabled_methods.push_back(method);
On 2017/07/05 13:14:22, rouslan wrote:
> That's very strange. Shouldn't service workers be keyed on "scope" [1], not
> "origin"? The same "origin" can actually have multiple service workers, as far
> as I know.
> 
> [1]
>
https://developer.mozilla.org/en-US/docs/Web/API/Service_Worker_API/Using_Ser...

I think this is fine. A origin may have multiple service workers for different
scopes, but each origin should have only one service worker as payment app,
right? Otherwise we should also consider how to present them.

Anyway, I think we could optimize it in a separate CL.

https://codereview.chromium.org/2969713002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_app_info_fetcher.cc (right):

https://codereview.chromium.org/2969713002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_info_fetcher.cc:25: const int
kPaymentAppIdealIconSize = 0xFFFF;
On 2017/07/05 13:14:22, rouslan wrote:
> Why jump from 64 to 65535?

As I mentioned in the above comments. 64 pixels is small which will cause the
image be scaled down. So I am choosing a large enough value for all platforms.

[please use gerrit instead, 2017-07-05 19:53:20]

lgtm

[gogerald1, 2017-07-05 20:02:03]

gogerald@chromium.org changed reviewers:
+ piman@chromium.org

[gogerald1, 2017-07-05 20:02:04]

Hi piman@ please review changes in c/p/browser/*

[piman, 2017-07-05 20:43:50]

lgtm

[gogerald1, 2017-07-05 20:52:22]

The CQ bit was unchecked by gogerald@chromium.org

[gogerald1, 2017-07-05 20:52:27]

The CQ bit was checked by gogerald@chromium.org

[commit-bot: I haz the power, 2017-07-05 20:52:42]

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-07-05 23:08:34]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1499287947417450,
"parent_rev": "4c07b9435b75504cebcff2d82c696f8800f0bccd", "commit_rev":
"ad5b9e54353ce83b4805f4d3e9ba5cc5fec90102"}

[commit-bot: I haz the power, 2017-07-05 23:08:52]

Description was changed from

==========
Present web payment app instead of instruments to user

Test video:
https://drive.google.com/file/d/0B4yF5XJG4VQAMEFaNzV1Y2t4ODQ/view?usp=sharing

This CL is based on:
https://codereview.chromium.org/2958333002/

BUG=735063
==========

to

==========
Present web payment app instead of instruments to user

Test video:
https://drive.google.com/file/d/0B4yF5XJG4VQAMEFaNzV1Y2t4ODQ/view?usp=sharing

This CL is based on:
https://codereview.chromium.org/2958333002/

BUG=735063

Review-Url: https://codereview.chromium.org/2969713002
Cr-Commit-Position: refs/heads/master@{#484368}
Committed:
https://chromium.googlesource.com/chromium/src/+/ad5b9e54353ce83b4805f4d3e9ba...
==========

[commit-bot: I haz the power, 2017-07-05 23:08:54]

Committed patchset #3 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/ad5b9e54353ce83b4805f4d3e9ba...