[UI] Make auto-resizable constrained web dialog work even when initiated

chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc

Index: chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc
diff --git a/chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc b/chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc
index 1f6302730572dbd5ca30c14391fd545ce5d555f0..2c61272b2b9405475dae2c2f84854c95f9d26158 100644
--- a/chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc
+++ b/chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc
@@ -80,11 +80,15 @@ class WebDialogWebContentsDelegateViews
     // Sets WebView's preferred size based on auto-resized contents.
     web_view_->SetPreferredSize(preferred_size);
 
+    content::WebContents* top_level_web_contents =

[msw, 2016/09/22 03:27:35]

It seems odd to present a dialog from an embedded webview over the top-level
embedder's web contents. Please post a screenshot of this behavior, and please
seek a security review. Your cl description seems to indicate that this is the
current behavior for other (non-resizable) dialogs; if so, please also point out
some examples of that existing behavior? What do other platforms do?

[imcheng, 2016/09/22 04:32:23]

Indeed, we currently allow embedded <webview> to create non-autoresizable
constrained web dialogs over the top-level WebContents. It is currently used by
the auth login flow, but lazyboy@ (CC'ed) may be able to provide additional
background about this. The dialog has the same behavior in terms of appearance /
modality, but I can provide a screenshot tomorrow.

Also, the autoresizable constrained web dialog already works on Mac today. The
Mac implementation of the auto-resizable dialog creates a window using the
embedder's top-level WebContents:
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/constrained_wind...

BTW, who do you suggest looping in for security review?

[msw, 2016/09/22 13:36:19]

Thanks for the info, this probably doesn't need security review if it's already
done for Mac and for Views non-autoresizable constrained web dialogs.

[imcheng, 2016/09/22 17:20:58]

Acknowledged.

+        constrained_window::GetTopLevelWebContents(
+            initiator_observer_->web_contents());
     constrained_window::UpdateWebContentsModalDialogPosition(
         web_view_->GetWidget(),
         web_modal::WebContentsModalDialogManager::FromWebContents(
-            initiator_observer_->web_contents())->delegate()->
-                GetWebContentsModalDialogHost());
+            top_level_web_contents)

[msw, 2016/09/22 13:36:19]

Check for null before using |top_level_web_contents|?

[wjmaclean, 2016/09/22 15:36:48]

Definitely check for null ... there's no guarantee this will be non-null.

[imcheng, 2016/09/22 17:20:57]

Done. Although I did notice that constrained_window_views.cc was calling this
and using the result without checking for nullptr? 

+            ->delegate()
+            ->GetWebContentsModalDialogHost());
   }
 
  private:
@@ -238,8 +242,10 @@ class ConstrainedWebDialogDelegateViewViews
   }
   void DocumentOnLoadCompletedInMainFrame() override {
     if (!max_size_.IsEmpty() && initiator_observer_.web_contents()) {
-      constrained_window::ShowModalDialog(GetWidget()->GetNativeWindow(),
-                                          initiator_observer_.web_contents());
+      constrained_window::ShowModalDialog(
+          GetWidget()->GetNativeWindow(),
+          constrained_window::GetTopLevelWebContents(

[msw, 2016/09/22 13:36:19]

Ditto, check for null first?

[imcheng, 2016/09/22 17:20:57]

Done.

+              initiator_observer_.web_contents()));
     }
   }
 
@@ -287,6 +293,10 @@ ConstrainedWebDialogDelegate* ShowConstrainedWebDialogWithAutoResize(
       new ConstrainedWebDialogDelegateViewViews(
           browser_context, delegate, web_contents,
           min_size, max_size);
-  constrained_window::CreateWebModalDialogViews(dialog, web_contents);
+  // For embedded WebContents, use the embedder's WebContents for constrained
+  // window.
+  content::WebContents* top_level_web_contents =
+      constrained_window::GetTopLevelWebContents(web_contents);
+  constrained_window::CreateWebModalDialogViews(dialog, top_level_web_contents);

[msw, 2016/09/22 13:36:19]

nit: maybe DCHECK that |top_level_web_contents| isn't null here?

[imcheng, 2016/09/22 17:20:57]

Done.

   return dialog;
 }