Enable AutoResize for Constrained Web Dialogs for Mac.

Enable AutoResize for Constrained Web Dialogs for Mac.

Currently only constrained web dialogs for views (Linux/Windows) are able to
autoresize. This change implements the option to pass in minimum and maximum
sizes and enabling autoresizing functionality for OSX.

This change adds two static functions for options on whether to create a
ConstrainedWindowsMac that autoresizes or is of fixed size.

BUG=217034
Committed: https://crrev.com/0dbc8ff6d68291773b8b233a19e399d00f9262b0
Cr-Commit-Position: refs/heads/master@{#359497}

[apacible, 2015-11-03 18:45:47]

Patchset #1 (id:1) has been deleted

[apacible, 2015-11-03 19:09:40]

Description was changed from

==========
asdf

BUG=
==========

to

==========
[Not For Review]
==========

[apacible, 2015-11-05 05:35:05]

Patchset #1 (id:20001) has been deleted

[apacible, 2015-11-05 05:35:11]

Patchset #1 (id:40001) has been deleted

[apacible, 2015-11-05 05:45:15]

Patchset #1 (id:60001) has been deleted

[apacible, 2015-11-06 00:31:13]

Patchset #2 (id:100001) has been deleted

[apacible, 2015-11-06 00:31:18]

Patchset #2 (id:120001) has been deleted

[apacible, 2015-11-06 00:59:49]

Patchset #1 (id:80001) has been deleted

[apacible, 2015-11-06 01:05:11]

Patchset #1 (id:140001) has been deleted

[apacible, 2015-11-06 01:07:26]

Description was changed from

==========
[Not For Review]
==========

to

==========
[Work In Progress] Enable AutoResize for Constrained Web Dialogs for Mac.

Currently only constrained web dialogs for views (linux/windows). This change
implements the option to pass in minimum and maximum sizes and enabling
autoresizing functionality.

BUG=217034
==========

[apacible, 2015-11-06 01:31:55]

Patchset #1 (id:150001) has been deleted

[apacible, 2015-11-07 23:47:45]

Patchset #1 (id:170001) has been deleted

[apacible, 2015-11-08 06:05:52]

Patchset #1 (id:190001) has been deleted

[apacible, 2015-11-08 06:06:10]

Patchset #2 (id:230001) has been deleted

[apacible, 2015-11-08 06:06:18]

Patchset #1 (id:210001) has been deleted

[apacible, 2015-11-09 19:28:23]

Patchset #3 (id:290001) has been deleted

[apacible, 2015-11-09 19:28:42]

Patchset #2 (id:270001) has been deleted

[apacible, 2015-11-09 23:37:54]

Patchset #2 (id:310001) has been deleted

[apacible, 2015-11-10 03:58:11]

Patchset #4 (id:370001) has been deleted

[apacible, 2015-11-10 03:58:16]

Patchset #3 (id:350001) has been deleted

[apacible, 2015-11-10 03:58:27]

Patchset #2 (id:330001) has been deleted

[apacible, 2015-11-10 04:21:01]

Patchset #2 (id:390001) has been deleted

[apacible, 2015-11-10 05:23:18]

Patchset #2 (id:410001) has been deleted

[apacible, 2015-11-10 08:14:24]

Patchset #2 (id:430001) has been deleted

[apacible, 2015-11-10 08:34:21]

Patchset #1 (id:250001) has been deleted

[apacible, 2015-11-10 08:34:30]

Patchset #1 (id:450001) has been deleted

[apacible, 2015-11-10 19:09:20]

Patchset #1 (id:450002) has been deleted

[apacible, 2015-11-10 19:09:29]

Patchset #1 (id:480001) has been deleted

[apacible, 2015-11-10 19:12:21]

Patchset #1 (id:500001) has been deleted

[apacible, 2015-11-10 19:12:31]

Description was changed from

==========
[Work In Progress] Enable AutoResize for Constrained Web Dialogs for Mac.

Currently only constrained web dialogs for views (linux/windows). This change
implements the option to pass in minimum and maximum sizes and enabling
autoresizing functionality.

BUG=217034
==========

to

==========
Enable AutoResize for Constrained Web Dialogs for Mac.

Currently only constrained web dialogs for views (linux/windows). This change
implements the option to pass in minimum and maximum sizes and enabling
autoresizing functionality.

BUG=217034
==========

[apacible, 2015-11-10 19:18:12]

Patchset #1 (id:500002) has been deleted

[apacible, 2015-11-10 19:25:17]

Patchset #1 (id:530001) has been deleted

[apacible, 2015-11-10 19:27:44]

apacible@chromium.org changed reviewers:
+ thakis@chromium.org

[apacible, 2015-11-10 19:29:01]

Description was changed from

==========
Enable AutoResize for Constrained Web Dialogs for Mac.

Currently only constrained web dialogs for views (linux/windows). This change
implements the option to pass in minimum and maximum sizes and enabling
autoresizing functionality.

BUG=217034
==========

to

==========
Enable AutoResize for Constrained Web Dialogs for Mac.

Currently only constrained web dialogs for views (Linux/Windows). This change
implements the option to pass in minimum and maximum sizes and enabling
autoresizing functionality for OSX.

BUG=217034
==========

[apacible, 2015-11-10 19:30:25]

Description was changed from

==========
Enable AutoResize for Constrained Web Dialogs for Mac.

Currently only constrained web dialogs for views (Linux/Windows). This change
implements the option to pass in minimum and maximum sizes and enabling
autoresizing functionality for OSX.

BUG=217034
==========

to

==========
Enable AutoResize for Constrained Web Dialogs for Mac.

Currently only constrained web dialogs for views (Linux/Windows). This change
implements the option to pass in minimum and maximum sizes and enabling
autoresizing functionality for OSX.

This change adds two static functions for options on whether to create a
ConstrainedWindowsMac that autoresizes or is of fixed size.

BUG=217034
==========

[apacible, 2015-11-10 19:31:05]

PTAL, thanks!

[apacible, 2015-11-11 19:02:08]

apacible@chromium.org changed reviewers:
+ erikchen@chromium.org

[apacible, 2015-11-11 19:02:20]

+erikchen

PTAL, thanks!

[apacible, 2015-11-11 19:06:00]

Description was changed from

==========
Enable AutoResize for Constrained Web Dialogs for Mac.

Currently only constrained web dialogs for views (Linux/Windows). This change
implements the option to pass in minimum and maximum sizes and enabling
autoresizing functionality for OSX.

This change adds two static functions for options on whether to create a
ConstrainedWindowsMac that autoresizes or is of fixed size.

BUG=217034
==========

to

==========
Enable AutoResize for Constrained Web Dialogs for Mac.

Currently only constrained web dialogs for views (Linux/Windows) are able to
autoresize. This change implements the option to pass in minimum and maximum
sizes and enabling autoresizing functionality for OSX.

This change adds two static functions for options on whether to create a
ConstrainedWindowsMac that autoresizes or is of fixed size.

BUG=217034
==========

[apacible, 2015-11-11 19:06:29]

Description was changed from

==========
Enable AutoResize for Constrained Web Dialogs for Mac.

Currently only constrained web dialogs for views (Linux/Windows) are able to
autoresize. This change implements the option to pass in minimum and maximum
sizes and enabling autoresizing functionality for OSX.

This change adds two static functions for options on whether to create a
ConstrainedWindowsMac that autoresizes or is of fixed size.

BUG=217034
==========

to

==========
Enable AutoResize for Constrained Web Dialogs for Mac.

Currently only constrained web dialogs for views (Linux/Windows) are able to
autoresize.

This change implements the option to pass in minimum and maximum sizes and
enabling autoresizing functionality for OSX.

This change adds two static functions for options on whether to create a
ConstrainedWindowsMac that autoresizes or is of fixed size.

BUG=217034
==========

[erikchen, 2015-11-11 21:22:31]

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm (right):

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:32: class
InitiatorWebContentsObserver
What's the point of this class? Why not just use WebContentsObserver in its
place?

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:43: class
WebDialogWebContentsDelegateMac
Provide a comment indicating the purpose of this class.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:66:
ConstrainedWebDialogDelegateBase* delegate_; // weak, owns us.
"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."

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:79: new
WebDialogWebContentsDelegateMac(browser_context, observer,
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.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:142: gfx::Size
size;
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?

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:157: if
(!max_size_.IsEmpty())
Shouldn't this conditional, and the other ones in WebContentsObserver callbacks
be DCHECKs?

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:181:
content::RenderViewHost* render_view_host =
This relies on the assumption that GetWebContents() is not nullptr. It's not
immediately clear to me that that's guaranteed to be true.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:193: const
gfx::Size max_size_;
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.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h
(right):

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:27:
ConstrainedWindowMac* ShowWebModalDialogMac(
s/Show/CreateAndShow/

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:43:
ConstrainedWindowMac(ConstrainedWindowMacDelegate* delegate);
explicit

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:46:
id<ConstrainedWindowSheet> sheet);
The public interface for ConstrainedWindowMac is confusing.

There are two constructors, which create objects with different internal states.
Depending on which constructor was used, a different set of public functions is
available. Using the other functions will result in incorrect behavior. 

Can you clean up this interface? Is the new constructor really necessary? Do we
need a new wrapping object?  If you think that the current interface is the best
interface, then please add comments, as well as assertions in the implementation
to ensure that these conditions are enforced.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:69:
id<ConstrainedWindowSheet> sheet_;
What are the expected/required lifetime semantics for sheet_? Since it's an
Obj-C object, should this class maintain a reference to it?

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:71:
web_modal::WebContentsModalDialogManager* dialog_manager_;
What are you expected/required lifetime semantics on dialog_manager_? At first
glance, it looks like the lifetime semantics are tied to the web_contents, in
which case perhaps there should be a method that dynamically fetches the
dialog_manager_, and the method can assert that the web_contents is still alive?

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet.h
(right):

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet.h:27: -
(void)ResizeWithNewSize:(NSSize)size;
nit: s/Resize/resize/

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/constrained_window/constrained_window_web_dialog_sheet.mm
(right):

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_web_dialog_sheet.mm:32:
[super updateSheetPosition];
why super instead of self?

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (left):

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/webu...
chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:177:
EXPECT_EQ(gfx::Size(), web_contents->GetPreferredSize());
Why did you remove this expectation? At the very least, shouldn't it be present
on other OSes still?

[apacible, 2015-11-11 23:39:15]

Patchset #2 (id:570001) has been deleted

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

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm (right):

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:32: class
InitiatorWebContentsObserver
On 2015/11/11 21:22:30, erikchen wrote:
> What's the point of this class? Why not just use WebContentsObserver in its
> place?

Removed.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:43: class
WebDialogWebContentsDelegateMac
On 2015/11/11 21:22:30, erikchen wrote:
> Provide a comment indicating the purpose of this class.

Done.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:66:
ConstrainedWebDialogDelegateBase* delegate_; // weak, owns us.
On 2015/11/11 21:22:30, erikchen wrote:
> "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."

Done.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:79: new
WebDialogWebContentsDelegateMac(browser_context, observer,
On 2015/11/11 21:22:30, erikchen wrote:
> 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.

Done.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:142: gfx::Size
size;
On 2015/11/11 21:22:30, erikchen wrote:
> 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?

Done.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:157: if
(!max_size_.IsEmpty())
On 2015/11/11 21:22:30, erikchen wrote:
> Shouldn't this conditional, and the other ones in WebContentsObserver
callbacks
> be DCHECKs?

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.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:181:
content::RenderViewHost* render_view_host =
On 2015/11/11 21:22:30, erikchen wrote:
> This relies on the assumption that GetWebContents() is not nullptr. It's not
> immediately clear to me that that's guaranteed to be true.

Done.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:193: const
gfx::Size max_size_;
On 2015/11/11 21:22:30, erikchen wrote:
> 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. 

Done.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h
(right):

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:27:
ConstrainedWindowMac* ShowWebModalDialogMac(
On 2015/11/11 21:22:30, erikchen wrote:
> s/Show/CreateAndShow/

Done.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:43:
ConstrainedWindowMac(ConstrainedWindowMacDelegate* delegate);
On 2015/11/11 21:22:30, erikchen wrote:
> explicit

Done.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:46:
id<ConstrainedWindowSheet> sheet);
On 2015/11/11 21:22:30, erikchen wrote:
> The public interface for ConstrainedWindowMac is confusing.
> 
> There are two constructors, which create objects with different internal
states.
> Depending on which constructor was used, a different set of public functions
is
> available. Using the other functions will result in incorrect behavior. 
> 
> Can you clean up this interface? Is the new constructor really necessary? Do
we
> need a new wrapping object?  If you think that the current interface is the
best
> interface, then please add comments, as well as assertions in the
implementation
> to ensure that these conditions are enforced.

Cleaned up:
- Back to one constructor.
- One ShowDialog() function.
- Renamed ShowDialog() to ShowWebContentsModalDialog() for consistency with
CloseWebContentsModalDialog().

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:69:
id<ConstrainedWindowSheet> sheet_;
On 2015/11/11 21:22:30, erikchen wrote:
> What are the expected/required lifetime semantics for sheet_? Since it's an
> Obj-C object, should this class maintain a reference to it?

Ownership should be passed to the WebContentsModalDialogManager when we call
ShowDialogWithManager(), similar to |native_manager_|.

I'm not familiar enough with Obj-C (and any special properties of Obj-C objects)
to answer your second question.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:71:
web_modal::WebContentsModalDialogManager* dialog_manager_;
On 2015/11/11 21:22:30, erikchen wrote:
> What are you expected/required lifetime semantics on dialog_manager_? At first
> glance, it looks like the lifetime semantics are tied to the web_contents, in
> which case perhaps there should be a method that dynamically fetches the
> dialog_manager_, and the method can assert that the web_contents is still
alive?

Correct, it's tied to web_contents. Updated to dynamically fetch
dialog_manager_.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet.h
(right):

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet.h:27: -
(void)ResizeWithNewSize:(NSSize)size;
On 2015/11/11 21:22:30, erikchen wrote:
> nit: s/Resize/resize/

Done.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/constrained_window/constrained_window_web_dialog_sheet.mm
(right):

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_web_dialog_sheet.mm:32:
[super updateSheetPosition];
On 2015/11/11 21:22:30, erikchen wrote:
> why super instead of self?

self's updateSheetPosition sets the contentSize to a manually specified dialog
size (used for fixed dialogs), whereas we want to resize to the passed in size
instead.

Using super instead of self bypasses the setContentSize in line 25.

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (left):

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/webu...
chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:177:
EXPECT_EQ(gfx::Size(), web_contents->GetPreferredSize());
On 2015/11/11 21:22:30, erikchen wrote:
> Why did you remove this expectation? At the very least, shouldn't it be
present
> on other OSes still?

Oops, returned.

[apacible, 2015-11-12 18:21:49]

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (left):

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/webu...
chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:177:
EXPECT_EQ(gfx::Size(), web_contents->GetPreferredSize());
On 2015/11/12 02:02:24, apacible wrote:
> On 2015/11/11 21:22:30, erikchen wrote:
> > Why did you remove this expectation? At the very least, shouldn't it be
> present
> > on other OSes still?
> 
> Oops, returned.

I remember why I'd removed it initially. In views, the active web contents for
the tab don't update their preferred size, but in cocoa, they do. This was the
case prior to my changes (just verified on tip); I didn't realize that when I
wrote these tests last year.

I'm looking into why this is the case (views vs cocoa), and am leaning towards
removing this line.

The preferred size of the active web contents of the *tab* aren't relevant to
the dialog and dialog content's sizes; the delegate's contents would be the
relevant content.

[erikchen, 2015-11-12 18:31:27]

https://codereview.chromium.org/1430023002/diff/590001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h
(right):

https://codereview.chromium.org/1430023002/diff/590001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:70:
id<ConstrainedWindowSheet> sheet_;
Instead of using an id (raw pointer), use a scoped_nsobject or
scoped_nsprotocol, which is the Obj-C requivalent of a scoped_refptr.

https://codereview.chromium.org/1430023002/diff/590001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/constrained_window/constrained_window_web_dialog_sheet.mm
(right):

https://codereview.chromium.org/1430023002/diff/590001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_web_dialog_sheet.mm:32:
[super updateSheetPosition];
Please add a comment for why you use super rather than self.

[apacible, 2015-11-12 21:57:42]

I opted to switch back to the previous changes for tests; see earlier comment
thread.

https://codereview.chromium.org/1430023002/diff/590001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h
(right):

https://codereview.chromium.org/1430023002/diff/590001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:70:
id<ConstrainedWindowSheet> sheet_;
On 2015/11/12 18:31:27, erikchen wrote:
> Instead of using an id (raw pointer), use a scoped_nsobject or
> scoped_nsprotocol, which is the Obj-C requivalent of a scoped_refptr. 

Done.

https://codereview.chromium.org/1430023002/diff/590001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/constrained_window/constrained_window_web_dialog_sheet.mm
(right):

https://codereview.chromium.org/1430023002/diff/590001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_web_dialog_sheet.mm:32:
[super updateSheetPosition];
On 2015/11/12 18:31:27, erikchen wrote:
> Please add a comment for why you use super rather than self.

Done.

[erikchen, 2015-11-12 22:14:58]

https://codereview.chromium.org/1430023002/diff/610001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h
(right):

https://codereview.chromium.org/1430023002/diff/610001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:71:
base::scoped_nsobject<id> sheet_;
using scoped_nsprotocol should allow you to retain type information
(ConstrainedWindowSheet), whereas an id is more like a void pointer.

https://codereview.chromium.org/1430023002/diff/610001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm
(right):

https://codereview.chromium.org/1430023002/diff/610001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:38:
sheet_(sheet) {
You probably want [sheet retain] here.

https://codereview.chromium.org/1430023002/diff/610001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:59:
[sheet_.release() sheetWindow], native_manager_.Pass());
This will leak a reference to the sheet, which is not what we want. You can just
use sheet_.get() here.

[apacible, 2015-11-13 00:01:08]

Patchset #4 (id:630001) has been deleted

[apacible, 2015-11-13 00:04:01]

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm (right):

https://codereview.chromium.org/1430023002/diff/550001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:157: if
(!max_size_.IsEmpty())
On 2015/11/12 02:02:23, apacible wrote:
> On 2015/11/11 21:22:30, erikchen wrote:
> > Shouldn't this conditional, and the other ones in WebContentsObserver
> callbacks
> > be DCHECKs?
> 
> 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.

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

https://codereview.chromium.org/1430023002/diff/610001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h
(right):

https://codereview.chromium.org/1430023002/diff/610001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:71:
base::scoped_nsobject<id> sheet_;
On 2015/11/12 22:14:57, erikchen wrote:
> using scoped_nsprotocol should allow you to retain type information
> (ConstrainedWindowSheet), whereas an id is more like a void pointer. 

Done.

https://codereview.chromium.org/1430023002/diff/610001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm
(right):

https://codereview.chromium.org/1430023002/diff/610001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:38:
sheet_(sheet) {
On 2015/11/12 22:14:57, erikchen wrote:
> You probably want [sheet retain] here.

Done.

https://codereview.chromium.org/1430023002/diff/610001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:59:
[sheet_.release() sheetWindow], native_manager_.Pass());
On 2015/11/12 22:14:57, erikchen wrote:
> This will leak a reference to the sheet, which is not what we want. You can
just
> use sheet_.get() here.

Done.

[erikchen, 2015-11-13 00:21:24]

lgtm

[apacible, 2015-11-13 00:44:44]

Thanks!

Nico, PTAL for OWNER, thanks!

[Nico, 2015-11-13 01:37:20]

is there any test coverage for the new functionality added in this patch?

[apacible, 2015-11-13 04:06:14]

On 2015/11/13 01:37:20, Nico wrote:
> is there any test coverage for the new functionality added in this patch?

See constrained_web_dialog_ui_browsertest.cc. Previously the tests were only run
for non-mac builds.

[Nico, 2015-11-13 04:42:51]

Ah, I missed that, thanks! In that case, rs-lgtm :-)

[apacible, 2015-11-13 05:39:47]

Thanks!

[apacible, 2015-11-13 05:39:54]

The CQ bit was checked by apacible@chromium.org

[commit-bot: I haz the power, 2015-11-13 05:40:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1430023002/650001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1430023002/650001

[commit-bot: I haz the power, 2015-11-13 05:45:39]

Committed patchset #4 (id:650001)

[commit-bot: I haz the power, 2015-11-13 05:46:54]

Patchset 4 (id:??) landed as
https://crrev.com/0dbc8ff6d68291773b8b233a19e399d00f9262b0
Cr-Commit-Position: refs/heads/master@{#359497}

[cbiesinger, 2015-11-13 19:00:42]

So I'm only a blink sheriff, not chromium, but this may have broken
browser_tests:

https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/11410
https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/114...

[erikchen, 2015-11-13 19:02:06]

On 2015/11/13 19:00:42, cbiesinger wrote:
> So I'm only a blink sheriff, not chromium, but this may have broken
> browser_tests:
> 
>
https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/11410
>
https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/114...

yup, looks like it.

[apacible, 2015-11-13 19:08:59]

On 2015/11/13 19:02:06, erikchen wrote:
> On 2015/11/13 19:00:42, cbiesinger wrote:
> > So I'm only a blink sheriff, not chromium, but this may have broken
> > browser_tests:
> > 
> >
>
https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/11410
> >
>
https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/114...
> 
> yup, looks like it.

Sorry about that! I'll revert this patch.

[apacible, 2015-11-13 19:10:09]

A revert of this CL (patchset #4 id:650001) has been created in
https://codereview.chromium.org/1440173003/ by apacible@chromium.org.

The reason for reverting is: This is breaking Mac10.9 browser_tests:

https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/11410
https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/114....