Make SigninViewControllerDelegate an interface.

There is a single implementation of the SigninViewControllerDelegate (the
SigninViewControllerDelegateViews). This CL moves all the implementation
login of SigninViewControllerDelegate to SigninViewControllerDelegateViews
and makes the SigninViewControllerDelegate an interface. The goal is to
simplify the implementation and avoid the current double implementation
layer that adds complexity without any benefit.

Change-Id: I2658e717d76b64921e6fff12229369aa8508dfc1
Reviewed-on: https://chromium-review.googlesource.com/c/1454524
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Thomas Tangl <tangltom@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629522}

chrome/browser/ui/signin_view_controller.cc

@@ -274,5 +274,5 @@ void SigninViewController::ShowDiceAddAccountTab(
 content::WebContents*
 SigninViewController::GetModalDialogWebContentsForTesting() {
   DCHECK(delegate_);
-  return delegate_->web_contents();
+  return delegate_->GetWebContents();
 }

chrome/browser/ui/signin_view_controller_delegate.cc

@@ -15,95 +15,6 @@
 #include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/web_contents.h"
 
-namespace {
-
-content::WebContents* GetAuthFrameWebContents(
-    content::WebContents* web_ui_web_contents) {
-  return signin::GetAuthFrameWebContents(web_ui_web_contents, "signin-frame");
-}
-
-}  // namespace
-
-SigninViewControllerDelegate::SigninViewControllerDelegate(
-    SigninViewController* signin_view_controller,
-    content::WebContents* web_contents,
-    Browser* browser)
-    : signin_view_controller_(signin_view_controller),
-      web_contents_(web_contents),
-      browser_(browser) {
-  DCHECK(web_contents_);
-  DCHECK(browser_);
-  DCHECK(browser_->tab_strip_model()->GetActiveWebContents())
-      << "A tab must be active to present the sign-in modal dialog.";
-  web_contents_->SetDelegate(this);
-}
+SigninViewControllerDelegate::SigninViewControllerDelegate() {}
 
 SigninViewControllerDelegate::~SigninViewControllerDelegate() {}
-
-void SigninViewControllerDelegate::AttachDialogManager() {
-  web_modal::WebContentsModalDialogManager::CreateForWebContents(web_contents_);
-  web_modal::WebContentsModalDialogManager* manager =
-      web_modal::WebContentsModalDialogManager::FromWebContents(web_contents_);
-  manager->SetDelegate(this);
-}
-
-void SigninViewControllerDelegate::CloseModalSignin() {
-  ResetSigninViewControllerDelegate();
-  PerformClose();
-}
-
-void SigninViewControllerDelegate::PerformNavigation() {
-  if (CanGoBack(web_contents_))
-    GetAuthFrameWebContents(web_contents_)->GetController().GoBack();
-  else
-    CloseModalSignin();
-}
-
-bool SigninViewControllerDelegate::HandleContextMenu(
-    const content::ContextMenuParams& params) {
-  // Discard the context menu
-  return true;
-}
-
-web_modal::WebContentsModalDialogHost*
-SigninViewControllerDelegate::GetWebContentsModalDialogHost() {
-  return browser()->window()->GetWebContentsModalDialogHost();
-}
-
-void SigninViewControllerDelegate::ResetSigninViewControllerDelegate() {
-  if (signin_view_controller_) {
-    signin_view_controller_->ResetModalSigninDelegate();
-    signin_view_controller_ = nullptr;
-  }
-}
-
-// content::WebContentsDelegate
-void SigninViewControllerDelegate::LoadingStateChanged(
-    content::WebContents* source,
-    bool to_different_document) {
-  // The WebUI object can be missing for an error page, per
-  // https://crbug.com/860409.
-  if (!source->GetWebUI())
-    return;
-
-  if (CanGoBack(source)) {
-    source->GetWebUI()->CallJavascriptFunctionUnsafe(
-        "inline.login.showBackButton");
-  } else {
-    source->GetWebUI()->CallJavascriptFunctionUnsafe(
-        "inline.login.showCloseButton");
-  }
-}
-
-bool SigninViewControllerDelegate::HandleKeyboardEvent(
-    content::WebContents* source,
-    const content::NativeWebKeyboardEvent& event) {
-  NOTREACHED();
-  return false;
-}
-
-bool SigninViewControllerDelegate::CanGoBack(
-    content::WebContents* web_ui_web_contents) const {
-  auto* auth_web_contents = GetAuthFrameWebContents(web_ui_web_contents);
-  return auth_web_contents && auth_web_contents->GetController().CanGoBack();
-}

chrome/browser/ui/signin_view_controller_delegate.h

@@ -5,10 +5,7 @@
 #ifndef CHROME_BROWSER_UI_SIGNIN_VIEW_CONTROLLER_DELEGATE_H_
 #define CHROME_BROWSER_UI_SIGNIN_VIEW_CONTROLLER_DELEGATE_H_
 
-#include "build/build_config.h"
-#include "chrome/browser/ui/chrome_web_modal_dialog_manager_delegate.h"
-#include "chrome/browser/ui/profile_chooser_constants.h"
-#include "content/public/browser/web_contents_delegate.h"
+#include "base/macros.h"
 
 class Browser;
 class SigninViewController;
@@ -17,25 +14,18 @@ namespace signin_metrics {
 enum class AccessPoint;
 }
 
-// Abstract base class to the platform-specific managers of the Signin and Sync
+namespace content {
+class WebContents;
+}
+
+// Interface to the platform-specific managers of the Signin and Sync
 // confirmation tab-modal dialogs. This and its platform-specific
 // implementations are responsible for actually creating and owning the dialogs,
 // as well as managing the navigation inside them.
 // Subclasses are responsible for deleting themselves when the window they're
 // managing closes.
-class SigninViewControllerDelegate
-    : public content::WebContentsDelegate,
-      public ChromeWebModalDialogManagerDelegate {
+class SigninViewControllerDelegate {
  public:
-  // Returns a platform-specific SigninViewControllerDelegate instance that
-  // displays the sign in flow. The returned object should delete itself when
-  // the window it's managing is closed.
-  static SigninViewControllerDelegate* CreateModalSigninDelegate(
-      SigninViewController* signin_view_controller,
-      profiles::BubbleViewMode mode,
-      Browser* browser,
-      signin_metrics::AccessPoint access_point);
-
   // Returns a platform-specific SigninViewControllerDelegate instance that
   // displays the sync confirmation dialog. The returned object should delete
   // itself when the window it's managing is closed.
@@ -50,71 +40,29 @@ class SigninViewControllerDelegate
       SigninViewController* signin_view_controller,
       Browser* browser);
 
-  // Attaches a dialog manager to this sign-in view controller dialog.
-  // Should be called by subclasses when a different dialog may need to be
-  // presented on top of the sign-in dialog.
-  void AttachDialogManager();
-
   // Closes the sign-in dialog. Note that this method may destroy this object,
   // so the caller should no longer use this object after calling this method.
-  void CloseModalSignin();
+  virtual void CloseModalSignin() = 0;
 
   // Either navigates back in the signin flow if the history state allows it or
   // closes the flow otherwise. Note that if view is closed, this method may
   // destroy this object, so the caller should no longer use this object after
   // calling this method.
-  void PerformNavigation();
+  virtual void PerformNavigation() = 0;
 
   // This will be called by the base class to request a resize of the native
   // view hosting the content to |height|. |height| is the total height of the
   // content, in pixels.
   virtual void ResizeNativeView(int height) = 0;
 
-  // content::WebContentsDelegate:
-  bool HandleContextMenu(const content::ContextMenuParams& params) override;
-
-  // ChromeWebModalDialogManagerDelegate:
-  web_modal::WebContentsModalDialogHost* GetWebContentsModalDialogHost()
-      override;
-
-  // WebContents is used for executing javascript in the context of a modal sync
-  // confirmation dialog.
-  content::WebContents* web_contents() { return web_contents_; }
+  // Returns the web contents of the modal dialog.
+  virtual content::WebContents* GetWebContents() = 0;
 
  protected:
-  SigninViewControllerDelegate(SigninViewController* signin_view_controller,
-                               content::WebContents* web_contents,
-                               Browser* browser);
-  ~SigninViewControllerDelegate() override;
-
-  Browser* browser() { return browser_; }
-
-  // Notifies the SigninViewController that this instance is being deleted.
-  void ResetSigninViewControllerDelegate();
-
-  // content::WebContentsDelegate
-  void LoadingStateChanged(content::WebContents* source,
-                           bool to_different_document) override;
-
-  // Subclasses must override this method to correctly handle accelerators.
-  bool HandleKeyboardEvent(
-      content::WebContents* source,
-      const content::NativeWebKeyboardEvent& event) override;
-
-  // This will be called by this base class when the tab-modal window must be
-  // closed. This should close the platform-specific window that is currently
-  // showing the sign in flow or the sync confirmation dialog. Note that this
-  // method may destroy this object, so the caller should no longer use this
-  // object after calling this method.
-  virtual void PerformClose() = 0;
+  SigninViewControllerDelegate();
+  virtual ~SigninViewControllerDelegate();
 
  private:
-  bool CanGoBack(content::WebContents* web_ui_web_contents) const;
-
-  SigninViewController* signin_view_controller_;  // Not owned.
-  content::WebContents* const web_contents_;      // Not owned.
-  Browser* const browser_;                        // Not owned.
-
   DISALLOW_COPY_AND_ASSIGN(SigninViewControllerDelegate);
 };
 

chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc

@@ -23,6 +23,17 @@
 #include "ui/views/controls/webview/webview.h"
 #include "ui/views/widget/widget.h"
 
+#include "chrome/browser/ui/webui/signin/signin_utils.h"
+
+namespace {
+
+content::WebContents* GetAuthFrameWebContents(
+    content::WebContents* web_ui_web_contents) {
+  return signin::GetAuthFrameWebContents(web_ui_web_contents, "signin-frame");
+}
+
+}  // namespace
+
 namespace {
 
 const int kModalDialogWidth = 448;
@@ -54,12 +65,18 @@ SigninViewControllerDelegateViews::SigninViewControllerDelegateViews(
     Browser* browser,
     ui::ModalType dialog_modal_type,
     bool wait_for_size)
-    : SigninViewControllerDelegate(signin_view_controller,
-                                   content_view->GetWebContents(),
-                                   browser),
+    : signin_view_controller_(signin_view_controller),
+      web_contents_(content_view->GetWebContents()),
+      browser_(browser),
       content_view_(content_view.release()),
       modal_signin_widget_(nullptr),
       dialog_modal_type_(dialog_modal_type) {
+  DCHECK(web_contents_);
+  DCHECK(browser_);
+  DCHECK(browser_->tab_strip_model()->GetActiveWebContents())
+      << "A tab must be active to present the sign-in modal dialog.";
+  web_contents_->SetDelegate(this);
+
   DCHECK(dialog_modal_type == ui::MODAL_TYPE_CHILD ||
          dialog_modal_type == ui::MODAL_TYPE_WINDOW)
       << "Unsupported dialog modal type " << dialog_modal_type;
@@ -120,16 +137,20 @@ void SigninViewControllerDelegateViews::ResizeNativeView(int height) {
   }
 }
 
-bool SigninViewControllerDelegateViews::HandleKeyboardEvent(
-    content::WebContents* source,
-    const content::NativeWebKeyboardEvent& event) {
-  // If this is a MODAL_TYPE_CHILD, then GetFocusManager() will return the focus
-  // manager of the parent window, which has registered accelerators, and the
-  // accelerators will fire. If this is a MODAL_TYPE_WINDOW, then this will have
-  // no effect, since no accelerators have been registered for this standalone
-  // window.
-  return unhandled_keyboard_event_handler_.HandleKeyboardEvent(
-      event, GetFocusManager());
+content::WebContents* SigninViewControllerDelegateViews::GetWebContents() {
+  return web_contents_;
+}
+
+void SigninViewControllerDelegateViews::PerformNavigation() {
+  if (CanGoBack(web_contents_))
+    GetAuthFrameWebContents(web_contents_)->GetController().GoBack();
+  else
+    CloseModalSignin();
+}
+
+void SigninViewControllerDelegateViews::CloseModalSignin() {
+  ResetSigninViewControllerDelegate();
+  PerformClose();
 }
 
 void SigninViewControllerDelegateViews::DisplayModal() {
@@ -161,40 +182,40 @@ void SigninViewControllerDelegateViews::DisplayModal() {
   content_view_->RequestFocus();
 }
 
-#if defined(OS_CHROMEOS)
-// static
-std::unique_ptr<views::WebView>
-SigninViewControllerDelegateViews::CreateGaiaWebView(
-    content::WebContentsDelegate* delegate,
-    profiles::BubbleViewMode mode,
-    Browser* browser,
-    signin_metrics::AccessPoint access_point) {
-  GURL url = signin::GetEmbeddedSigninURLFromBubbleViewMode(browser->profile(),
-                                                            mode, access_point);
-
-  constexpr int kFixedGaiaViewHeight = 612;
-  int max_height = browser
-      ->window()
-      ->GetWebContentsModalDialogHost()
-      ->GetMaximumDialogSize().height();
-  // Adds Gaia signin webview.
-  const gfx::Size pref_size(kModalDialogWidth,
-                            std::min(kFixedGaiaViewHeight, max_height));
-  views::WebView* web_view = new views::WebView(browser->profile());
-  web_view->LoadInitialURL(url);
+bool SigninViewControllerDelegateViews::HandleKeyboardEvent(
+    content::WebContents* source,
+    const content::NativeWebKeyboardEvent& event) {
+  // If this is a MODAL_TYPE_CHILD, then GetFocusManager() will return the focus
+  // manager of the parent window, which has registered accelerators, and the
+  // accelerators will fire. If this is a MODAL_TYPE_WINDOW, then this will have
+  // no effect, since no accelerators have been registered for this standalone
+  // window.
+  return unhandled_keyboard_event_handler_.HandleKeyboardEvent(
+      event, GetFocusManager());
+}
 
-  if (delegate)
-    web_view->GetWebContents()->SetDelegate(delegate);
+bool SigninViewControllerDelegateViews::HandleContextMenu(
+    const content::ContextMenuParams& params) {
+  // Discard the context menu
+  return true;
+}
 
-  web_view->SetPreferredSize(pref_size);
-  content::RenderWidgetHostView* rwhv =
-      web_view->GetWebContents()->GetRenderWidgetHostView();
-  if (rwhv)
-    rwhv->SetBackgroundColor(profiles::kAvatarBubbleGaiaBackgroundColor);
+void SigninViewControllerDelegateViews::LoadingStateChanged(
+    content::WebContents* source,
+    bool to_different_document) {
+  // The WebUI object can be missing for an error page, per
+  // https://crbug.com/860409.
+  if (!source->GetWebUI())
+    return;
 
-  return std::unique_ptr<views::WebView>(web_view);
+  if (CanGoBack(source)) {
+    source->GetWebUI()->CallJavascriptFunctionUnsafe(
+        "inline.login.showBackButton");
+  } else {
+    source->GetWebUI()->CallJavascriptFunctionUnsafe(
+        "inline.login.showCloseButton");
+  }
 }
-#endif
 
 std::unique_ptr<views::WebView>
 SigninViewControllerDelegateViews::CreateSyncConfirmationWebView(
@@ -238,20 +259,27 @@ SigninViewControllerDelegateViews::CreateDialogWebView(
   return std::unique_ptr<views::WebView>(web_view);
 }
 
-#if defined(OS_CHROMEOS)
-SigninViewControllerDelegate*
-SigninViewControllerDelegate::CreateModalSigninDelegate(
-    SigninViewController* signin_view_controller,
-    profiles::BubbleViewMode mode,
-    Browser* browser,
-    signin_metrics::AccessPoint access_point) {
-  return new SigninViewControllerDelegateViews(
-      signin_view_controller,
-      SigninViewControllerDelegateViews::CreateGaiaWebView(
-          nullptr, mode, browser, access_point),
-      browser, ui::MODAL_TYPE_CHILD, false);
+web_modal::WebContentsModalDialogHost*
+SigninViewControllerDelegateViews::GetWebContentsModalDialogHost() {
+  return browser()->window()->GetWebContentsModalDialogHost();
 }
-#endif
+
+void SigninViewControllerDelegateViews::ResetSigninViewControllerDelegate() {
+  if (signin_view_controller_) {
+    signin_view_controller_->ResetModalSigninDelegate();
+    signin_view_controller_ = nullptr;
+  }
+}
+
+bool SigninViewControllerDelegateViews::CanGoBack(
+    content::WebContents* web_ui_web_contents) const {
+  auto* auth_web_contents = GetAuthFrameWebContents(web_ui_web_contents);
+  return auth_web_contents && auth_web_contents->GetController().CanGoBack();
+}
+
+// --------------------------------------------------------------------
+// SigninViewControllerDelegate static methods
+// --------------------------------------------------------------------
 
 SigninViewControllerDelegate*
 SigninViewControllerDelegate::CreateSyncConfirmationDelegate(

chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.h

@@ -6,21 +6,17 @@
 #define CHROME_BROWSER_UI_VIEWS_PROFILES_SIGNIN_VIEW_CONTROLLER_DELEGATE_VIEWS_H_
 
 #include "base/macros.h"
+#include "chrome/browser/ui/chrome_web_modal_dialog_manager_delegate.h"
 #include "chrome/browser/ui/profile_chooser_constants.h"
 #include "chrome/browser/ui/signin_view_controller_delegate.h"
+#include "content/public/browser/web_contents_delegate.h"
 #include "ui/views/controls/webview/unhandled_keyboard_event_handler.h"
 #include "ui/views/window/dialog_delegate.h"
 
-class Browser;
-
 namespace content {
 class WebContentsDelegate;
 }
 
-namespace signin_metrics {
-enum class AccessPoint;
-}
-
 namespace views {
 class WebView;
 }
@@ -29,8 +25,11 @@ class WebView;
 // managing the Signin and Sync Confirmation tab-modal dialogs.
 // Instances of this class delete themselves when the window they're managing
 // closes (in the DeleteDelegate callback).
-class SigninViewControllerDelegateViews : public views::DialogDelegateView,
-                                          public SigninViewControllerDelegate {
+class SigninViewControllerDelegateViews
+    : public views::DialogDelegateView,
+      public SigninViewControllerDelegate,
+      public content::WebContentsDelegate,
+      public ChromeWebModalDialogManagerDelegate {
  public:
 
   static std::unique_ptr<views::WebView> CreateSyncConfirmationWebView(
@@ -48,20 +47,27 @@ class SigninViewControllerDelegateViews : public views::DialogDelegateView,
   bool ShouldShowCloseButton() const override;
   int GetDialogButtons() const override;
 
+  // SigninViewControllerDelegate:
+  void CloseModalSignin() override;
+  void PerformNavigation() override;
+  void ResizeNativeView(int height) override;
+  content::WebContents* GetWebContents() override;
+
+  // content::WebContentsDelegate:
+  void LoadingStateChanged(content::WebContents* source,
+                           bool to_different_document) override;
+  bool HandleContextMenu(const content::ContextMenuParams& params) override;
+  bool HandleKeyboardEvent(
+      content::WebContents* source,
+      const content::NativeWebKeyboardEvent& event) override;
+
+  // ChromeWebModalDialogManagerDelegate:
+  web_modal::WebContentsModalDialogHost* GetWebContentsModalDialogHost()
+      override;
+
  private:
   friend SigninViewControllerDelegate;
 
-#if defined(OS_CHROMEOS)
-  // Creates the web view that contains the signin flow in |mode| using
-  // |profile| as the web content's profile, then sets |delegate| as the created
-  // web content's delegate.
-  static std::unique_ptr<views::WebView> CreateGaiaWebView(
-      content::WebContentsDelegate* delegate,
-      profiles::BubbleViewMode mode,
-      Browser* browser,
-      signin_metrics::AccessPoint access_point);
-#endif
-
   // Creates and displays a constrained window containing |web_contents|. If
   // |wait_for_size| is true, the delegate will wait for ResizeNativeView() to
   // be called by the base class before displaying the constrained window.
@@ -73,16 +79,6 @@ class SigninViewControllerDelegateViews : public views::DialogDelegateView,
       bool wait_for_size);
   ~SigninViewControllerDelegateViews() override;
 
-  void PerformClose() override;
-  void ResizeNativeView(int height) override;
-
-  // content::WebContentsDelegate:
-  bool HandleKeyboardEvent(
-      content::WebContents* source,
-      const content::NativeWebKeyboardEvent& event) override;
-
-  void DisplayModal();
-
   // Creates a WebView for a dialog with the specified URL.
   static std::unique_ptr<views::WebView> CreateDialogWebView(
       Browser* browser,
@@ -90,6 +86,25 @@ class SigninViewControllerDelegateViews : public views::DialogDelegateView,
       int dialog_height,
       base::Optional<int> dialog_width);
 
+  // Notifies the SigninViewController that this instance is being deleted.
+  void ResetSigninViewControllerDelegate();
+
+  // Displays the modal dialog.
+  void DisplayModal();
+
+  // Returns true if |web_ui_web_contents| can go back.
+  bool CanGoBack(content::WebContents* web_ui_web_contents) const;
+
+  // Close the platform-specific dialog window. Note that this
+  // method may destroy this object, so the caller should no longer use this
+  // object after calling this method.
+  void PerformClose();
+
+  Browser* browser() { return browser_; }
+
+  SigninViewController* signin_view_controller_;  // Not owned.
+  content::WebContents* const web_contents_;      // Not owned.
+  Browser* const browser_;                        // Not owned.
   views::WebView* content_view_;
   views::Widget* modal_signin_widget_;  // Not owned.
   ui::ModalType dialog_modal_type_;