Using native sheet to display modal dialogs for sign in

BUG=677012

Review-Url: https://codereview.chromium.org/2617583006
Cr-Commit-Position: refs/heads/master@{#443241}

chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.h

@@ -12,6 +12,7 @@
 #include "chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h"
 #include "chrome/browser/ui/profile_chooser_constants.h"
 #include "chrome/browser/ui/signin_view_controller_delegate.h"
+#include "ui/base/ui_base_types.h"
 
 @class ConstrainedWindowCustomWindow;
 class ConstrainedWindowMac;
@@ -33,17 +34,6 @@ enum class AccessPoint;
 class SigninViewControllerDelegateMac : public ConstrainedWindowMacDelegate,
                                         public SigninViewControllerDelegate {
  public:
-  // 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.
-  // Otherwise, the window's dimensions will be |frame|.
-  SigninViewControllerDelegateMac(
-      SigninViewController* signin_view_controller,
-      std::unique_ptr<content::WebContents> web_contents,
-      Browser* browser,
-      NSRect frame,
-      bool wait_for_size);
-
   void OnConstrainedWindowClosed(ConstrainedWindowMac* window) override;
 
   // Creates the web view that contains the signin flow in |mode| using
@@ -62,6 +52,21 @@ class SigninViewControllerDelegateMac : public ConstrainedWindowMacDelegate,
       Profile* profile);
 
  private:
+  friend SigninViewControllerDelegate;
+
+  // 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.
+  // Otherwise, the window's dimensions will be |frame|.
+  SigninViewControllerDelegateMac(
+      SigninViewController* signin_view_controller,
+      std::unique_ptr<content::WebContents> web_contents,
+      Browser* browser,
+      NSRect frame,
+      ui::ModalType dialog_modal_type,
+      bool wait_for_size);
+  ~SigninViewControllerDelegateMac() override;
+
   void PerformClose() override;
   void ResizeNativeView(int height) override;
 
@@ -71,7 +76,8 @@ class SigninViewControllerDelegateMac : public ConstrainedWindowMacDelegate,
       content::WebContents* source,
       const content::NativeWebKeyboardEvent& event) override;
 
-  ~SigninViewControllerDelegateMac() override;
+  // Cleans up and deletes this object.
+  void CleanupAndDeleteThis();
 
   // The constrained window opened by this delegate to display signin flow
   // content.
@@ -88,6 +94,9 @@ class SigninViewControllerDelegateMac : public ConstrainedWindowMacDelegate,
 
   Browser* browser_;
 
+  // The dialog modal presentation type.
+  ui::ModalType dialog_modal_type_;
+
   NSRect window_frame_;
 
   DISALLOW_COPY_AND_ASSIGN(SigninViewControllerDelegateMac);

chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.mm

@@ -55,11 +55,13 @@ CGFloat GetSyncConfirmationDialogPreferredHeight(Profile* profile) {
     std::unique_ptr<content::WebContents> web_contents,
     Browser* browser,
     NSRect frame,
+    ui::ModalType dialog_modal_type,
     bool wait_for_size)
     : SigninViewControllerDelegate(signin_view_controller, web_contents.get()),
       web_contents_(std::move(web_contents)),
       wait_for_size_(wait_for_size),
       browser_(browser),
+      dialog_modal_type_(dialog_modal_type),
       window_frame_(frame) {
   DCHECK(browser_);
   DCHECK(browser_->tab_strip_model()->GetActiveWebContents())
@@ -73,8 +75,7 @@ CGFloat GetSyncConfirmationDialogPreferredHeight(Profile* profile) {
 
 void SigninViewControllerDelegateMac::OnConstrainedWindowClosed(
     ConstrainedWindowMac* window) {
-  ResetSigninViewControllerDelegate();
-  delete this;
+  CleanupAndDeleteThis();
 }
 
 // static
@@ -149,8 +150,21 @@ CGFloat GetSyncConfirmationDialogPreferredHeight(Profile* profile) {
 }
 
 void SigninViewControllerDelegateMac::PerformClose() {
-  if (constrained_window_.get())
-    constrained_window_->CloseWebContentsModalDialog();
+  switch (dialog_modal_type_) {
+    case ui::MODAL_TYPE_CHILD:
+      if (constrained_window_.get())
+        constrained_window_->CloseWebContentsModalDialog();
+      break;
+    case ui::MODAL_TYPE_WINDOW:
+      if (window_.get()) {
+        [window_.get().sheetParent endSheet:window_];
+        window_.reset(nil);
+        CleanupAndDeleteThis();
+      }
+      break;
+    default:
+      NOTREACHED() << "Unsupported dialog modal type " << dialog_modal_type_;
+  }
 }
 
 void SigninViewControllerDelegateMac::ResizeNativeView(int height) {
@@ -179,8 +193,18 @@ CGFloat GetSyncConfirmationDialogPreferredHeight(Profile* profile) {
   window_.get().contentView = web_contents_->GetNativeView();
   base::scoped_nsobject<CustomConstrainedWindowSheet> sheet(
       [[CustomConstrainedWindowSheet alloc] initWithCustomWindow:window_]);
-  constrained_window_ =
-      CreateAndShowWebModalDialogMac(this, host_web_contents, sheet);
+  switch (dialog_modal_type_) {
+    case ui::MODAL_TYPE_CHILD:
+      constrained_window_ =
+          CreateAndShowWebModalDialogMac(this, host_web_contents, sheet);
+      break;
+    case ui::MODAL_TYPE_WINDOW:
+      [host_web_contents->GetTopLevelNativeWindow() beginSheet:window_
+                                             completionHandler:nil];
+      break;
+    default:
+      NOTREACHED() << "Unsupported dialog modal type " << dialog_modal_type_;
+  }
 }
 
 void SigninViewControllerDelegateMac::HandleKeyboardEvent(
@@ -196,6 +220,11 @@ CGFloat GetSyncConfirmationDialogPreferredHeight(Profile* profile) {
   }
 }
 
+void SigninViewControllerDelegateMac::CleanupAndDeleteThis() {
+  ResetSigninViewControllerDelegate();
+  delete this;
+}
+
 // static
 SigninViewControllerDelegate*
 SigninViewControllerDelegate::CreateModalSigninDelegate(
@@ -208,7 +237,7 @@ CGFloat GetSyncConfirmationDialogPreferredHeight(Profile* profile) {
       SigninViewControllerDelegateMac::CreateGaiaWebContents(
           nullptr, mode, browser->profile(), access_point),
       browser, NSMakeRect(0, 0, kModalDialogWidth, kFixedGaiaViewHeight),
-      false /* wait_for_size */);
+      ui::MODAL_TYPE_CHILD, false /* wait_for_size */);
 }
 
 // static
@@ -223,7 +252,7 @@ CGFloat GetSyncConfirmationDialogPreferredHeight(Profile* profile) {
       browser,
       NSMakeRect(0, 0, kModalDialogWidth,
                  GetSyncConfirmationDialogPreferredHeight(browser->profile())),
-      true /* wait_for_size */);
+      ui::MODAL_TYPE_WINDOW, true /* wait_for_size */);
 }
 
 // static
@@ -236,5 +265,5 @@ CGFloat GetSyncConfirmationDialogPreferredHeight(Profile* profile) {
       SigninViewControllerDelegateMac::CreateSigninErrorWebContents(
           browser->profile()),
       browser, NSMakeRect(0, 0, kModalDialogWidth, kSigninErrorDialogHeight),
-      true /* wait_for_size */);
+      ui::MODAL_TYPE_WINDOW, true /* wait_for_size */);
 }

chrome/browser/ui/signin_view_controller_delegate.h

@@ -46,11 +46,14 @@ class SigninViewControllerDelegate : public content::WebContentsDelegate {
       SigninViewController* signin_view_controller,
       Browser* browser);
 
-  // Closes the modal sign-in dialog.
+  // 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();
 
   // Either navigates back in the signin flow if the history state allows it or
-  // closes the flow otherwise.
+  // 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();
 
   // This will be called by the base class to request a resize of the native
@@ -79,7 +82,9 @@ class SigninViewControllerDelegate : public content::WebContentsDelegate {
 
   // 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.
+  // 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;
 
  private:

chrome/browser/ui/sync/one_click_signin_sync_starter.cc

@@ -140,7 +140,6 @@ void OneClickSigninSyncStarter::Initialize(Profile* profile, Browser* browser) {
   // will not be able to complete successfully.
   syncer::SyncPrefs sync_prefs(profile_->GetPrefs());
   sync_prefs.SetSyncRequested(true);
-  skip_sync_confirm_ = false;
 }
 
 void OneClickSigninSyncStarter::ConfirmSignin(ProfileMode profile_mode,
@@ -323,14 +322,6 @@ void OneClickSigninSyncStarter::CompleteInitForNewProfile(
       Initialize(new_profile, nullptr);
       DCHECK_EQ(profile_, new_profile);
 
-#if defined(OS_MACOSX)
-      // On macOS, the sync confirmation dialog is web-contents modal and thus
-      // it is dismissed on tab navigation (which always occurs when signing in
-      // to a new profile).
-      // Skip sync confirmation on macOS to workaround this issue.
-      skip_sync_confirm_ = true;
-#endif
-
       // We've transferred our credentials to the new profile - notify that
       // the signin for the original profile was cancelled (must do this after
       // we have called Initialize() with the new profile, as otherwise this
@@ -501,11 +492,6 @@ void OneClickSigninSyncStarter::AccountAddedToCookie(
   // Regardless of whether the account was successfully added or not,
   // continue with sync starting.
 
-  if (skip_sync_confirm_) {
-    OnSyncConfirmationUIClosed(LoginUIService::ABORT_SIGNIN);
-    return;
-  }
-
   if (switches::UsePasswordSeparatedSigninFlow()) {
     // Under the new signin flow, the sync confirmation dialog should always be
     // shown regardless of |start_mode_|. |sync_setup_completed_callback_| will

chrome/browser/ui/sync/one_click_signin_sync_starter.h

@@ -260,12 +260,6 @@ class OneClickSigninSyncStarter : public SigninTracker::Observer,
   // Prevents Sync from running until configuration is complete.
   std::unique_ptr<syncer::SyncSetupInProgressHandle> sync_blocker_;
 
-  // Temporary flag used only on macOS to disable new sync confirm page if user
-  // choose to create a new profile.
-  //
-  // TODO(msarda): Remove this flag once the https://crbug.com/677012 fixed.
-  bool skip_sync_confirm_;
-
   base::WeakPtrFactory<OneClickSigninSyncStarter> weak_pointer_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(OneClickSigninSyncStarter);