[WebPayments] Move views-specific code to c/b/ui/views/

[WebPayments] Move views-specific code to c/b/ui/views/

This CL moves the PaymentRequestDialog class to c/b/ui/views/.
Moving it is required because:

a) It's views-specific, so convention dictates it should live in
   c/b/ui/views/
b) It will consume views specific code, which is disallowed from c/b/*

This also means that PaymentRequestImpl can't directly instantiate
PaymentRequestDialog anymore. This is solved by adding the
ShowWebPaymentsDialog function to BrowserWindow.

This CL also fixes some lifetime and ownership issues that became
apparent during the move. Specifically:

a) payments::mojom::PaymentRequestClientPtr should be owned by
   PaymentRequestImpl rather than the dialog code. This way, a non-views
   implementation of the UI only has to know about PaymentRequestImpl
   and the code that interacts with the client doesn't have to be
   duplicated.
b) The dialog is now opened through
   constrained_window::ShowWebModalDialogViews(), constraining it to the
   triggering WebContents and closing it automatically when the tab is
   closed.

BUG=667872
Committed: https://crrev.com/3d7f9727782b932937b4e031bc0d88ccceefe1f3
Cr-Commit-Position: refs/heads/master@{#437018}

[anthonyvd, 2016-11-23 20:05:43]

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

[commit-bot: I haz the power, 2016-11-23 20:06:10]

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

[anthonyvd, 2016-11-23 20:13:45]

anthonyvd@chromium.org changed reviewers:
+ krb@chromium.org, rouslan@chromium.org

[anthonyvd, 2016-11-23 20:13:45]

Hello Rouslan and Kevin,

As requested in https://codereview.chromium.org/2528503002, this CL moves all
views-specific code to c/b/ui/views/. I initially wrote this as a patchset of
the other CL but it became pretty big so here it is as a separate CL.

Can you please take a look? I'm particularily interested in your opinion re: the
payments::mojom::PaymentRequestClientPtr instance being owned by
PaymentRequestImpl instead of PaymentRequestDialog and the dialog only knowing
about PaymentRequestImpl (not the PaymentRequestClient directly). This makes
sense to me but I just want to make sure I'm not missing anything.

Thanks!

[commit-bot: I haz the power, 2016-11-23 20:24:04]

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

[commit-bot: I haz the power, 2016-11-23 20:24:05]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[anthonyvd, 2016-11-23 20:30:51]

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

[commit-bot: I haz the power, 2016-11-23 20:31: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, 2016-11-23 20:52:11]

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

[commit-bot: I haz the power, 2016-11-23 20:52:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[anthonyvd, 2016-11-23 21:29:14]

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

[commit-bot: I haz the power, 2016-11-23 21:29: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, 2016-11-24 00:21:18]

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

[commit-bot: I haz the power, 2016-11-24 00:21:19]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2016-11-28 15:16:41]

LGTM after comment

We should avoid raw pointers and be super clear about ownership of objects. This
helps to reduce memory leaks and other bugs.

https://codereview.chromium.org/2529733002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_dialog.h (right):

https://codereview.chromium.org/2529733002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog.h:30:
PaymentRequestImpl* impl_;  // Not owned.
Clearly describe the lifetime of PaymentRequestImpl and PaymentRequestDialog in
comments. Raw pointers with comments "not owned" are scary.

[Kevin Bailey, 2016-11-28 15:53:52]

https://codereview.chromium.org/2529733002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/2529733002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window.h:402: virtual void
ShowWebPaymentsDialog(payments::PaymentRequestImpl* impl) = 0;
This addition looks very invasive to me. The other methods in this class look
like generic windowing methods, but this one serves a single app. Shouldn't this
functionality be behind something like 'ShowAppMenu()' ?  If not, shouldn't the
method be named more generically, so that other apps can use and implement it?

Did the previous implementation, which didn't need to touch this class, not do
what we want? If it allowed interaction with the page, maybe there's a way to
prevent that?

If the goal of this approach was to remove the Views-specific references from
generic code, I'm not sure that shoe-horning it into a generic class is really
better.

IOW, I'm not seeing that other apps are having to do this, so I'm wondering if
there's a better canonical solution.

[anthonyvd, 2016-11-28 16:12:18]

https://codereview.chromium.org/2529733002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_dialog.h (right):

https://codereview.chromium.org/2529733002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog.h:30:
PaymentRequestImpl* impl_;  // Not owned.
On 2016/11/28 at 15:16:41, rouslan wrote:
> Clearly describe the lifetime of PaymentRequestImpl and PaymentRequestDialog
in comments. Raw pointers with comments "not owned" are scary.

Done.

Totally agree with you. Added a comment, let me know what you think.

[anthonyvd, 2016-11-28 16:34:01]

https://codereview.chromium.org/2529733002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/2529733002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window.h:402: virtual void
ShowWebPaymentsDialog(payments::PaymentRequestImpl* impl) = 0;
On 2016/11/28 at 15:53:52, Kevin Bailey wrote:
> This addition looks very invasive to me. The other methods in this class look
like generic windowing methods, but this one serves a single app. Shouldn't this
functionality be behind something like 'ShowAppMenu()' ?  If not, shouldn't the
method be named more generically, so that other apps can use and implement it?
> 
> Did the previous implementation, which didn't need to touch this class, not do
what we want? If it allowed interaction with the page, maybe there's a way to
prevent that?
> 
> If the goal of this approach was to remove the Views-specific references from
generic code, I'm not sure that shoe-horning it into a generic class is really
better.
> 
> IOW, I'm not seeing that other apps are having to do this, so I'm wondering if
there's a better canonical solution.

Yeah so it's not really that the previous solution didn't do what we wanted. It
did, but it violated the convention that Views-specific code must live in
c/b/ui/views/. Once the dialog code is moved to c/b/ui/views/ however, it
becomes impossible for PaymentRequestImpl to instantiate it directly because
including c/b/ui/views file from c/b/ files is forbidden (and that's enforced
through presubmit rules).

I'm not sure what you mean by "app" in the context of "...this one serves a
single app. Shouldn't this functionality be behind something like
'ShowAppMenu()' ?  If not, shouldn't the method be named more generically, so
that other apps can use and implement it?". Do you mean a different javascript
API? ShowAppMenu() appears to be an accessibility thing so I'm not sure how it
relates.

Ultimately, this solution is similar to the way the Avatar Bubble and the Sign
In flow are implemented, so there's precedent for it:

  - The ShowAvatarBubbleFromAvatarButton is also a platform-specific
implementation that can be called from generic code, so it's in BrowserWindow as
well. 
  - ShowModalSigninWindow is basically the same thing as ShowWebPaymentsDialog
(it's platform-specific code that is called from generic code to show a
tab-modal dialog) and it hangs from Browser (with a views/cocoa specific object
calling to platform-specific implementations).

I'd also be interested in finding a solution that doesn't appear so intrusive so
I'm very open to suggestions, but I think it might be unavoidable to go through
Browser or BrowserWindow if the code files stay where they are.

[Kevin Bailey, 2016-11-28 17:27:06]

https://codereview.chromium.org/2529733002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/2529733002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window.h:402: virtual void
ShowWebPaymentsDialog(payments::PaymentRequestImpl* impl) = 0;
Saying 'app' near 'ShowAppMenu()' was simply a coincidence. All I meant was
that, I see dozens of Mojo registrations in chrome_content_browser_client.cc,
but I don't see nearly as many invasive methods, so I assume that they either
don't do anything graphical, or they found a way to do it non-invasively.

One solution would be to move PaymentRequestImpl to .../views as well. The Mojo
connection is the latest binding one in this chain, so if we want to put
distance between the browser code and our implementation, lining up those two
"seams" would make sense.

Another possibility is to make the new method more generic, and have it take a
callback or other well-known object, and pass it something platform-specific.

[anthonyvd, 2016-11-29 09:08:01]

https://codereview.chromium.org/2529733002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/2529733002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window.h:402: virtual void
ShowWebPaymentsDialog(payments::PaymentRequestImpl* impl) = 0;
On 2016/11/28 at 17:27:06, Kevin Bailey wrote:
> Saying 'app' near 'ShowAppMenu()' was simply a coincidence. All I meant was
that, I see dozens of Mojo registrations in chrome_content_browser_client.cc,
but I don't see nearly as many invasive methods, so I assume that they either
don't do anything graphical, or they found a way to do it non-invasively.

Fair point.

> 
> One solution would be to move PaymentRequestImpl to .../views as well. The
Mojo connection is the latest binding one in this chain, so if we want to put
distance between the browser code and our implementation, lining up those two
"seams" would make sense.

Conceptually, PaymentRequestImpl is independent from the UI stack so I'd like to
avoid moving it to views if possible (for example, we'd have to duplicate it if
we end up needing a Cocoa implementation because MacViews slips).

> 
> Another possibility is to make the new method more generic, and have it take a
callback or other well-known object, and pass it something platform-specific.

I'll look into improving this when I have time (in Paris atm so it might take a
few days).

Thanks for your feedback!

[anthonyvd, 2016-12-05 17:26:23]

anthonyvd@chromium.org changed reviewers:
+ sky@chromium.org

[anthonyvd, 2016-12-05 17:26:24]

So looking into this a bit more, it seems to me like having a function in
BrowserWindow is indeed the canonical way of showing UI-stack-specific UI from
generic chrome/browser/ code. Bookmarks, Autofill, Translate and Sign In all do
it this way (see functions around
https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_window.h?rcl=0...).

+sky@ for advice and an OWNER review. 

Scott, can you please take a look at this CL that means to address your comments
about views-specific code location in https://codereview.chromium.org/2528503002
? Thanks!

[sky, 2016-12-05 21:49:46]

https://codereview.chromium.org/2529733002/diff/60001/chrome/browser/payments...
File chrome/browser/payments/payment_request_impl.cc (right):

https://codereview.chromium.org/2529733002/diff/60001/chrome/browser/payments...
chrome/browser/payments/payment_request_impl.cc:59: void
PaymentRequestImpl::Init(
Who is creating this class? Do they not have a browser at the time they create
it? Having to find the browser given a webcontents will not always work (say the
webcontents isn't in a browser).

https://codereview.chromium.org/2529733002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2529733002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2494: void
BrowserView::ShowWebPaymentsDialog(payments::PaymentRequestImpl* impl) {
Why do you need to route this through BrowserWindow when it doesn't seem you use
any context from BrowserWindow?

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

https://codereview.chromium.org/2529733002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog.h:15: class
PaymentRequestDialog : public views::DialogDelegateView {
Add description.

[anthonyvd, 2016-12-05 22:11:27]

https://codereview.chromium.org/2529733002/diff/60001/chrome/browser/payments...
File chrome/browser/payments/payment_request_impl.cc (right):

https://codereview.chromium.org/2529733002/diff/60001/chrome/browser/payments...
chrome/browser/payments/payment_request_impl.cc:59: void
PaymentRequestImpl::Init(
On 2016/12/05 at 21:49:46, sky wrote:
> Who is creating this class? Do they not have a browser at the time they create
it? Having to find the browser given a webcontents will not always work (say the
webcontents isn't in a browser).

This is created from a mojo message
(https://cs.chromium.org/chromium/src/out/Debug/gen/components/payments/paymen...)
which I don't think knows about browser. 

The UI depends on the webUI having a browser though, so I imagine the best
course of action would be to no-op and return an error to the WebPayments API.
rouslan@, thoughts?

https://codereview.chromium.org/2529733002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2529733002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2494: void
BrowserView::ShowWebPaymentsDialog(payments::PaymentRequestImpl* impl) {
On 2016/12/05 at 21:49:46, sky wrote:
> Why do you need to route this through BrowserWindow when it doesn't seem you
use any context from BrowserWindow?

I routed it through here because payments::PaymentRequestImpl can't call into
c/b/ui/views/ code (being in c/b/ itself). Basically it's generic browser code
that wants to show some UI that happens to be implemented in views. Kevin raised
this point as well and I wasn't able to find a better way to achieve this. 

This pattern is also used for autofill's credit card save dialog, the avatar
menu, the translate bubble, and others. Is there a better way?

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

https://codereview.chromium.org/2529733002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog.h:15: class
PaymentRequestDialog : public views::DialogDelegateView {
On 2016/12/05 at 21:49:46, sky wrote:
> Add description.

Done.

[sky, 2016-12-05 23:23:33]

https://codereview.chromium.org/2529733002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2529733002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2494: void
BrowserView::ShowWebPaymentsDialog(payments::PaymentRequestImpl* impl) {
On 2016/12/05 22:11:27, anthonyvd wrote:
> On 2016/12/05 at 21:49:46, sky wrote:
> > Why do you need to route this through BrowserWindow when it doesn't seem you
> use any context from BrowserWindow?
> 
> I routed it through here because payments::PaymentRequestImpl can't call into
> c/b/ui/views/ code (being in c/b/ itself). Basically it's generic browser code
> that wants to show some UI that happens to be implemented in views. Kevin
raised
> this point as well and I wasn't able to find a better way to achieve this. 
> 
> This pattern is also used for autofill's credit card save dialog, the avatar
> menu, the translate bubble, and others. Is there a better way?

Follow the pattern in c/b/ui/browser_dialogs, which is you declare the function
in browser_dialogs.h and have the definition in a platform specific file.

[anthonyvd, 2016-12-06 15:48:26]

Hi everyone, patchset 6 addresses krb@ and sky@'s comments about having
ShowPaymentRequestDialog on BrowserWindow by moving that function to
browser_dialogs. Can you PTAL?

Thanks!

https://codereview.chromium.org/2529733002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2529733002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2494: void
BrowserView::ShowWebPaymentsDialog(payments::PaymentRequestImpl* impl) {
On 2016/12/05 at 23:23:33, sky wrote:
> On 2016/12/05 22:11:27, anthonyvd wrote:
> > On 2016/12/05 at 21:49:46, sky wrote:
> > > Why do you need to route this through BrowserWindow when it doesn't seem
you
> > use any context from BrowserWindow?
> > 
> > I routed it through here because payments::PaymentRequestImpl can't call
into
> > c/b/ui/views/ code (being in c/b/ itself). Basically it's generic browser
code
> > that wants to show some UI that happens to be implemented in views. Kevin
raised
> > this point as well and I wasn't able to find a better way to achieve this. 
> > 
> > This pattern is also used for autofill's credit card save dialog, the avatar
> > menu, the translate bubble, and others. Is there a better way?
> 
> Follow the pattern in c/b/ui/browser_dialogs, which is you declare the
function in browser_dialogs.h and have the definition in a platform specific
file.

Ah! This is what we were looking for, thanks a ton!

Done.

[sky, 2016-12-06 17:38:44]

LGTM

[Kevin Bailey, 2016-12-07 17:06:16]

lgtm

[anthonyvd, 2016-12-07 17:52:06]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2016-12-07 17:52:07]

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/2529733002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-12-07 17:52:38]

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, 2016-12-07 18:43:58]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481133126733940,
"parent_rev": "e4441cf1701681fbdc35fdc2d11e640299f720ac", "commit_rev":
"9a3fddfbb7bd604a00ed8a428bea06019e67c55f"}

[commit-bot: I haz the power, 2016-12-07 18:44:25]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-07 18:46:59]

Description was changed from

==========
[WebPayments] Move views-specific code to c/b/ui/views/

This CL moves the PaymentRequestDialog class to c/b/ui/views/.
Moving it is required because:

a) It's views-specific, so convention dictates it should live in
   c/b/ui/views/
b) It will consume views specific code, which is disallowed from c/b/*

This also means that PaymentRequestImpl can't directly instantiate
PaymentRequestDialog anymore. This is solved by adding the
ShowWebPaymentsDialog function to BrowserWindow.

This CL also fixes some lifetime and ownership issues that became
apparent during the move. Specifically:

a) payments::mojom::PaymentRequestClientPtr should be owned by
   PaymentRequestImpl rather than the dialog code. This way, a non-views
   implementation of the UI only has to know about PaymentRequestImpl
   and the code that interacts with the client doesn't have to be
   duplicated.
b) The dialog is now opened through
   constrained_window::ShowWebModalDialogViews(), constraining it to the
   triggering WebContents and closing it automatically when the tab is
   closed.

BUG=667872
==========

to

==========
[WebPayments] Move views-specific code to c/b/ui/views/

This CL moves the PaymentRequestDialog class to c/b/ui/views/.
Moving it is required because:

a) It's views-specific, so convention dictates it should live in
   c/b/ui/views/
b) It will consume views specific code, which is disallowed from c/b/*

This also means that PaymentRequestImpl can't directly instantiate
PaymentRequestDialog anymore. This is solved by adding the
ShowWebPaymentsDialog function to BrowserWindow.

This CL also fixes some lifetime and ownership issues that became
apparent during the move. Specifically:

a) payments::mojom::PaymentRequestClientPtr should be owned by
   PaymentRequestImpl rather than the dialog code. This way, a non-views
   implementation of the UI only has to know about PaymentRequestImpl
   and the code that interacts with the client doesn't have to be
   duplicated.
b) The dialog is now opened through
   constrained_window::ShowWebModalDialogViews(), constraining it to the
   triggering WebContents and closing it automatically when the tab is
   closed.

BUG=667872

Committed: https://crrev.com/3d7f9727782b932937b4e031bc0d88ccceefe1f3
Cr-Commit-Position: refs/heads/master@{#437018}
==========

[commit-bot: I haz the power, 2016-12-07 18:47:00]

Patchset 7 (id:??) landed as
https://crrev.com/3d7f9727782b932937b4e031bc0d88ccceefe1f3
Cr-Commit-Position: refs/heads/master@{#437018}