Enable AutoResize for Constrained Web Dialogs

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 c98917173b820a6ed5319febb73e00ea237ce188..730475d638897fcb793cb74a72b27f2a0a5d8344 100644
--- a/chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc
+++ b/chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc
@@ -9,10 +9,14 @@
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/webui/chrome_web_contents_handler.h"
 #include "components/constrained_window/constrained_window_views.h"
+#include "components/web_modal/popup_manager.h"

[miu, 2014/11/26 00:47:43]

Is this used anywhere?

[apacible, 2014/11/26 17:35:14]

Nope, fixed.

+#include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/native_web_keyboard_event.h"
+#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/views/controls/webview/unhandled_keyboard_event_handler.h"
 #include "ui/views/controls/webview/webview.h"
+#include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/dialog_delegate.h"
 #include "ui/web_dialogs/web_dialog_delegate.h"
@@ -30,6 +34,7 @@ class WebDialogWebContentsDelegateViews
                                          new ChromeWebContentsHandler()),
         initiator_(initiator),
         web_view_(web_view) {
+    preferred_size_ = gfx::Size();
   }
   ~WebDialogWebContentsDelegateViews() override {}
 
@@ -52,9 +57,26 @@ class WebDialogWebContentsDelegateViews
 #endif
   }
 
+  void ResizeDueToAutoResize(content::WebContents* source,
+                             const gfx::Size& preferred_size) override {
+    web_view_->SetPreferredSize(preferred_size);
+    preferred_size_ = preferred_size;
+
+    constrained_window::UpdateWebContentsModalDialogPosition(
+        web_view_->GetWidget(),
+        web_modal::WebContentsModalDialogManager::FromWebContents(
+            initiator_)->delegate()->GetWebContentsModalDialogHost());
+    web_view_->SetBoundsRect(web_view_->bounds());

[miu, 2014/11/26 00:47:43]

Is this call to SetBoundsRect() necessary?  Seems like it should be a no-op (see
implementation of SetBoundsRect in src/ui/views/view.cc).

[apacible, 2014/11/26 17:35:13]

No, and fixed.

+  }
+
+  gfx::Size GetPreferredSize() const {

[miu, 2014/11/26 00:47:43]

I don't think this method is needed.  See comments for |delegate_| member (in
the ConstrainedWebDialogDelegateViewViews class) below.

[apacible, 2014/11/26 17:35:13]

Done.

+    return preferred_size_;
+  }
+
  private:
   content::WebContents* initiator_;
   views::WebView* web_view_;
+  gfx::Size preferred_size_;

[miu, 2014/11/26 00:47:43]

Don't need this if you don't need GetPreferredSize() (see prior comment).

[apacible, 2014/11/26 17:35:13]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(WebDialogWebContentsDelegateViews);
 };
@@ -111,9 +133,38 @@ class ConstrainedWebDialogDelegateViewViews
       : views::WebView(browser_context),
         impl_(new ConstrainedWebDialogDelegateViews(browser_context, delegate,
                                                     web_contents, this)) {
+    delegate_ = static_cast<WebDialogWebContentsDelegateViews*>(
+        GetWebContents()->GetDelegate());
+    DCHECK(delegate_);
     SetWebContents(GetWebContents());
     AddAccelerator(ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE));
+
+    web_contents_ = web_contents;
+    min_size_ = gfx::Size();

[miu, 2014/11/26 00:47:43]

In C++, both |min_size_| and |max_size_| are automatically initialized with
their zero-arg constructor.  So, you don't need to explicitly assign them here.

[apacible, 2014/11/26 17:35:14]

Done.

+    max_size_ = gfx::Size();
+    auto_resize_enabled_ = false;
   }
+  ConstrainedWebDialogDelegateViewViews(

[miu, 2014/11/26 00:47:43]

nit: Add newline above this line.

[apacible, 2014/11/26 17:35:14]

Removed one constructor.

+      content::BrowserContext* browser_context,
+      ui::WebDialogDelegate* delegate,
+      content::WebContents* web_contents,
+      gfx::Size min_size,
+      gfx::Size max_size)
+      : views::WebView(browser_context),

[miu, 2014/11/26 00:47:43]

Chromium now allows C++11 delegated constructors, which allows you to greatly
simplify things.  In other words, I think you want the 2nd constructor to be:

  ConstrainedWebDialogDelegateViewViews(
      content::BrowserContext* browser_context,
      ui::WebDialogDelegate* delegate,
      content::WebContents* web_contents,
      gfx::Size min_size,
      gfx::Size max_size)
    : ConstrainedWebDialogDelegateViewViews(browser_context, delegate,
web_contents),
      min_size_(min_size),
      max_size_(max_size) {
      DCHECK(!min_size.IsEmpty());
      DCHECK(!max_size.IsEmpty());
  }

[apacible, 2014/11/26 17:35:14]

Combined both constructors into one.

+        impl_(new ConstrainedWebDialogDelegateViews(browser_context, delegate,
+                                                    web_contents, this)) {
+    delegate_ = static_cast<WebDialogWebContentsDelegateViews*>(
+        GetWebContents()->GetDelegate());
+    DCHECK(delegate_);
+    SetWebContents(GetWebContents());
+    AddAccelerator(ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE));
+
+    DCHECK(!min_size.IsEmpty());
+    DCHECK(!max_size.IsEmpty());
+    SetBoundsForAutoResize(min_size, max_size);
+
+    web_contents_ = web_contents;
+   }
   ~ConstrainedWebDialogDelegateViewViews() override {}
 
   // ConstrainedWebDialogDelegate:
@@ -135,6 +186,13 @@ class ConstrainedWebDialogDelegateViewViews
   content::WebContents* GetWebContents() override {
     return impl_->GetWebContents();
   }
+  void EnableAutoResize(const gfx::Size& min_size, const gfx::Size& max_size)

[miu, 2014/11/26 00:47:43]

This should be a private method.  Also, you don't need the two arguments, since
you can reference |min_size_| and |max_size_|.

[apacible, 2014/11/26 17:35:14]

Done.

+      override {
+    content::RenderViewHost* render_view_host =
+        GetWebContents()->GetRenderViewHost();
+    DCHECK(render_view_host);
+    render_view_host->EnableAutoResize(min_size, max_size);
+  }
 
   // views::WidgetDelegate:
   views::View* GetInitiallyFocusedView() override { return this; }
@@ -146,6 +204,7 @@ class ConstrainedWebDialogDelegateViewViews
   const views::Widget* GetWidget() const override { return View::GetWidget(); }
   base::string16 GetWindowTitle() const override {
     return impl_->closed_via_webui() ? base::string16() :
+

[miu, 2014/11/26 00:47:43]

nit: Remove this newline.

[apacible, 2014/11/26 17:35:14]

Done.

         GetWebDialogDelegate()->GetDialogTitle();
   }
   views::View* GetContentsView() override { return this; }
@@ -168,18 +227,54 @@ class ConstrainedWebDialogDelegateViewViews
   }
   gfx::Size GetPreferredSize() const override {
     gfx::Size size;
-    if (!impl_->closed_via_webui())
-      GetWebDialogDelegate()->GetDialogSize(&size);
+    if (!impl_->closed_via_webui()) {
+      size = delegate_->GetPreferredSize();

[miu, 2014/11/26 00:47:43]

If you unwind the code, this looks identical to:

  size = WebView::GetPreferredSize();

So, you don't need |delegate_|.

[apacible, 2014/11/26 17:35:14]

Done.

+      if (size.IsEmpty())
+        GetWebDialogDelegate()->GetDialogSize(&size);
+    }
     return size;
   }
   gfx::Size GetMinimumSize() const override {
     // Return an empty size so that we can be made smaller.
     return gfx::Size();

[miu, 2014/11/26 00:47:43]

Should this method return min_size_ instead?

Also, should you override View::GetMaximumSize() to return max_size_ if
!max_size_.IsEmpty()?

[apacible, 2014/11/26 17:35:14]

Done.

Talked offline about functionality of GetMaximumSize().

   }
+  void RenderViewCreated(content::RenderViewHost* render_view_host) override {
+    if (auto_resize_enabled_)
+      EnableAutoResize(min_size_, max_size_);
+  }
+  void RenderViewHostChanged(content::RenderViewHost* old_host,
+                             content::RenderViewHost* new_host) override {
+    if (auto_resize_enabled_)
+      EnableAutoResize(min_size_, max_size_);
+  }
+  void DocumentOnLoadCompletedInMainFrame() override {
+    if (auto_resize_enabled_) {
+      EnableAutoResize(min_size_, max_size_);
+      constrained_window::ShowWebModalDialogViews(this, web_contents_);
+    }
+  }
 
  private:
   scoped_ptr<ConstrainedWebDialogDelegateViews> impl_;
 
+  content::WebContents* web_contents_;

[miu, 2014/11/26 00:47:43]

Don't need this.  The code can always call the web_contents() method, which is
inherited from WebView (which inherits from WebContentsObserver).

[apacible, 2014/11/26 17:35:13]

Talked offline -- keeping web_contents_ but renaming as initiator_web_contents_.

GetWebContents() and web_contents() refer to WebContents from
ConstrainedWebDialogDelegateBase, but this is referring to
WebDialogWebContentsDelegateView's initiator WebContents.

+
+  gfx::Size min_size_;

[miu, 2014/11/26 00:47:43]

Use 'const gfx::Size', since these are not modified after construction.

[apacible, 2014/11/26 17:35:14]

Done.

+  gfx::Size max_size_;
+
+  bool auto_resize_enabled_;

[miu, 2014/11/26 00:47:44]

Don't need this extra bool member.  The code could simply check whether
!max_size_.IsEmpty() instead.

[apacible, 2014/11/26 17:35:14]

Done.

+
+  WebDialogWebContentsDelegateViews* delegate_;

[miu, 2014/11/26 00:47:43]

Don't need this one, either.  ;-)

[apacible, 2014/11/26 17:35:15]

Done.

+
+  void SetBoundsForAutoResize(const gfx::Size& min_size,

[miu, 2014/11/26 00:47:43]

Only the ctor calls this method.  Assuming you agree with my suggestion above
(about how to define the 2nd constructor), you can get rid of this method.

[apacible, 2014/11/26 17:35:14]

Done.

+                              const gfx::Size& max_size) {
+    DCHECK(!min_size.IsEmpty());
+    DCHECK(!max_size.IsEmpty());
+    min_size_ = min_size;
+    max_size_ = max_size;
+    auto_resize_enabled_ = true;
+  }
+
   DISALLOW_COPY_AND_ASSIGN(ConstrainedWebDialogDelegateViewViews);
 };
 
@@ -195,3 +290,17 @@ ConstrainedWebDialogDelegate* CreateConstrainedWebDialog(
   constrained_window::ShowWebModalDialogViews(dialog, web_contents);
   return dialog;
 }
+
+ConstrainedWebDialogDelegate* CreateConstrainedWebDialogWithAutoResize(
+    content::BrowserContext* browser_context,
+    ui::WebDialogDelegate* delegate,
+    content::WebContents* web_contents,
+    gfx::Size min_size,
+    gfx::Size max_size) {
+  ConstrainedWebDialogDelegateViewViews* dialog =
+      new ConstrainedWebDialogDelegateViewViews(
+          browser_context, delegate, web_contents,
+          min_size, max_size);
+  constrained_window::CreateWebModalDialogViews(dialog, web_contents);

[miu, 2014/11/26 00:47:43]

Did you mean to call ShowWebModalDialogViews() instead?  Or, does the showing
happen later?

If you did mean to call CreateWebModalDialogViews(), it seems like the other
CreateConstrainedWebDialog() method should be renamed to
ShowConstrainedWebDialog() for readability.

[apacible, 2014/11/26 17:35:13]

The showing happens later. I will rename the former method.

+  return dialog;
+}