[Payments] Improve the closing of the PR dialog.

[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the
  dialog. This will need to happen if the Mojo pipe is closed by the
  renderer (can happen on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit
  action, the PaymentRequest object will inform the renderer and
  subsequently self destruct (this is not new but is improved)
* Some logic is added to avoid cycles: if the dialog is closing and it
  informs PaymentRequest, PaymentRequest will not ask to close it again
  (and vice versa). This is done through closing the bindings.
* abort() is implemented, it currently simply returns onAbort(true) to
  the caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload,
  etc, the dialog will now close too.
* PaymentRequestDialog is renamed PaymentRequestDialogView and
  implements a new interface, PaymentRequestDialog.
* The tests are now a WidgetObserver to be warned of all possible
  closures of the dialog.

BUG=683731, 679245
TEST=PaymentRequest interactive_ui_tests

Review-Url: https://codereview.chromium.org/2649683002
Cr-Commit-Position: refs/heads/master@{#445652}
Committed: https://chromium.googlesource.com/chromium/src/+/f4bc50e24731fad2ef8ac320f6347aa60fe70532

[Mathieu, 2017-01-22 20:57:50]

Description was changed from

==========
[Payments] Improve the closing of the PR dialog.

Will now close on navigation, reload, and abort() called on the PR promise.

BUG=
TEST=PaymentRequest interactive_ui_tests
==========

to

==========
[Payments] Improve the closing of the PR dialog.

Various changes bundled here:
* The PaymentRequest object, through its delegate, can now close the dialog.
This will need to happen if the Mojo pipe is closed by the renderer (can happen
on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit action, the
PaymentRequest object will inform the renderer and subsequently self destruct.


Will now close on navigation, reload, and abort() called on the PR promise.

BUG=683731
TEST=PaymentRequest interactive_ui_tests
==========

[Mathieu, 2017-01-22 21:09:19]

Description was changed from

==========
[Payments] Improve the closing of the PR dialog.

Various changes bundled here:
* The PaymentRequest object, through its delegate, can now close the dialog.
This will need to happen if the Mojo pipe is closed by the renderer (can happen
on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit action, the
PaymentRequest object will inform the renderer and subsequently self destruct.


Will now close on navigation, reload, and abort() called on the PR promise.

BUG=683731
TEST=PaymentRequest interactive_ui_tests
==========

to

==========
[Payments] Improve the closing of the PR dialog.

Various changes bundled here:
* The PaymentRequest object, through its delegate, can now close the dialog.
This will need to happen if the Mojo pipe is closed by the renderer (can happen
on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit action, the
PaymentRequest object will inform the renderer and subsequently self destruct.
* Some logic is added to avoid cycles: if the dialog is closing, PaymentRequest
will not ask to close it again (and vice versa).
* abort() is implemented, it currently simply returns onAbort(true) to the
caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload, etc, the
dialog will now close too.

BUG=683731
TEST=PaymentRequest interactive_ui_tests
==========

[Mathieu, 2017-01-22 21:09:19]

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

[Mathieu, 2017-01-22 21:09:48]

Description was changed from

==========
[Payments] Improve the closing of the PR dialog.

Various changes bundled here:
* The PaymentRequest object, through its delegate, can now close the dialog.
This will need to happen if the Mojo pipe is closed by the renderer (can happen
on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit action, the
PaymentRequest object will inform the renderer and subsequently self destruct.
* Some logic is added to avoid cycles: if the dialog is closing, PaymentRequest
will not ask to close it again (and vice versa).
* abort() is implemented, it currently simply returns onAbort(true) to the
caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload, etc, the
dialog will now close too.

BUG=683731
TEST=PaymentRequest interactive_ui_tests
==========

to

==========
[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the dialog.
This will need to happen if the Mojo pipe is closed by the renderer (can happen
on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit action, the
PaymentRequest object will inform the renderer and subsequently self destruct.
* Some logic is added to avoid cycles: if the dialog is closing, PaymentRequest
will not ask to close it again (and vice versa).
* abort() is implemented, it currently simply returns onAbort(true) to the
caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload, etc, the
dialog will now close too.

BUG=683731
TEST=PaymentRequest interactive_ui_tests
==========

[Mathieu, 2017-01-22 21:31:36]

Description was changed from

==========
[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the dialog.
This will need to happen if the Mojo pipe is closed by the renderer (can happen
on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit action, the
PaymentRequest object will inform the renderer and subsequently self destruct.
* Some logic is added to avoid cycles: if the dialog is closing, PaymentRequest
will not ask to close it again (and vice versa).
* abort() is implemented, it currently simply returns onAbort(true) to the
caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload, etc, the
dialog will now close too.

BUG=683731
TEST=PaymentRequest interactive_ui_tests
==========

to

==========
[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the dialog.
This will need to happen if the Mojo pipe is closed by the renderer (can happen
on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit action, the
PaymentRequest object will inform the renderer and subsequently self destruct
(this is not new but is improved)
* Some logic is added to avoid cycles: if the dialog is closing, PaymentRequest
will not ask to close it again (and vice versa).
* abort() is implemented, it currently simply returns onAbort(true) to the
caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload, etc, the
dialog will now close too.

BUG=683731
TEST=PaymentRequest interactive_ui_tests
==========

[Mathieu, 2017-01-22 21:32:10]

Description was changed from

==========
[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the dialog.
This will need to happen if the Mojo pipe is closed by the renderer (can happen
on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit action, the
PaymentRequest object will inform the renderer and subsequently self destruct
(this is not new but is improved)
* Some logic is added to avoid cycles: if the dialog is closing, PaymentRequest
will not ask to close it again (and vice versa).
* abort() is implemented, it currently simply returns onAbort(true) to the
caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload, etc, the
dialog will now close too.

BUG=683731
TEST=PaymentRequest interactive_ui_tests
==========

to

==========
[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the dialog.
This will need to happen if the Mojo pipe is closed by the renderer (can happen
on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit action, the
PaymentRequest object will inform the renderer and subsequently self destruct
(this is not new but is improved)
* Some logic is added to avoid cycles: if the dialog is closing and it informs
PaymentRequest, PaymentRequest will not ask to close it again (and vice versa).
* abort() is implemented, it currently simply returns onAbort(true) to the
caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload, etc, the
dialog will now close too.

BUG=683731
TEST=PaymentRequest interactive_ui_tests
==========

[Mathieu, 2017-01-22 21:47:17]

Description was changed from

==========
[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the dialog.
This will need to happen if the Mojo pipe is closed by the renderer (can happen
on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit action, the
PaymentRequest object will inform the renderer and subsequently self destruct
(this is not new but is improved)
* Some logic is added to avoid cycles: if the dialog is closing and it informs
PaymentRequest, PaymentRequest will not ask to close it again (and vice versa).
* abort() is implemented, it currently simply returns onAbort(true) to the
caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload, etc, the
dialog will now close too.

BUG=683731
TEST=PaymentRequest interactive_ui_tests
==========

to

==========
[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the dialog.
This will need to happen if the Mojo pipe is closed by the renderer (can happen
on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit action, the
PaymentRequest object will inform the renderer and subsequently self destruct
(this is not new but is improved)
* Some logic is added to avoid cycles: if the dialog is closing and it informs
PaymentRequest, PaymentRequest will not ask to close it again (and vice versa).
* abort() is implemented, it currently simply returns onAbort(true) to the
caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload, etc, the
dialog will now close too.
* PaymentRequestDialog is renamed PaymentRequestDialogView and implements a new
interface, PaymentRequestDialog.
* The tests are now a WidgetObserver to be warned of all possible closures of
the dialog.

BUG=683731
TEST=PaymentRequest interactive_ui_tests
==========

[Mathieu, 2017-01-22 21:47:51]

Hi, PTAL

Rouslan: everything :)

sky: browser/ui review

[Mathieu, 2017-01-23 12:30:52]

Patchset #1 (id:1) has been deleted

[Mathieu, 2017-01-23 12:31:01]

Patchset #1 (id:20001) has been deleted

[Mathieu, 2017-01-23 12:31:12]

Patchset #1 (id:40001) has been deleted

[please use gerrit instead, 2017-01-23 13:39:31]

Description was changed from

==========
[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the dialog.
This will need to happen if the Mojo pipe is closed by the renderer (can happen
on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit action, the
PaymentRequest object will inform the renderer and subsequently self destruct
(this is not new but is improved)
* Some logic is added to avoid cycles: if the dialog is closing and it informs
PaymentRequest, PaymentRequest will not ask to close it again (and vice versa).
* abort() is implemented, it currently simply returns onAbort(true) to the
caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload, etc, the
dialog will now close too.
* PaymentRequestDialog is renamed PaymentRequestDialogView and implements a new
interface, PaymentRequestDialog.
* The tests are now a WidgetObserver to be warned of all possible closures of
the dialog.

BUG=683731
TEST=PaymentRequest interactive_ui_tests
==========

to

==========
[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the
  dialog. This will need to happen if the Mojo pipe is closed by the
  renderer (can happen on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit
  action, the PaymentRequest object will inform the renderer and
  subsequently self destruct (this is not new but is improved)
* Some logic is added to avoid cycles: if the dialog is closing and it
  informs PaymentRequest, PaymentRequest will not ask to close it again
  (and vice versa).
* abort() is implemented, it currently simply returns onAbort(true) to
  the caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload,
  etc, the dialog will now close too.
* PaymentRequestDialog is renamed PaymentRequestDialogView and
  implements a new interface, PaymentRequestDialog.
* The tests are now a WidgetObserver to be warned of all possible
  closures of the dialog.

BUG=683731
TEST=PaymentRequest interactive_ui_tests
==========

[please use gerrit instead, 2017-01-23 15:02:08]

So happy you're looking into this! We need this to be solid :-D

https://codereview.chromium.org/2649683002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_dialog_view.h (right):

https://codereview.chromium.org/2649683002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:62:
views::Widget* widget() { return widget_; }
widget_for_testing()

https://codereview.chromium.org/2649683002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:78:
ObserverForTest* observer_;
observer_for_testing_ to be consistent? Maybe good to group *for_testing
variables together.

https://codereview.chromium.org/2649683002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:81: // A
reference to the widget created when the dialog is shown.
Add a note that this widget is owned by its parent in the widget hierarchy. Also
add a note that this pointer is used only in tests.

https://codereview.chromium.org/2649683002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:82:
views::Widget* widget_;
widget_for_testing_

https://codereview.chromium.org/2649683002/diff/60001/components/payments/pay...
File components/payments/payment_request.cc (right):

https://codereview.chromium.org/2649683002/diff/60001/components/payments/pay...
components/payments/payment_request.cc:79: void
PaymentRequest::OnConnectionTerminated() {
Also need to close "client_" connection everywhere you close "binding_"
connection. PaymentRequest has two pipes to the renderer, unfortunately.

https://codereview.chromium.org/2649683002/diff/60001/components/payments/pay...
components/payments/payment_request.cc:85: binding_.Close();
I have a hunch that "binding_.is_bound()" is equiavlent to
"!user_cancelled_dialog_". Can you double-check? If so, then we don't need the
user_cancelled_dialog_ variable.

We can also withstand a call to UserCancelled() if we check for
"client_.is_bound()" and "binding_.is_bound()" in there. Then we also don't need
the "propagate_to_renderer" flag. What do you think?

As a general rule of thumb #1, every method that's going to talk to UI and finds
UI gone should abort communications with renderer, because it's misbehaving.

General rule of thumb #2, every method that's going to the renderer first should
check that both pipes to the renderer are open first. We can't tell anything to
a renderer, if we're not connected to it.

[Mathieu, 2017-01-23 16:06:39]

Description was changed from

==========
[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the
  dialog. This will need to happen if the Mojo pipe is closed by the
  renderer (can happen on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit
  action, the PaymentRequest object will inform the renderer and
  subsequently self destruct (this is not new but is improved)
* Some logic is added to avoid cycles: if the dialog is closing and it
  informs PaymentRequest, PaymentRequest will not ask to close it again
  (and vice versa).
* abort() is implemented, it currently simply returns onAbort(true) to
  the caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload,
  etc, the dialog will now close too.
* PaymentRequestDialog is renamed PaymentRequestDialogView and
  implements a new interface, PaymentRequestDialog.
* The tests are now a WidgetObserver to be warned of all possible
  closures of the dialog.

BUG=683731
TEST=PaymentRequest interactive_ui_tests
==========

to

==========
[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the
  dialog. This will need to happen if the Mojo pipe is closed by the
  renderer (can happen on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit
  action, the PaymentRequest object will inform the renderer and
  subsequently self destruct (this is not new but is improved)
* Some logic is added to avoid cycles: if the dialog is closing and it
  informs PaymentRequest, PaymentRequest will not ask to close it again
  (and vice versa).
* abort() is implemented, it currently simply returns onAbort(true) to
  the caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload,
  etc, the dialog will now close too.
* PaymentRequestDialog is renamed PaymentRequestDialogView and
  implements a new interface, PaymentRequestDialog.
* The tests are now a WidgetObserver to be warned of all possible
  closures of the dialog.

BUG=683731, 679245
TEST=PaymentRequest interactive_ui_tests
==========

[Mathieu, 2017-01-23 20:57:38]

Patchset #2 (id:80001) has been deleted

[Mathieu, 2017-01-23 20:58:03]

Description was changed from

==========
[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the
  dialog. This will need to happen if the Mojo pipe is closed by the
  renderer (can happen on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit
  action, the PaymentRequest object will inform the renderer and
  subsequently self destruct (this is not new but is improved)
* Some logic is added to avoid cycles: if the dialog is closing and it
  informs PaymentRequest, PaymentRequest will not ask to close it again
  (and vice versa).
* abort() is implemented, it currently simply returns onAbort(true) to
  the caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload,
  etc, the dialog will now close too.
* PaymentRequestDialog is renamed PaymentRequestDialogView and
  implements a new interface, PaymentRequestDialog.
* The tests are now a WidgetObserver to be warned of all possible
  closures of the dialog.

BUG=683731, 679245
TEST=PaymentRequest interactive_ui_tests
==========

to

==========
[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the
  dialog. This will need to happen if the Mojo pipe is closed by the
  renderer (can happen on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit
  action, the PaymentRequest object will inform the renderer and
  subsequently self destruct (this is not new but is improved)
* Some logic is added to avoid cycles: if the dialog is closing and it
  informs PaymentRequest, PaymentRequest will not ask to close it again
  (and vice versa). This is done through closing the bindings.
* abort() is implemented, it currently simply returns onAbort(true) to
  the caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload,
  etc, the dialog will now close too.
* PaymentRequestDialog is renamed PaymentRequestDialogView and
  implements a new interface, PaymentRequestDialog.
* The tests are now a WidgetObserver to be warned of all possible
  closures of the dialog.

BUG=683731, 679245
TEST=PaymentRequest interactive_ui_tests
==========

[Mathieu, 2017-01-23 20:58:36]

Thanks Rouslan, I think everything is covered, PTAL!

https://codereview.chromium.org/2649683002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_dialog_view.h (right):

https://codereview.chromium.org/2649683002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:62:
views::Widget* widget() { return widget_; }
On 2017/01/23 15:02:08, rouslan wrote:
> widget_for_testing()

Done.

https://codereview.chromium.org/2649683002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:78:
ObserverForTest* observer_;
On 2017/01/23 15:02:08, rouslan wrote:
> observer_for_testing_ to be consistent? Maybe good to group *for_testing
> variables together.

Done.

https://codereview.chromium.org/2649683002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:81: // A
reference to the widget created when the dialog is shown.
On 2017/01/23 15:02:08, rouslan wrote:
> Add a note that this widget is owned by its parent in the widget hierarchy.
Also
> add a note that this pointer is used only in tests.

Done.

https://codereview.chromium.org/2649683002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:82:
views::Widget* widget_;
On 2017/01/23 15:02:08, rouslan wrote:
> widget_for_testing_

Done.

https://codereview.chromium.org/2649683002/diff/60001/components/payments/pay...
File components/payments/payment_request.cc (right):

https://codereview.chromium.org/2649683002/diff/60001/components/payments/pay...
components/payments/payment_request.cc:79: void
PaymentRequest::OnConnectionTerminated() {
On 2017/01/23 15:02:08, rouslan wrote:
> Also need to close "client_" connection everywhere you close "binding_"
> connection. PaymentRequest has two pipes to the renderer, unfortunately.

Done.

https://codereview.chromium.org/2649683002/diff/60001/components/payments/pay...
components/payments/payment_request.cc:85: binding_.Close();
On 2017/01/23 15:02:08, rouslan wrote:
> I have a hunch that "binding_.is_bound()" is equiavlent to
> "!user_cancelled_dialog_". Can you double-check? If so, then we don't need the
> user_cancelled_dialog_ variable.
> 
> We can also withstand a call to UserCancelled() if we check for
> "client_.is_bound()" and "binding_.is_bound()" in there. Then we also don't
need
> the "propagate_to_renderer" flag. What do you think?
> 
> As a general rule of thumb #1, every method that's going to talk to UI and
finds
> UI gone should abort communications with renderer, because it's misbehaving.
> 
> General rule of thumb #2, every method that's going to the renderer first
should
> check that both pipes to the renderer are open first. We can't tell anything
to
> a renderer, if we're not connected to it.

I rearranged things so that when UserCancelled is called, the renderer is
informed and bindings are closed afterwards (+PaymentRequest destroyed). It
avoids waiting for the OnConnectionTerminated message to achieve the same. By
the way if the renderer closes the binding, |binding_.is_bound()| stays true on
the browser side.

Applied rule #2 to check for client_.is_bound() before calling methods on
|client_|.

Removed the propagate_to_renderer flag because UserCancelled aborts when the
client is not bound.

[please use gerrit instead, 2017-01-23 21:18:50]

LGTM % nits

Wonderful code! :-D

https://codereview.chromium.org/2649683002/diff/100001/chrome/browser/payment...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2649683002/diff/100001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:19: void
ChromePaymentRequestDelegate::ShowDialog(PaymentRequest* request) {
nit: Would it be a good idea to add "DCHECK_EQ(dialog_, nullptr)"?

https://codereview.chromium.org/2649683002/diff/100001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:25: if (dialog_) {
nit: no {}

https://codereview.chromium.org/2649683002/diff/100001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:27: }
nit: would it be a good idea to add "dialog_ = nullptr"?

https://codereview.chromium.org/2649683002/diff/100001/components/payments/pa...
File components/payments/payment_request.cc (right):

https://codereview.chromium.org/2649683002/diff/100001/components/payments/pa...
components/payments/payment_request.cc:54: if (!client_.is_bound() ||
!binding_.is_bound()) {
Nit for the future: If the dialog is already showing, it would be nice to
terminate the connection to the renderer and close the dialog.

https://codereview.chromium.org/2649683002/diff/100001/components/payments/pa...
components/payments/payment_request.cc:82: manager_->DestroyRequest(this);
This line should be called unconditionally, even if the renderer connection is
gone, right?

[Mathieu, 2017-01-23 21:53:43]

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

[Mathieu, 2017-01-23 21:53:58]

Thanks!

https://codereview.chromium.org/2649683002/diff/100001/chrome/browser/payment...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2649683002/diff/100001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:19: void
ChromePaymentRequestDelegate::ShowDialog(PaymentRequest* request) {
On 2017/01/23 21:18:50, rouslan wrote:
> nit: Would it be a good idea to add "DCHECK_EQ(dialog_, nullptr)"?

Done.

https://codereview.chromium.org/2649683002/diff/100001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:25: if (dialog_) {
On 2017/01/23 21:18:50, rouslan wrote:
> nit: no {}

Done.

https://codereview.chromium.org/2649683002/diff/100001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:27: }
On 2017/01/23 21:18:50, rouslan wrote:
> nit: would it be a good idea to add "dialog_ = nullptr"?

Yes that's not a bad idea. Gets you back into a better state.

https://codereview.chromium.org/2649683002/diff/100001/components/payments/pa...
File components/payments/payment_request.cc (right):

https://codereview.chromium.org/2649683002/diff/100001/components/payments/pa...
components/payments/payment_request.cc:54: if (!client_.is_bound() ||
!binding_.is_bound()) {
On 2017/01/23 21:18:50, rouslan wrote:
> Nit for the future: If the dialog is already showing, it would be nice to
> terminate the connection to the renderer and close the dialog.

Acknowledged.

https://codereview.chromium.org/2649683002/diff/100001/components/payments/pa...
components/payments/payment_request.cc:82: manager_->DestroyRequest(this);
On 2017/01/23 21:18:50, rouslan wrote:
> This line should be called unconditionally, even if the renderer connection is
> gone, right?

Not really. If we are calling this method while the client is not bound it means
that it was closed as part of OnConnectionTerminated, which means destruction
was already requested on line 94.

[commit-bot: I haz the power, 2017-01-23 21:54:49]

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

[sky, 2017-01-24 00:45:02]

https://codereview.chromium.org/2649683002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_request_dialog_view.h (right):

https://codereview.chromium.org/2649683002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:62:
views::Widget* widget_for_testing() { return widget_for_testing_; }
Why can't you use GetWidget()?

https://codereview.chromium.org/2649683002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_request_interactive_uitest.cc
(right):

https://codereview.chromium.org/2649683002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_interactive_uitest.cc:46:
event_observer_.reset(new DialogEventObserver(DialogEvent::DIALOG_CLOSED));
MakeUnique where possible.

https://codereview.chromium.org/2649683002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.h
(right):

https://codereview.chromium.org/2649683002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.h:107:
std::unique_ptr<DialogEventObserver> event_observer_;
Style guide says members should be private.

[Mathieu, 2017-01-24 01:11:40]

Hi Scott, PTAL!

https://codereview.chromium.org/2649683002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_request_dialog_view.h (right):

https://codereview.chromium.org/2649683002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:62:
views::Widget* widget_for_testing() { return widget_for_testing_; }
On 2017/01/24 00:45:02, sky wrote:
> Why can't you use GetWidget()?

Thanks for that.

https://codereview.chromium.org/2649683002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_request_interactive_uitest.cc
(right):

https://codereview.chromium.org/2649683002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_interactive_uitest.cc:46:
event_observer_.reset(new DialogEventObserver(DialogEvent::DIALOG_CLOSED));
On 2017/01/24 00:45:02, sky wrote:
> MakeUnique where possible.

Acknowledged.

https://codereview.chromium.org/2649683002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.h
(right):

https://codereview.chromium.org/2649683002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.h:107:
std::unique_ptr<DialogEventObserver> event_observer_;
On 2017/01/24 00:45:02, sky wrote:
> Style guide says members should be private.

Done. Added ResetEventObserver and WaitForObservedEvent

[sky, 2017-01-24 03:09:38]

LGTM

[Mathieu, 2017-01-24 03:22:05]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-01-24 03:22:05]

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/2649683002/#ps140001
(title: "addressed comments from sky")

[commit-bot: I haz the power, 2017-01-24 03:22:31]

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

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485228125472580,
"parent_rev": "b156b16a00574a89d85ccb034b30cdfd9cc5676a", "commit_rev":
"f4bc50e24731fad2ef8ac320f6347aa60fe70532"}

[commit-bot: I haz the power, 2017-01-24 05:18:56]

Description was changed from

==========
[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the
  dialog. This will need to happen if the Mojo pipe is closed by the
  renderer (can happen on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit
  action, the PaymentRequest object will inform the renderer and
  subsequently self destruct (this is not new but is improved)
* Some logic is added to avoid cycles: if the dialog is closing and it
  informs PaymentRequest, PaymentRequest will not ask to close it again
  (and vice versa). This is done through closing the bindings.
* abort() is implemented, it currently simply returns onAbort(true) to
  the caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload,
  etc, the dialog will now close too.
* PaymentRequestDialog is renamed PaymentRequestDialogView and
  implements a new interface, PaymentRequestDialog.
* The tests are now a WidgetObserver to be warned of all possible
  closures of the dialog.

BUG=683731, 679245
TEST=PaymentRequest interactive_ui_tests
==========

to

==========
[Payments] Improve the closing of the PR dialog.

Various changes around the dialog's disappearance.
* The PaymentRequest object, through its delegate, can now close the
  dialog. This will need to happen if the Mojo pipe is closed by the
  renderer (can happen on failure or success of the PR logic).
* Similarly, when the user closes the dialog through an explicit
  action, the PaymentRequest object will inform the renderer and
  subsequently self destruct (this is not new but is improved)
* Some logic is added to avoid cycles: if the dialog is closing and it
  informs PaymentRequest, PaymentRequest will not ask to close it again
  (and vice versa). This is done through closing the bindings.
* abort() is implemented, it currently simply returns onAbort(true) to
  the caller (to be improved).
* As a result of the Mojo connection closing on navigation, reload,
  etc, the dialog will now close too.
* PaymentRequestDialog is renamed PaymentRequestDialogView and
  implements a new interface, PaymentRequestDialog.
* The tests are now a WidgetObserver to be warned of all possible
  closures of the dialog.

BUG=683731, 679245
TEST=PaymentRequest interactive_ui_tests

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

[commit-bot: I haz the power, 2017-01-24 05:18:57]

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