Enable AutoResize for Constrained Web Dialogs for Mac.

chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm

Index: chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm
diff --git a/chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm b/chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm
index 220299f6aa7e8884d1e398de16523749443517aa..419d9c70528991f4588161977297b08459b11218 100644
--- a/chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm
+++ b/chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm
@@ -10,7 +10,10 @@
 #import "chrome/browser/ui/cocoa/constrained_window/constrained_window_custom_window.h"
 #import "chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h"
 #import "chrome/browser/ui/cocoa/constrained_window/constrained_window_web_dialog_sheet.h"
+#include "chrome/browser/ui/webui/chrome_web_contents_handler.h"
+#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/web_dialogs/web_dialog_delegate.h"
 #include "ui/web_dialogs/web_dialog_ui.h"
@@ -22,19 +25,72 @@ using ui::WebDialogWebContentsDelegate;
 
 namespace {
 
+class ConstrainedWebDialogDelegateMac;
+
+// WebContentsObserver that tracks the lifetime of the WebContents to avoid
+// potential use after destruction.
+class InitiatorWebContentsObserver

[erikchen, 2015/11/11 21:22:30]

What's the point of this class? Why not just use WebContentsObserver in its
place?

[apacible, 2015/11/12 02:02:24]

Removed.

+    : public content::WebContentsObserver {
+  public:
+   explicit InitiatorWebContentsObserver(content::WebContents* web_contents)
+      : content::WebContentsObserver(web_contents) {
+   }
+
+  private:
+    DISALLOW_COPY_AND_ASSIGN(InitiatorWebContentsObserver);
+};
+
+class WebDialogWebContentsDelegateMac

[erikchen, 2015/11/11 21:22:30]

Provide a comment indicating the purpose of this class.

[apacible, 2015/11/12 02:02:24]

Done.

+    : public ui::WebDialogWebContentsDelegate {
+ public:
+  WebDialogWebContentsDelegateMac(content::BrowserContext* browser_context,
+                                  InitiatorWebContentsObserver* observer,
+                                  ConstrainedWebDialogDelegateBase* delegate)
+      : ui::WebDialogWebContentsDelegate(browser_context,
+                                         new ChromeWebContentsHandler()),
+        initiator_observer_(observer),
+        delegate_(delegate) {
+  }
+  ~WebDialogWebContentsDelegateMac() override {}
+
+  void ResizeDueToAutoResize(content::WebContents* source,
+                             const gfx::Size& preferred_size) override {
+    if (!initiator_observer_->web_contents())
+      return;
+    delegate_->ResizeToGivenSize(preferred_size);
+  }
+
+ private:
+  // weak, owned by ConstrainedWebDialogDelegateViewMac.
+  InitiatorWebContentsObserver* const initiator_observer_;
+  ConstrainedWebDialogDelegateBase* delegate_; // weak, owns us.

[erikchen, 2015/11/11 21:22:30]

"us" is ambiguous. For both this member, and the one above, what really matter
is the lifetime semantics. I'd use the comment: "This member must outlive the
instance."

[apacible, 2015/11/12 02:02:24]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(WebDialogWebContentsDelegateMac);
+};
+
 class ConstrainedWebDialogDelegateMac
     : public ConstrainedWebDialogDelegateBase {
  public:
   ConstrainedWebDialogDelegateMac(
       content::BrowserContext* browser_context,
-      WebDialogDelegate* delegate)
-      : ConstrainedWebDialogDelegateBase(browser_context, delegate, NULL) {}
+      WebDialogDelegate* delegate,
+      InitiatorWebContentsObserver* observer)
+      : ConstrainedWebDialogDelegateBase(browser_context, delegate,
+          new WebDialogWebContentsDelegateMac(browser_context, observer,

[erikchen, 2015/11/11 21:22:30]

Can you add a comment to the constructor of ConstrainedWebDialogDelegateBase
indicating that |browser_context| and |delegate| must outlive the instance,
whereas the instance takes ownership of |tab_delegate|? Alternatively, you could
change the paramter to be a scoped_ptr<WebDialogWebContentsDelegate>, which
makes the ownership semantics explicit.

[apacible, 2015/11/12 02:02:23]

Done.

+                                              this)) {}
 
   // WebDialogWebContentsDelegate interface.
   void CloseContents(WebContents* source) override {
     window_->CloseWebContentsModalDialog();
   }
 
+  // ConstrainedWebDialogDelegateBase:
+  void ResizeToGivenSize(const gfx::Size size) override {
+    NSSize updated_preferred_size = NSMakeSize(size.width(),
+                                               size.height());
+    [window_->sheet() ResizeWithNewSize:updated_preferred_size];
+  }
+
   void set_window(ConstrainedWindowMac* window) { window_ = window; }
   ConstrainedWindowMac* window() const { return window_; }
 
@@ -49,13 +105,16 @@ class ConstrainedWebDialogDelegateMac
 
 class ConstrainedWebDialogDelegateViewMac :
     public ConstrainedWindowMacDelegate,
-    public ConstrainedWebDialogDelegate {
+    public ConstrainedWebDialogDelegate,
+    public content::WebContentsObserver {
 
  public:
   ConstrainedWebDialogDelegateViewMac(
       content::BrowserContext* browser_context,
       WebDialogDelegate* delegate,
-      content::WebContents* web_contents);
+      content::WebContents* web_contents,
+      const gfx::Size& min_size,
+      const gfx::Size& max_size);
   ~ConstrainedWebDialogDelegateViewMac() override {}
 
   // ConstrainedWebDialogDelegate interface
@@ -74,16 +133,36 @@ class ConstrainedWebDialogDelegateViewMac :
   gfx::NativeWindow GetNativeDialog() override { return window_; }
   WebContents* GetWebContents() override { return impl_->GetWebContents(); }
   gfx::Size GetMinimumSize() const override {
-    NOTIMPLEMENTED();
-    return gfx::Size();
+    return min_size_;
   }
   gfx::Size GetMaximumSize() const override {
-    NOTIMPLEMENTED();
-    return gfx::Size();
+    return max_size_;
   }
   gfx::Size GetPreferredSize() const override {
-    NOTIMPLEMENTED();
-    return gfx::Size();
+    gfx::Size size;

[erikchen, 2015/11/11 21:22:30]

What does it mean for the preferred size to be 0,0?  Can you add documentation
to chrome/browser/ui/webui/constrained_web_dialog_ui.h?

[apacible, 2015/11/12 02:02:23]

Done.

+    if (!impl_->closed_via_webui()) {
+      NSRect frame = [window_ frame];
+      size = gfx::Size(frame.size.width, frame.size.height);
+    }
+    return size;
+  }
+
+  // content::WebContentsObserver:
+  void RenderViewCreated(content::RenderViewHost* render_view_host) override {
+    if (!max_size_.IsEmpty())
+      EnableAutoResize();
+  }
+  void RenderViewHostChanged(content::RenderViewHost* old_host,
+                             content::RenderViewHost* new_host) override {
+    if (!max_size_.IsEmpty())

[erikchen, 2015/11/11 21:22:30]

Shouldn't this conditional, and the other ones in WebContentsObserver callbacks
be DCHECKs?

[apacible, 2015/11/12 02:02:23]

These should actually all be DCHECK as they're all from WebContentsObserver
which we only use if we want to be able to autoresize the dialog.

[apacible, 2015/11/13 00:04:00]

Updating this - Since ConstrainedWebDialogDelegateViewMac is a
WebContentsObserver (observes in construction, not just with explicity Observe()
calls), these should all be conditional.

+      EnableAutoResize();
+  }
+  void DocumentOnLoadCompletedInMainFrame() override {
+    if (!max_size_.IsEmpty()) {
+      EnableAutoResize();
+      if (initiator_observer_.web_contents())
+        ShowDialog();
+    }
   }
 
   // ConstrainedWindowMacDelegate interface
@@ -93,26 +172,50 @@ class ConstrainedWebDialogDelegateViewMac :
     delete this;
   }
 
+  void ShowDialog() {
+    constrained_window_->ShowDialog();
+  }
+
  private:
+  void EnableAutoResize() {
+    content::RenderViewHost* render_view_host =

[erikchen, 2015/11/11 21:22:30]

This relies on the assumption that GetWebContents() is not nullptr. It's not
immediately clear to me that that's guaranteed to be true.

[apacible, 2015/11/12 02:02:24]

Done.

+        GetWebContents()->GetRenderViewHost();
+    render_view_host->EnableAutoResize(min_size_, max_size_);
+  }
+
+  InitiatorWebContentsObserver initiator_observer_;
   scoped_ptr<ConstrainedWebDialogDelegateMac> impl_;
   scoped_ptr<ConstrainedWindowMac> constrained_window_;
   base::scoped_nsobject<NSWindow> window_;
 
+  // Minimum and maximum sizes to determine dialog bounds for auto-resizing.
+  const gfx::Size min_size_;
+  const gfx::Size max_size_;

[erikchen, 2015/11/11 21:22:30]

It looks like this object has two internal states:
autoresize and non-autoresize. You are encoding this information in
max_size.IsEmpty(), which is logic you use throughout the class. This is not
clear.

I would either add a bool member, or add a function that returns a bool (which
internally checks max size). This makes the internal state explicit. 

[apacible, 2015/11/12 02:02:23]

Done.

+
   DISALLOW_COPY_AND_ASSIGN(ConstrainedWebDialogDelegateViewMac);
 };
 
 ConstrainedWebDialogDelegateViewMac::ConstrainedWebDialogDelegateViewMac(
     content::BrowserContext* browser_context,
     WebDialogDelegate* delegate,
-    content::WebContents* web_contents)
-    : impl_(new ConstrainedWebDialogDelegateMac(browser_context, delegate)) {
+    content::WebContents* web_contents,
+    const gfx::Size& min_size,
+    const gfx::Size& max_size)
+    : initiator_observer_(web_contents),
+      impl_(new ConstrainedWebDialogDelegateMac(browser_context, delegate,
+            &initiator_observer_)),
+      min_size_(min_size),
+      max_size_(max_size) {
+  if (!max_size_.IsEmpty())
+    Observe(GetWebContents());
+
   // Create a window to hold web_contents in the constrained sheet:
   gfx::Size size;
   delegate->GetDialogSize(&size);
   NSRect frame = NSMakeRect(0, 0, size.width(), size.height());
 
-  window_.reset(
-      [[ConstrainedWindowCustomWindow alloc] initWithContentRect:frame]);
+  window_.reset([[ConstrainedWindowCustomWindow alloc]
+                initWithContentRect:frame]);
   [GetWebContents()->GetNativeView() setFrame:frame];
   [GetWebContents()->GetNativeView() setAutoresizingMask:
       NSViewWidthSizable|NSViewHeightSizable];
@@ -121,8 +224,14 @@ ConstrainedWebDialogDelegateViewMac::ConstrainedWebDialogDelegateViewMac(
   base::scoped_nsobject<WebDialogConstrainedWindowSheet> sheet(
       [[WebDialogConstrainedWindowSheet alloc] initWithCustomWindow:window_
                                                   webDialogDelegate:delegate]);
-  constrained_window_.reset(new ConstrainedWindowMac(
-      this, web_contents, sheet));
+
+  if (max_size_.IsEmpty()) {
+    constrained_window_.reset(ShowWebModalDialogMac(
+          this, web_contents, sheet));
+  } else {
+    constrained_window_.reset(CreateWebModalDialogMac(
+          this, web_contents, sheet));
+  }
 
   impl_->set_window(constrained_window_.get());
 }
@@ -134,6 +243,23 @@ ConstrainedWebDialogDelegate* ShowConstrainedWebDialog(
   // Deleted when the dialog closes.
   ConstrainedWebDialogDelegateViewMac* constrained_delegate =
       new ConstrainedWebDialogDelegateViewMac(
-          browser_context, delegate, web_contents);
+          browser_context, delegate, web_contents,
+          gfx::Size(), gfx::Size());
+  return constrained_delegate;
+}
+
+ConstrainedWebDialogDelegate* ShowConstrainedWebDialogWithAutoResize(
+    content::BrowserContext* browser_context,
+    WebDialogDelegate* delegate,
+    content::WebContents* web_contents,
+    const gfx::Size& min_size,
+    const gfx::Size& max_size) {
+  DCHECK(!min_size.IsEmpty());
+  DCHECK(!max_size.IsEmpty());
+  // Deleted when the dialog closes.
+  ConstrainedWebDialogDelegateViewMac* constrained_delegate =
+      new ConstrainedWebDialogDelegateViewMac(
+          browser_context, delegate, web_contents,
+          min_size, max_size);
   return constrained_delegate;
 }