Enable AutoResize for Constrained Web Dialogs

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/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/browser_finder.h"
 #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"
 #include "ui/web_dialogs/web_dialog_ui.h"
 
 namespace {
 
 class WebDialogWebContentsDelegateViews
     : public ui::WebDialogWebContentsDelegate {
  public:
   WebDialogWebContentsDelegateViews(content::BrowserContext* browser_context,
                                     content::WebContents* initiator,
                                     views::WebView* web_view)
       : ui::WebDialogWebContentsDelegate(browser_context,
                                          new ChromeWebContentsHandler()),
         initiator_(initiator),
         web_view_(web_view) {
+    preferred_size_ = gfx::Size();
   }
   ~WebDialogWebContentsDelegateViews() override {}
 
   // ui::WebDialogWebContentsDelegate:
   void WebContentsFocused(content::WebContents* contents) override {
     // Ensure the WebView is focused when its WebContents is focused.
     web_view_->RequestFocus();
   }
   void HandleKeyboardEvent(
       content::WebContents* source,
       const content::NativeWebKeyboardEvent& event) override {
     // Forward shortcut keys in dialog to our initiator's delegate.
     // http://crbug.com/104586
     // Disabled on Mac due to http://crbug.com/112173
 #if !defined(OS_MACOSX)
     auto delegate = initiator_->GetDelegate();
     if (!delegate)
       return;
     delegate->HandleKeyboardEvent(initiator_, event);
 #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);
 };
 
 class ConstrainedWebDialogDelegateViews
     : public ConstrainedWebDialogDelegateBase {
  public:
   ConstrainedWebDialogDelegateViews(content::BrowserContext* context,
                                     ui::WebDialogDelegate* delegate,
                                     content::WebContents* web_contents,
@@ skipping 36 matching lines @@
       public ConstrainedWebDialogDelegate,
       public views::WidgetDelegate {
  public:
   ConstrainedWebDialogDelegateViewViews(
       content::BrowserContext* browser_context,
       ui::WebDialogDelegate* delegate,
       content::WebContents* web_contents)
       : 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:
   const ui::WebDialogDelegate* GetWebDialogDelegate() const override {
     return impl_->GetWebDialogDelegate();
   }
   ui::WebDialogDelegate* GetWebDialogDelegate() override {
     return impl_->GetWebDialogDelegate();
   }
   void OnDialogCloseFromWebUI() override {
     return impl_->OnDialogCloseFromWebUI();
   }
   void ReleaseWebContentsOnDialogClose() override {
     return impl_->ReleaseWebContentsOnDialogClose();
   }
   web_modal::NativeWebContentsModalDialog GetNativeDialog() override {
     return impl_->GetNativeDialog();
   }
   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; }
   void WindowClosing() override {
     if (!impl_->closed_via_webui())
       GetWebDialogDelegate()->OnDialogClosed(std::string());
   }
   views::Widget* GetWidget() override { return View::GetWidget(); }
   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; }
   views::NonClientFrameView* CreateNonClientFrameView(
       views::Widget* widget) override {
     return views::DialogDelegate::CreateDialogFrameView(widget);
   }
   bool ShouldShowCloseButton() const override {
     // No close button if the dialog doesn't want a title bar.
     return impl_->GetWebDialogDelegate()->ShouldShowDialogTitle();
   }
   ui::ModalType GetModalType() const override { return ui::MODAL_TYPE_CHILD; }
 
   // views::WebView:
   bool AcceleratorPressed(const ui::Accelerator& accelerator) override {
     // Pressing ESC closes the dialog.
     DCHECK_EQ(ui::VKEY_ESCAPE, accelerator.key_code());
     GetWidget()->Close();
     return true;
   }
   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);
 };
 
 }  // namespace
 
 ConstrainedWebDialogDelegate* CreateConstrainedWebDialog(
     content::BrowserContext* browser_context,
     ui::WebDialogDelegate* delegate,
     content::WebContents* web_contents) {
   ConstrainedWebDialogDelegateViewViews* dialog =
       new ConstrainedWebDialogDelegateViewViews(
           browser_context, delegate, web_contents);
   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;
+}