[Reland] Enable AutoResize for Constrained Web Dialogs for Mac.

[Reland] 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.

The initial patch was reverted because of flaky tests in Mac 10.9. Upon further investigation, the ConstrainedWebDialogBrowserTest tests were also flaky on other Mac versions, such as 10.10. At the time, the test would fail once every 4-5,000 runs. After picking up this patch again, I was unable to repro the failing test after ~20,000 runs.

The original patch can be found at: https://codereview.chromium.org/1430023002

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

[apacible, 2015-11-13 23:10:38]

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

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

patch from issue 1430023002 at patchset 650001
(http://crrev.com/1430023002#ps650001)
==========

to

==========
[Reland] [Work In Progress] 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.

The initial patch was reverted because of flaky tests in Mac 10.9.

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

[apacible, 2015-11-13 23:17:46]

Description was changed from

==========
[Reland] [Work In Progress] 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.

The initial patch was reverted because of flaky tests in Mac 10.9.

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

to

==========
[Work In Progress] [Reland] 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.

The initial patch was reverted because of flaky tests in Mac 10.9.

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

[apacible, 2015-11-16 18:23:33]

Description was changed from

==========
[Work In Progress] [Reland] 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.

The initial patch was reverted because of flaky tests in Mac 10.9.

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

to

==========
[Work In Progress] [Reland] 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.

The initial patch was reverted because of flaky tests in Mac 10.9. Upon further
investigation, the ConstrainedWebDialogBrowserTest tests were also flaky on
other Mac versions, such as 10.10.

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

[apacible, 2015-11-16 23:42:41]

Patchset #1 (id:1) has been deleted

[apacible, 2015-11-16 23:51:44]

Description was changed from

==========
[Work In Progress] [Reland] 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.

The initial patch was reverted because of flaky tests in Mac 10.9. Upon further
investigation, the ConstrainedWebDialogBrowserTest tests were also flaky on
other Mac versions, such as 10.10.

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

to

==========
[Work In Progress] [Reland] 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.

The initial patch was reverted because of flaky tests in Mac 10.9. Upon further
investigation, the ConstrainedWebDialogBrowserTest tests were also flaky on
other Mac versions, such as 10.10.

The original patch can be found at: https://codereview.chromium.org/1430023002

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

[apacible, 2015-12-03 21:54:37]

Description was changed from

==========
[Work In Progress] [Reland] 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.

The initial patch was reverted because of flaky tests in Mac 10.9. Upon further
investigation, the ConstrainedWebDialogBrowserTest tests were also flaky on
other Mac versions, such as 10.10.

The original patch can be found at: https://codereview.chromium.org/1430023002

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

to

==========
[Reland] 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.

The initial patch was reverted because of flaky tests in Mac 10.9. Upon further
investigation, the ConstrainedWebDialogBrowserTest tests were also flaky on
other Mac versions, such as 10.10.

The original patch can be found at: https://codereview.chromium.org/1430023002

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

[apacible, 2016-02-02 22:08:38]

Patchset #3 (id:60001) has been deleted

[apacible, 2016-02-02 22:26:11]

Patchset #2 (id:40001) has been deleted

[apacible, 2016-02-03 04:28:15]

Patchset #5 (id:140001) has been deleted

[apacible, 2016-02-03 04:28:22]

Patchset #4 (id:120001) has been deleted

[apacible, 2016-02-03 05:21:40]

Patchset #4 (id:160001) has been deleted

[apacible, 2016-02-03 16:34:34]

Patchset #4 (id:180001) has been deleted

[apacible, 2016-02-03 18:06:51]

Patchset #5 (id:220001) has been deleted

[apacible, 2016-02-03 18:06:57]

Patchset #5 (id:240001) has been deleted

[apacible, 2016-02-03 18:16:18]

Patchset #4 (id:200001) has been deleted

[apacible, 2016-02-03 18:22:55]

Patchset #4 (id:260001) has been deleted

[apacible, 2016-02-03 18:44:12]

Patchset #4 (id:280001) has been deleted

[apacible, 2016-02-03 18:48:58]

Description was changed from

==========
[Reland] 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.

The initial patch was reverted because of flaky tests in Mac 10.9. Upon further
investigation, the ConstrainedWebDialogBrowserTest tests were also flaky on
other Mac versions, such as 10.10.

The original patch can be found at: https://codereview.chromium.org/1430023002

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

to

==========
[Reland] 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.

The initial patch was reverted because of flaky tests in Mac 10.9. Upon further
investigation, the ConstrainedWebDialogBrowserTest tests were also flaky on
other Mac versions, such as 10.10. At the time, the test would fail once every
4-5,000 runs. After picking up this patch again, I was unable to repro the
failing test after ~20,000 runs.

The original patch can be found at: https://codereview.chromium.org/1430023002

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

[apacible, 2016-02-03 18:48:59]

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

[apacible, 2016-02-03 18:56:06]

Previously the initial patch was reverted because of flaky tests on Mac.
Initially I was able to hit a failure once every few thousand test runs, but I
haven't been able to repro it again after ~20,000 runs recently, so I'm sending
this out again.

PTAL, thanks!
+thakis for OWNER on everything
+erikchen for cocoa feedback

Patch 1 is the same changes I landed in the previous patch[1].
Patch 2 is the rebase after a couple months.
Patches 3+ are new changes.

[1] https://codereview.chromium.org/1430023002

[erikchen, 2016-02-03 19:11:27]

I reviewed the diff between ps2 and ps4.

https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm
(right):

https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm:64: if
(client_->DialogWasShown())
I don't understand why this code was changed. You have a comment on line #62
that says "client_ might delete itself", and then you use it on line 64?

[apacible, 2016-02-03 19:17:27]

https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm
(right):

https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm:64: if
(client_->DialogWasShown())
On 2016/02/03 19:11:27, erikchen wrote:
> I don't understand why this code was changed. You have a comment on line #62
> that says "client_ might delete itself", and then you use it on line 64?

Fixed. This should not have be changed.

[erikchen, 2016-02-03 19:26:33]

On 2016/02/03 19:17:27, apacible wrote:
>
https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm
> (right):
> 
>
https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm:64: if
> (client_->DialogWasShown())
> On 2016/02/03 19:11:27, erikchen wrote:
> > I don't understand why this code was changed. You have a comment on line #62
> > that says "client_ might delete itself", and then you use it on line 64?
> 
> Fixed. This should not have be changed.

I glanced through the changes in the CL again, and they seem pretty reasonable,
but I didn't do a thorough review. I'm trusting my previous review in
https://codereview.chromium.org/1430023002. lgtm

[Nico, 2016-02-04 22:28:29]

https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac_browsertest.mm
(right):

https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac_browsertest.mm:166:
ConstrainedWindowMac* dialog =
doesn't this leak?

https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac_browsertest.mm:167:
CreateAndShowWebModalDialogMac(&delegate, tab1_, sheet_);
this should probably return a unique_ptr, right?

https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (right):

https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:175: //
creation.
is there any harm in setting this to 1, 1 on all platforms?

[apacible, 2016-02-05 01:21:50]

Patchset #6 (id:340001) has been deleted

[apacible, 2016-02-05 01:55:01]

Patchset #6 (id:360001) has been deleted

[apacible, 2016-02-05 03:46:00]

https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac_browsertest.mm
(right):

https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac_browsertest.mm:166:
ConstrainedWindowMac* dialog =
On 2016/02/04 22:28:29, Nico wrote:
> doesn't this leak?

Fixed.

https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac_browsertest.mm:167:
CreateAndShowWebModalDialogMac(&delegate, tab1_, sheet_);
On 2016/02/04 22:28:29, Nico wrote:
> this should probably return a unique_ptr, right?

Done.

https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (right):

https://codereview.chromium.org/1446623003/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:175: //
creation.
On 2016/02/04 22:28:29, Nico wrote:
> is there any harm in setting this to 1, 1 on all platforms?

The initial dialog size for views is empty since the preferred size is initially
determined by WebView/View:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

[Nico, 2016-02-05 15:31:10]

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

https://codereview.chromium.org/1446623003/diff/380001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:28:
ConstrainedWindowMac* CreateAndShowWebModalDialogMac(
i mean this should return a unique_ptr, so that it's impossible to use this
wrong

https://codereview.chromium.org/1446623003/diff/380001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:34:
ConstrainedWindowMac* CreateWebModalDialogMac(
this too

[apacible, 2016-02-05 21:40:42]

Switched to unique_ptr.

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

https://codereview.chromium.org/1446623003/diff/380001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:28:
ConstrainedWindowMac* CreateAndShowWebModalDialogMac(
On 2016/02/05 15:31:10, Nico wrote:
> i mean this should return a unique_ptr, so that it's impossible to use this
> wrong

Done.

https://codereview.chromium.org/1446623003/diff/380001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:34:
ConstrainedWindowMac* CreateWebModalDialogMac(
On 2016/02/05 15:31:10, Nico wrote:
> this too

Done.

[Nico, 2016-02-05 21:52:40]

lgtm

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

https://codereview.chromium.org/1446623003/diff/380001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:28:
ConstrainedWindowMac* CreateAndShowWebModalDialogMac(
On 2016/02/05 21:40:41, apacible wrote:
> On 2016/02/05 15:31:10, Nico wrote:
> > i mean this should return a unique_ptr, so that it's impossible to use this
> > wrong
> 
> Done.

Sorry, returning a scoped_ptr would've been ok too and would've been less churn
for you. Should've been more clear -- the idea was just to make it impossible to
use this function and accidentally leak the return value.

[apacible, 2016-02-06 17:31:11]

On 2016/02/05 21:52:40, Nico wrote:
> lgtm
> 
>
https://codereview.chromium.org/1446623003/diff/380001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h
> (right):
> 
>
https://codereview.chromium.org/1446623003/diff/380001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:28:
> ConstrainedWindowMac* CreateAndShowWebModalDialogMac(
> On 2016/02/05 21:40:41, apacible wrote:
> > On 2016/02/05 15:31:10, Nico wrote:
> > > i mean this should return a unique_ptr, so that it's impossible to use
this
> > > wrong
> > 
> > Done.
> 
> Sorry, returning a scoped_ptr would've been ok too and would've been less
churn
> for you. Should've been more clear -- the idea was just to make it impossible
to
> use this function and accidentally leak the return value.

No worries, thanks for the feedback!

[apacible, 2016-02-06 17:31:19]

The CQ bit was checked by apacible@chromium.org

[apacible, 2016-02-06 17:31:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1446623003/#ps440001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-02-06 17:32:42]

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

[commit-bot: I haz the power, 2016-02-06 18:35:03]

Description was changed from

==========
[Reland] 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.

The initial patch was reverted because of flaky tests in Mac 10.9. Upon further
investigation, the ConstrainedWebDialogBrowserTest tests were also flaky on
other Mac versions, such as 10.10. At the time, the test would fail once every
4-5,000 runs. After picking up this patch again, I was unable to repro the
failing test after ~20,000 runs.

The original patch can be found at: https://codereview.chromium.org/1430023002

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

to

==========
[Reland] 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.

The initial patch was reverted because of flaky tests in Mac 10.9. Upon further
investigation, the ConstrainedWebDialogBrowserTest tests were also flaky on
other Mac versions, such as 10.10. At the time, the test would fail once every
4-5,000 runs. After picking up this patch again, I was unable to repro the
failing test after ~20,000 runs.

The original patch can be found at: https://codereview.chromium.org/1430023002

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

[commit-bot: I haz the power, 2016-02-06 18:35:04]

Committed patchset #9 (id:440001)

[commit-bot: I haz the power, 2016-02-06 18:36:18]

Description was changed from

==========
[Reland] 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.

The initial patch was reverted because of flaky tests in Mac 10.9. Upon further
investigation, the ConstrainedWebDialogBrowserTest tests were also flaky on
other Mac versions, such as 10.10. At the time, the test would fail once every
4-5,000 runs. After picking up this patch again, I was unable to repro the
failing test after ~20,000 runs.

The original patch can be found at: https://codereview.chromium.org/1430023002

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

to

==========
[Reland] 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.

The initial patch was reverted because of flaky tests in Mac 10.9. Upon further
investigation, the ConstrainedWebDialogBrowserTest tests were also flaky on
other Mac versions, such as 10.10. At the time, the test would fail once every
4-5,000 runs. After picking up this patch again, I was unable to repro the
failing test after ~20,000 runs.

The original patch can be found at: https://codereview.chromium.org/1430023002

BUG=217034

Committed: https://crrev.com/c47b9b44157a34aa5df0acebe357dc0164f49887
Cr-Commit-Position: refs/heads/master@{#374039}
==========

[commit-bot: I haz the power, 2016-02-06 18:36:20]

Patchset 9 (id:??) landed as
https://crrev.com/c47b9b44157a34aa5df0acebe357dc0164f49887
Cr-Commit-Position: refs/heads/master@{#374039}

[apacible, 2016-02-07 20:54:08]

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

The reason for reverting is: Flaky on Mac 10.9 again.

See:
https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests
https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29.