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

chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc

 // Copyright (c) 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.
 
 #include "chrome/browser/ui/webui/constrained_web_dialog_delegate_base.h"
 
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ skipping 62 matching lines @@
                              const gfx::Size& preferred_size) override {
     if (source != web_view_->GetWebContents())
       return;
 
     if (!initiator_observer_->web_contents())
       return;
 
     // 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:
   InitiatorWebContentsObserver* const initiator_observer_;
   views::WebView* web_view_;
 
   DISALLOW_COPY_AND_ASSIGN(WebDialogWebContentsDelegateViews);
 };
 
 class ConstrainedWebDialogDelegateViews
@@ skipping 133 matching lines @@
     if (!max_size_.IsEmpty())
       EnableAutoResize();
   }
   void RenderViewHostChanged(content::RenderViewHost* old_host,
                              content::RenderViewHost* new_host) override {
     if (!max_size_.IsEmpty())
       EnableAutoResize();
   }
   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()));
     }
   }
 
  private:
   void EnableAutoResize() {
     content::RenderViewHost* render_view_host =
         GetWebContents()->GetRenderViewHost();
     render_view_host->EnableAutoResize(min_size_, max_size_);
   }
 
@@ skipping 27 matching lines @@
     ui::WebDialogDelegate* delegate,
     content::WebContents* web_contents,
     const gfx::Size& min_size,
     const gfx::Size& max_size) {
   DCHECK(!min_size.IsEmpty());
   DCHECK(!max_size.IsEmpty());
   ConstrainedWebDialogDelegateViewViews* dialog =
       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;
 }