Make the MediaRouterDialogDelegate set initial dialog size in OSX

Make the MediaRouterDialogDelegate set initial dialog size in OSX

This CL fixes the bug in which the media router dialog would sometimes fail to show on OSX.

We make MediaRouterDialogDelegate::GetDialogSize() set the dialog size the first time it's called by WebDialogConstrainedWindowSheet::updateSheetPosition(), so that the custom window is not initialized with size 0.

BUG=633403
Committed: https://crrev.com/3613d6709e4b55f87c6855f71708b2c91b5308c0
Cr-Commit-Position: refs/heads/master@{#418720}

[takumif, 2016-08-29 21:30:13]

Patchset #2 (id:20001) has been deleted

[takumif, 2016-08-29 21:30:19]

Patchset #1 (id:1) has been deleted

[takumif, 2016-08-29 21:46:40]

Description was changed from

==========
no dialog bug

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

to

==========
Make the MediaRouterDialogDelegate set initial dialog size in OSX

This CL fixes the bug in which the media router dialog would sometimes fail to
show on OSX.

We make MediaRouterDialogDelegate::GetDialogSize() set the dialog size first
time it's called by WebDialogConstrainedWindowSheet::updateSheetPosition(), so
that the dialog is not initialized with size 0.

BUG=633403
==========

[takumif, 2016-08-29 21:46:58]

Description was changed from

==========
Make the MediaRouterDialogDelegate set initial dialog size in OSX

This CL fixes the bug in which the media router dialog would sometimes fail to
show on OSX.

We make MediaRouterDialogDelegate::GetDialogSize() set the dialog size first
time it's called by WebDialogConstrainedWindowSheet::updateSheetPosition(), so
that the dialog is not initialized with size 0.

BUG=633403
==========

to

==========
Make the MediaRouterDialogDelegate set initial dialog size in OSX

This CL fixes the bug in which the media router dialog would sometimes fail to
show on OSX.

We make MediaRouterDialogDelegate::GetDialogSize() set the dialog size the first
time it's called by WebDialogConstrainedWindowSheet::updateSheetPosition(), so
that the dialog is not initialized with size 0.

BUG=633403
==========

[takumif, 2016-08-29 21:48:11]

Description was changed from

==========
Make the MediaRouterDialogDelegate set initial dialog size in OSX

This CL fixes the bug in which the media router dialog would sometimes fail to
show on OSX.

We make MediaRouterDialogDelegate::GetDialogSize() set the dialog size the first
time it's called by WebDialogConstrainedWindowSheet::updateSheetPosition(), so
that the dialog is not initialized with size 0.

BUG=633403
==========

to

==========
Make the MediaRouterDialogDelegate set initial dialog size in OSX

This CL fixes the bug in which the media router dialog would sometimes fail to
show on OSX.

We make MediaRouterDialogDelegate::GetDialogSize() set the dialog size the first
time it's called by WebDialogConstrainedWindowSheet::updateSheetPosition(), so
that the custom window is not initialized with size 0.

BUG=633403
==========

[takumif, 2016-08-29 21:52:10]

Patchset #1 (id:40001) has been deleted

[takumif, 2016-08-29 21:58:11]

Patchset #1 (id:60001) has been deleted

[takumif, 2016-08-29 22:14:43]

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

[takumif, 2016-08-29 22:14:43]

thakis@chromium.org: Please review changes in
constrained_window_web_dialog_sheet.mm

apacible@chromium.org: Please review changes in both the files

Thank you!

[apacible, 2016-08-31 18:19:12]

https://codereview.chromium.org/2275413002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc
(right):

https://codereview.chromium.org/2275413002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:85:
if (size->IsEmpty())
I would prefer handling everything on the sheets side rather than requiring
future autosizing dialogs to send an arbitrary size for mac as a workaround.

[takumif, 2016-08-31 22:44:30]

https://codereview.chromium.org/2275413002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc
(right):

https://codereview.chromium.org/2275413002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:85:
if (size->IsEmpty())
On 2016/08/31 18:19:12, apacible wrote:
> I would prefer handling everything on the sheets side rather than requiring
> future autosizing dialogs to send an arbitrary size for mac as a workaround.

I don't think the sheet has enough information to be able to set the initial
window size.

I looked a bit more into where else GetDialogSize() gets called, and it seems
that on other platforms we also expect it to modify |size| when it's empty [1],
in a similar manner as my change in WebDialogConstrainedWindowSheet above. I'm
not seeing any change in behavior on Linux regardless of whether or not we set
the |size| here though.

So perhaps we should set the |size| for OSX for now, and also set for other
platforms later if that's preferable?

[1]
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/constrained_web_...

[apacible, 2016-09-06 17:24:54]

Any thoughts on why this is only an issue in M54? In the bug, you mentioned you
couldn't repro with M53. The size was removed in M51.

https://codereview.chromium.org/2275413002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc
(right):

https://codereview.chromium.org/2275413002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:84:
// dialog may fail to show.
Add a comment mentioning this is done because specifically because
WebDialogConstrainedWindowSheet's |size| cannot be empty.

https://codereview.chromium.org/2275413002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:85:
if (size->IsEmpty())
On 2016/08/31 22:44:30, takumif wrote:
> On 2016/08/31 18:19:12, apacible wrote:
> > I would prefer handling everything on the sheets side rather than requiring
> > future autosizing dialogs to send an arbitrary size for mac as a workaround.
> 
> I don't think the sheet has enough information to be able to set the initial
> window size.
> 
> I looked a bit more into where else GetDialogSize() gets called, and it seems
> that on other platforms we also expect it to modify |size| when it's empty
[1],
> in a similar manner as my change in WebDialogConstrainedWindowSheet above. I'm
> not seeing any change in behavior on Linux regardless of whether or not we set
> the |size| here though.
> 
> So perhaps we should set the |size| for OSX for now, and also set for other
> platforms later if that's preferable?
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/constrained_web_...

Acknowledged. I don't think having an arbitrary size in updateSheetsPosition()
would work well if the dialog doesn't immediately sized. This is fine, don't set
the size with other platforms if not needed.

[takumif, 2016-09-06 19:01:37]

I'm not sure why it didn't repro on M52/53. Also, it reproed on M54/OSX 10.10
but not on M54/OSX 10.11. Perhaps it has to do with specific Cocoa versions?

https://codereview.chromium.org/2275413002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc
(right):

https://codereview.chromium.org/2275413002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:84:
// dialog may fail to show.
On 2016/09/06 17:24:53, apacible wrote:
> Add a comment mentioning this is done because specifically because
> WebDialogConstrainedWindowSheet's |size| cannot be empty.

Done.

[takumif, 2016-09-08 17:56:30]

A friendly ping -- please take a look, thank you!

[apacible, 2016-09-08 18:49:05]

lgtm (Oops, thought I sent that with my last comments!)

[takumif, 2016-09-09 22:10:18]

takumif@chromium.org changed required reviewers:
+ thakis@chromium.org

[takumif, 2016-09-12 21:27:31]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[takumif, 2016-09-12 21:27:43]

The CQ bit was unchecked by takumif@chromium.org

[takumif, 2016-09-12 21:33:27]

takumif@chromium.org changed reviewers:
+ groby@chromium.org
- thakis@chromium.org

[takumif, 2016-09-12 21:33:27]

takumif@chromium.org changed required reviewers:
- thakis@chromium.org

[takumif, 2016-09-12 21:33:28]

groby@, please take a look at constrained_window_web_dialog_sheet.mm. Thank you!

[groby-ooo-7-16, 2016-09-14 19:30:03]

I think there's a much simpler fix. Please LMK if that works for you.

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

https://codereview.chromium.org/2275413002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_web_dialog_sheet.mm:27:
DCHECK(!size.IsEmpty());
I'm assuming this triggers only if updateSheetPosition is called before
current_size_ is set via resizeWithNewSize - we're never resizing back to (0,0),
correct?

In that case, I'd suggest initializing |current_size|_ to
ui::kWindowSizeDeterminedLater.size 
(in -initWithCustomWindow)

I think that would do the trick, without the need of a per-platform ifdef, or
more code here. If there's a different sequence that leads here, could you
describe that sequence?

https://codereview.chromium.org/2275413002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_web_dialog_sheet.mm:35:
- (void)resizeWithNewSize:(NSSize)size {
You probably want a DCHECK(!size.IsEmpty()) here, since empty rects are never a
good plan for windows.

[takumif, 2016-09-14 21:35:38]

Patchset #3 (id:120001) has been deleted

[takumif, 2016-09-14 21:46:44]

Thank you for your advice, Rachel. I've made changes accordingly. Please take a
look, thanks!

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

https://codereview.chromium.org/2275413002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_web_dialog_sheet.mm:27:
DCHECK(!size.IsEmpty());
On 2016/09/14 19:30:02, groby wrote:
> I'm assuming this triggers only if updateSheetPosition is called before
> current_size_ is set via resizeWithNewSize - we're never resizing back to
(0,0),
> correct?
> 
> In that case, I'd suggest initializing |current_size|_ to
> ui::kWindowSizeDeterminedLater.size 
> (in -initWithCustomWindow)
> 
> I think that would do the trick, without the need of a per-platform ifdef, or
> more code here. If there's a different sequence that leads here, could you
> describe that sequence?

Right, the problem was with updateSheetPosition being called before resizing,
and setting it to ui::kWindowSizeDeterminedLater.size solves the problem.

https://codereview.chromium.org/2275413002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/constrained_window/constrained_window_web_dialog_sheet.mm:35:
- (void)resizeWithNewSize:(NSSize)size {
On 2016/09/14 19:30:03, groby wrote:
> You probably want a DCHECK(!size.IsEmpty()) here, since empty rects are never
a
> good plan for windows.

Checking that height and width are positive, as NSSize doesn't have an isEmpty
method.

[groby-ooo-7-16, 2016-09-14 22:17:23]

LGTM

[takumif, 2016-09-14 22:31:28]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 22:32:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 23:57:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 23:57:37]

Dry run: This issue passed the CQ dry run.

[takumif, 2016-09-15 00:02:34]

The CQ bit was checked by takumif@chromium.org

[takumif, 2016-09-15 00:02:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
apacible@chromium.org
Link to the patchset: https://codereview.chromium.org/2275413002/#ps140001
(title: "Use kWindowSizeDeterminedLater")

[commit-bot: I haz the power, 2016-09-15 00:03:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 00:14:44]

Description was changed from

==========
Make the MediaRouterDialogDelegate set initial dialog size in OSX

This CL fixes the bug in which the media router dialog would sometimes fail to
show on OSX.

We make MediaRouterDialogDelegate::GetDialogSize() set the dialog size the first
time it's called by WebDialogConstrainedWindowSheet::updateSheetPosition(), so
that the custom window is not initialized with size 0.

BUG=633403
==========

to

==========
Make the MediaRouterDialogDelegate set initial dialog size in OSX

This CL fixes the bug in which the media router dialog would sometimes fail to
show on OSX.

We make MediaRouterDialogDelegate::GetDialogSize() set the dialog size the first
time it's called by WebDialogConstrainedWindowSheet::updateSheetPosition(), so
that the custom window is not initialized with size 0.

BUG=633403
==========

[commit-bot: I haz the power, 2016-09-15 00:14:45]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2016-09-15 00:17:26]

Description was changed from

==========
Make the MediaRouterDialogDelegate set initial dialog size in OSX

This CL fixes the bug in which the media router dialog would sometimes fail to
show on OSX.

We make MediaRouterDialogDelegate::GetDialogSize() set the dialog size the first
time it's called by WebDialogConstrainedWindowSheet::updateSheetPosition(), so
that the custom window is not initialized with size 0.

BUG=633403
==========

to

==========
Make the MediaRouterDialogDelegate set initial dialog size in OSX

This CL fixes the bug in which the media router dialog would sometimes fail to
show on OSX.

We make MediaRouterDialogDelegate::GetDialogSize() set the dialog size the first
time it's called by WebDialogConstrainedWindowSheet::updateSheetPosition(), so
that the custom window is not initialized with size 0.

BUG=633403

Committed: https://crrev.com/3613d6709e4b55f87c6855f71708b2c91b5308c0
Cr-Commit-Position: refs/heads/master@{#418720}
==========

[commit-bot: I haz the power, 2016-09-15 00:17:27]

Patchset 3 (id:??) landed as
https://crrev.com/3613d6709e4b55f87c6855f71708b2c91b5308c0
Cr-Commit-Position: refs/heads/master@{#418720}