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

chrome/browser/ui/browser_window.h

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_UI_BROWSER_WINDOW_H_
 #define CHROME_BROWSER_UI_BROWSER_WINDOW_H_
 
 #include "base/callback_forward.h"
 #include "build/build_config.h"
 #include "chrome/browser/lifetime/browser_close_manager.h"
@@ skipping 39 matching lines @@
 namespace extensions {
 class Command;
 class Extension;
 }
 
 namespace gfx {
 class Rect;
 class Size;
 }
 
+namespace payments {
+class PaymentRequestImpl;
+}
+
 namespace security_state {
 struct SecurityInfo;
 }  // namespace security_state
 
 namespace signin_metrics {
 enum class AccessPoint;
 }
 
 namespace web_modal {
 class WebContentsModalDialogHost;
@@ skipping 318 matching lines @@
   virtual void ShowImeWarningBubble(
       const extensions::Extension* extension,
       const base::Callback<void(ImeWarningBubblePermissionStatus status)>&
           callback) = 0;
 
   // Returns the platform-specific ID of the workspace the browser window
   // currently resides in.
   virtual std::string GetWorkspace() const = 0;
   virtual bool IsVisibleOnAllWorkspaces() const = 0;
 
+  virtual void ShowWebPaymentsDialog(payments::PaymentRequestImpl* impl) = 0;

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

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:34:01]

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]

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]

Fair point.
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).
I'll look into improving this when I have time (in Paris atm so it might take a
few days).

Thanks for your feedback!

+
  protected:
   friend class BrowserCloseManager;
   friend class BrowserView;
   virtual void DestroyBrowser() = 0;
 };
 
 #endif  // CHROME_BROWSER_UI_BROWSER_WINDOW_H_