Verify behavior of PaymentRequest.show() method, part 1.

Verify behavior of PaymentRequest.show() method, part 1.

Before this patch, Chrome on Android would allow only one instance of
PaymentRequest per browser process to be displayed, but desktop had no
such restriction.

This patch adds a check for currently displaying PaymentRequest UI on
desktop. The currently displayed instance of PaymentRequest is kept
track of in PaymentRequestWebContentsManager.

After this patch, Chrome on desktop allows only one instance of
PaymentRequest per tab to be displayed.

From https://w3c.github.io/browser-payment-api/#show-method
"If the user agent's "payment request is showing" boolean is true, then
 return a promise rejected with an "AbortError" DOMException."

From https://w3c.github.io/browser-payment-api/#dfn-payment-request-is-showing
"The user agent as a whole has a single "payment request is showing"
 boolean, initially false. This is used to prevent multiple
 PaymentRequests from being shown, via their show() method, at the same
 time."

BUG=705252
Review-Url: https://codereview.chromium.org/2864013002
Cr-Commit-Position: refs/heads/master@{#469988}
Committed: https://chromium.googlesource.com/chromium/src/+/7d433cc2ac0b3e36e6753e84fbfc9253aac646d8

[please use gerrit instead, 2017-05-05 17:10:57]

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

[commit-bot: I haz the power, 2017-05-05 17:11:37]

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-05-05 17:22:11]

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

[commit-bot: I haz the power, 2017-05-05 17:22:37]

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-05-05 17:24:57]

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

[please use gerrit instead, 2017-05-05 17:24:58]

Marijn, ptal LayoutTests.

Mathieu, ptal C++.

[Marijn Kruisselbrink, 2017-05-05 17:53:20]

https://codereview.chromium.org/2864013002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/payment-request/payment-request-show-method.https.html
(right):

https://codereview.chromium.org/2864013002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/payment-request/payment-request-show-method.https.html:4:
<title>Test for PaymentRequest Constructor</title>
copy/past error?

https://codereview.chromium.org/2864013002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/payment-request/payment-request-show-method.https.html:35:
request2.show().catch(t.step_func(e => {
you could rewrite this part as:
promise_rejects(t, 'AbortError', request2.show()).then(t.step_func(() =>
request1.abort()));

And I wonder if it would make sense to change the test to use promise_test
rather than async_test. Especially if in the future you want to add more async
show() tests, using promise_test would ensure tests are run serially rather than
potentially overlapping. Something like:

promise_test(t => {
  const request1 = ...;
  const request2 = ...;
  const result = promise_rejects(t, null, request1.show());
  promise_rejects(t, 'AbortError', request2.show())
    .then(t.step_func(() => request1.abort()));
  return result;
}, '...');

[please use gerrit instead, 2017-05-05 18:16:27]

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

[please use gerrit instead, 2017-05-05 18:16:33]

Marijn, ptal patch 3.

https://codereview.chromium.org/2864013002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/payment-request/payment-request-show-method.https.html
(right):

https://codereview.chromium.org/2864013002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/payment-request/payment-request-show-method.https.html:4:
<title>Test for PaymentRequest Constructor</title>
On 2017/05/05 17:53:19, Marijn Kruisselbrink wrote:
> copy/past error?

Done.

https://codereview.chromium.org/2864013002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/payment-request/payment-request-show-method.https.html:35:
request2.show().catch(t.step_func(e => {
On 2017/05/05 17:53:19, Marijn Kruisselbrink wrote:
> you could rewrite this part as:
> promise_rejects(t, 'AbortError', request2.show()).then(t.step_func(() =>
> request1.abort()));
> 
> And I wonder if it would make sense to change the test to use promise_test
> rather than async_test. Especially if in the future you want to add more async
> show() tests, using promise_test would ensure tests are run serially rather
than
> potentially overlapping. Something like:
> 
> promise_test(t => {
>   const request1 = ...;
>   const request2 = ...;
>   const result = promise_rejects(t, null, request1.show());
>   promise_rejects(t, 'AbortError', request2.show())
>     .then(t.step_func(() => request1.abort()));
>   return result;
> }, '...');

Done.

[commit-bot: I haz the power, 2017-05-05 18:16:47]

Dry run: CQ is trying da patch.

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

[Marijn Kruisselbrink, 2017-05-05 18:20:05]

lgtm

[commit-bot: I haz the power, 2017-05-05 22:04:53]

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

[commit-bot: I haz the power, 2017-05-05 22:04:54]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-05-08 13:01:42]

lgtm, small stuffs

https://codereview.chromium.org/2864013002/diff/40001/components/payments/con...
File components/payments/content/payment_request.cc (right):

https://codereview.chromium.org/2864013002/diff/40001/components/payments/con...
components/payments/content/payment_request.cc:106: client_.reset();
I'm curious why not OnConnectionTerminated here? I'm pretty sure CloseDialog
would do nothing, if you were worried about that.

https://codereview.chromium.org/2864013002/diff/40001/components/payments/con...
File components/payments/content/payment_request_web_contents_manager.h (right):

https://codereview.chromium.org/2864013002/diff/40001/components/payments/con...
components/payments/content/payment_request_web_contents_manager.h:54: //
|request| can be shown. (Only one request at a time can be shown per tab.)
you should add that this manager will now assume that |request| is showing (the
name CanShow is misleading because it has side effects. WillShow could be better
although I'm bad with names).

https://codereview.chromium.org/2864013002/diff/40001/components/payments/con...
components/payments/content/payment_request_web_contents_manager.h:66:
std::unordered_map<PaymentRequest*, std::unique_ptr<PaymentRequest>>
per Brett's email to chromium-dev, unordered_map is not efficient. Let's use
std::map

https://chromium.googlesource.com/chromium/src/+/master/base/containers/READM...

[please use gerrit instead, 2017-05-08 13:12:35]

Sending to cq.

https://codereview.chromium.org/2864013002/diff/40001/components/payments/con...
File components/payments/content/payment_request.cc (right):

https://codereview.chromium.org/2864013002/diff/40001/components/payments/con...
components/payments/content/payment_request.cc:106: client_.reset();
On 2017/05/08 13:01:42, Mathieu wrote:
> I'm curious why not OnConnectionTerminated here? I'm pretty sure CloseDialog
> would do nothing, if you were worried about that.

Done.

https://codereview.chromium.org/2864013002/diff/40001/components/payments/con...
File components/payments/content/payment_request_web_contents_manager.h (right):

https://codereview.chromium.org/2864013002/diff/40001/components/payments/con...
components/payments/content/payment_request_web_contents_manager.h:54: //
|request| can be shown. (Only one request at a time can be shown per tab.)
On 2017/05/08 13:01:42, Mathieu wrote:
> you should add that this manager will now assume that |request| is showing
(the
> name CanShow is misleading because it has side effects. WillShow could be
better
> although I'm bad with names).

Done.

https://codereview.chromium.org/2864013002/diff/40001/components/payments/con...
components/payments/content/payment_request_web_contents_manager.h:66:
std::unordered_map<PaymentRequest*, std::unique_ptr<PaymentRequest>>
On 2017/05/08 13:01:42, Mathieu wrote:
> per Brett's email to chromium-dev, unordered_map is not efficient. Let's use
> std::map
> 
>
https://chromium.googlesource.com/chromium/src/+/master/base/containers/READM...

Done.

[please use gerrit instead, 2017-05-08 13:13:58]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2017-05-08 13:13:58]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
mek@chromium.org
Link to the patchset: https://codereview.chromium.org/2864013002/#ps60001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-05-08 13:14:06]

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-05-08 15:18:10]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494249238423070,
"parent_rev": "8511820ec8280caacbd4f81f3ecd13b6c61681b0", "commit_rev":
"7d433cc2ac0b3e36e6753e84fbfc9253aac646d8"}

[commit-bot: I haz the power, 2017-05-08 15:18:25]

Description was changed from

==========
Verify behavior of PaymentRequest.show() method, part 1.

Before this patch, Chrome on Android would allow only one instance of
PaymentRequest per browser process to be displayed, but desktop had no
such restriction.

This patch adds a check for currently displaying PaymentRequest UI on
desktop. The currently displayed instance of PaymentRequest is kept
track of in PaymentRequestWebContentsManager.

After this patch, Chrome on desktop allows only one instance of
PaymentRequest per tab to be displayed.

From https://w3c.github.io/browser-payment-api/#show-method
"If the user agent's "payment request is showing" boolean is true, then
 return a promise rejected with an "AbortError" DOMException."

From https://w3c.github.io/browser-payment-api/#dfn-payment-request-is-showing
"The user agent as a whole has a single "payment request is showing"
 boolean, initially false. This is used to prevent multiple
 PaymentRequests from being shown, via their show() method, at the same
 time."

BUG=705252
==========

to

==========
Verify behavior of PaymentRequest.show() method, part 1.

Before this patch, Chrome on Android would allow only one instance of
PaymentRequest per browser process to be displayed, but desktop had no
such restriction.

This patch adds a check for currently displaying PaymentRequest UI on
desktop. The currently displayed instance of PaymentRequest is kept
track of in PaymentRequestWebContentsManager.

After this patch, Chrome on desktop allows only one instance of
PaymentRequest per tab to be displayed.

From https://w3c.github.io/browser-payment-api/#show-method
"If the user agent's "payment request is showing" boolean is true, then
 return a promise rejected with an "AbortError" DOMException."

From https://w3c.github.io/browser-payment-api/#dfn-payment-request-is-showing
"The user agent as a whole has a single "payment request is showing"
 boolean, initially false. This is used to prevent multiple
 PaymentRequests from being shown, via their show() method, at the same
 time."

BUG=705252

Review-Url: https://codereview.chromium.org/2864013002
Cr-Commit-Position: refs/heads/master@{#469988}
Committed:
https://chromium.googlesource.com/chromium/src/+/7d433cc2ac0b3e36e6753e84fbfc...
==========

[commit-bot: I haz the power, 2017-05-08 15:18:28]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/7d433cc2ac0b3e36e6753e84fbfc...