Limit constrained windows to the size of the parent view.

Limit constrained windows to the size of the parent view.

Constrained windows would get drawn partially outside the browser
window, obscuring some of the contents. This CL limits their size so
everything is in view. This mimics the behavior of the old style
dialogs.

This CL also changes the size request to use the current preferred size
of the view, instead of reusing the existing size. Some dialogs (like
the print preview) changes their size preferences when the browser
window size changes, so the old size might not be optimal any more.

Additionally, added a GetMinimumSize to the bubble frame view, as
previously GetPreferredSize would get used, effectively undoing the
additional restrictions placed based on Window sizes.

BUG=272760, 274236, 276150
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221573

[Rune Fevang, 2013-08-22 01:55:37]

Made some changes to fix one more bug (crbug.com/276150).

[Mike Wittman, 2013-08-22 17:46:10]

IIRC the print preview dialog insets itself from the edges of the
WebContentsView; does that logic need to be updated with this change?

https://codereview.chromium.org/22903022/diff/8001/chrome/browser/ui/views/co...
File chrome/browser/ui/views/constrained_window_views.cc (right):

https://codereview.chromium.org/22903022/diff/8001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_views.cc:582: gfx::Point position =
dialog_host->GetDialogPosition(size);
Have you tested on other web contents modal dialogs, and while resizing the
browser window?

https://codereview.chromium.org/22903022/diff/8001/components/web_modal/web_c...
File components/web_modal/web_contents_modal_dialog_host.h (right):

https://codereview.chromium.org/22903022/diff/8001/components/web_modal/web_c...
components/web_modal/web_contents_modal_dialog_host.h:46: virtual gfx::Size
GetMaximumDialogSize() = 0;
Probably needs to be implemented for chromeos::WebUILoginView, and the other
platform-specific NativeAppWindow subclasses as well.

https://codereview.chromium.org/22903022/diff/8001/ui/views/bubble/bubble_fra...
File ui/views/bubble/bubble_frame_view.h (right):

https://codereview.chromium.org/22903022/diff/8001/ui/views/bubble/bubble_fra...
ui/views/bubble/bubble_frame_view.h:92: gfx::Size GetNeededSize(const gfx::Size&
client_size);
GetSizeForClientSize maybe?

[Rune Fevang, 2013-08-23 02:57:43]

On 2013/08/22 17:46:10, Mike Wittman wrote:
> IIRC the print preview dialog insets itself from the edges of the
> WebContentsView; does that logic need to be updated with this change?

No, that code is called when I ask for the non client view's preferred size. It
doesn't work perfectly at the moment (it asks for a smaller size when there are
toolbar present at the top), but that was broken before this change also.

https://chromiumcodereview.appspot.com/22903022/diff/8001/chrome/browser/ui/v...
File chrome/browser/ui/views/constrained_window_views.cc (right):

https://chromiumcodereview.appspot.com/22903022/diff/8001/chrome/browser/ui/v...
chrome/browser/ui/views/constrained_window_views.cc:582: gfx::Point position =
dialog_host->GetDialogPosition(size);
On 2013/08/22 17:46:10, Mike Wittman wrote:
> Have you tested on other web contents modal dialogs, and while resizing the
> browser window?

Yeah, works fine for all dialogs I tested on up to their respective
GetMinimumSize().

https://chromiumcodereview.appspot.com/22903022/diff/8001/components/web_moda...
File components/web_modal/web_contents_modal_dialog_host.h (right):

https://chromiumcodereview.appspot.com/22903022/diff/8001/components/web_moda...
components/web_modal/web_contents_modal_dialog_host.h:46: virtual gfx::Size
GetMaximumDialogSize() = 0;
On 2013/08/22 17:46:10, Mike Wittman wrote:
> Probably needs to be implemented for chromeos::WebUILoginView, and the other
> platform-specific NativeAppWindow subclasses as well.

Done.

https://chromiumcodereview.appspot.com/22903022/diff/8001/ui/views/bubble/bub...
File ui/views/bubble/bubble_frame_view.h (right):

https://chromiumcodereview.appspot.com/22903022/diff/8001/ui/views/bubble/bub...
ui/views/bubble/bubble_frame_view.h:92: gfx::Size GetNeededSize(const gfx::Size&
client_size);
On 2013/08/22 17:46:10, Mike Wittman wrote:
> GetSizeForClientSize maybe?

Done.

[Mike Wittman, 2013-08-23 17:14:29]

lgtm

[Rune Fevang, 2013-08-23 19:15:59]

ben: Could you do the OWNERS review please?

[Rune Fevang, 2013-08-23 19:40:44]

wittman just told me ben is out of office, so trying sky instead :)

[msw, 2013-08-23 19:57:25]

Just one driveby bubble nit.

https://chromiumcodereview.appspot.com/22903022/diff/20001/ui/views/bubble/bu...
File ui/views/bubble/bubble_frame_view.cc (right):

https://chromiumcodereview.appspot.com/22903022/diff/20001/ui/views/bubble/bu...
ui/views/bubble/bubble_frame_view.cc:178: gfx::Size
BubbleFrameView::GetSizeForClientSize(const gfx::Size& client_size) {
nit: Move this function to match the declaration order.

[Rune Fevang, 2013-08-23 20:17:44]

https://chromiumcodereview.appspot.com/22903022/diff/20001/ui/views/bubble/bu...
File ui/views/bubble/bubble_frame_view.cc (right):

https://chromiumcodereview.appspot.com/22903022/diff/20001/ui/views/bubble/bu...
ui/views/bubble/bubble_frame_view.cc:178: gfx::Size
BubbleFrameView::GetSizeForClientSize(const gfx::Size& client_size) {
On 2013/08/23 19:57:25, msw wrote:
> nit: Move this function to match the declaration order.

Done.

[sky, 2013-08-23 20:30:34]

How about some tests here?

[Rune Fevang, 2013-08-30 01:31:11]

On 2013/08/23 20:30:34, sky wrote:
> How about some tests here?

Added a unit test.

[sky, 2013-08-30 16:49:21]

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/constrained_window_views.cc (right):

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
chrome/browser/ui/views/constrained_window_views.cc:573: gfx::Size size =
widget->non_client_view()->GetPreferredSize();
Can you use RootView instead of non_client_view? That should mean you don't need
to look at the border.

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/constrained_window_views_unittest.cc (right):

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
chrome/browser/ui/views/constrained_window_views_unittest.cc:78:
scoped_ptr<Widget> parent(new Widget);
nit: you can declare parent on the stack since you have it as
WIDGET_OWNS_NATIVE_WIDGET.

[Mike Wittman, 2013-08-30 17:53:35]

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/constrained_window_views_unittest.cc (right):

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
chrome/browser/ui/views/constrained_window_views_unittest.cc:92:
UpdateWebContentsModalDialogPosition(dialog, dialog_host.get());
add an expectation on the dialog size here also

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
chrome/browser/ui/views/constrained_window_views_unittest.cc:121:
expected_size.Enlarge(0, border->GetInsets().top());
nit: assert border != NULL

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
chrome/browser/ui/views/constrained_window_views_unittest.cc:124: }
Can you add a final test with the maximum size increasing to be greater than the
preferred dialog size?

[Rune Fevang, 2013-08-30 21:08:52]

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/constrained_window_views.cc (right):

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
chrome/browser/ui/views/constrained_window_views.cc:573: gfx::Size size =
widget->non_client_view()->GetPreferredSize();
On 2013/08/30 16:49:21, sky wrote:
> Can you use RootView instead of non_client_view? That should mean you don't
need
> to look at the border.

I still need to look at the border (the root view and non client view are the
same size). Enlarging by the top border inset is done because this function
offsets the dialog from the position the host view specifies by that amount
further down in this function.

Changed it to use RootView as that seems more conceptually correct, as well as
added a comment explaining why I'm changing the maximum size.

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/constrained_window_views_unittest.cc (right):

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
chrome/browser/ui/views/constrained_window_views_unittest.cc:78:
scoped_ptr<Widget> parent(new Widget);
On 2013/08/30 16:49:21, sky wrote:
> nit: you can declare parent on the stack since you have it as
> WIDGET_OWNS_NATIVE_WIDGET.

Done.

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
chrome/browser/ui/views/constrained_window_views_unittest.cc:92:
UpdateWebContentsModalDialogPosition(dialog, dialog_host.get());
On 2013/08/30 17:53:35, Mike Wittman wrote:
> add an expectation on the dialog size here also

I don't know what to expect at this point, since it depends on the preferred
size of the non-content parts of the dialog. The reason I make sure that I set
the content are size to something larger than an empty dialog here is so I know
that any changes in the content size will directly influence the dialog size.

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
chrome/browser/ui/views/constrained_window_views_unittest.cc:121:
expected_size.Enlarge(0, border->GetInsets().top());
On 2013/08/30 17:53:35, Mike Wittman wrote:
> nit: assert border != NULL

Done.

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
chrome/browser/ui/views/constrained_window_views_unittest.cc:124: }
On 2013/08/30 17:53:35, Mike Wittman wrote:
> Can you add a final test with the maximum size increasing to be greater than
the
> preferred dialog size?

Done.

[Mike Wittman, 2013-08-30 22:00:26]

test lgtm

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/constrained_window_views_unittest.cc (right):

https://codereview.chromium.org/22903022/diff/66001/chrome/browser/ui/views/c...
chrome/browser/ui/views/constrained_window_views_unittest.cc:92:
UpdateWebContentsModalDialogPosition(dialog, dialog_host.get());
On 2013/08/30 21:08:52, Rune Fevang wrote:
> On 2013/08/30 17:53:35, Mike Wittman wrote:
> > add an expectation on the dialog size here also
> 
> I don't know what to expect at this point, since it depends on the preferred
> size of the non-content parts of the dialog. The reason I make sure that I set
> the content are size to something larger than an empty dialog here is so I
know
> that any changes in the content size will directly influence the dialog size.

Ah, makes sense.

[Rune Fevang, 2013-09-05 22:05:40]

Ping

[sky, 2013-09-05 23:39:14]

LGTM

https://codereview.chromium.org/22903022/diff/76001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/constrained_window_views_unittest.cc (right):

https://codereview.chromium.org/22903022/diff/76001/chrome/browser/ui/views/c...
chrome/browser/ui/views/constrained_window_views_unittest.cc:24:
DialogContents() : preferred_size_() {}
nit: no need to explicitly initialize preferred_size_.

[Rune Fevang, 2013-09-06 00:04:25]

Thanks :)

https://codereview.chromium.org/22903022/diff/76001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/constrained_window_views_unittest.cc (right):

https://codereview.chromium.org/22903022/diff/76001/chrome/browser/ui/views/c...
chrome/browser/ui/views/constrained_window_views_unittest.cc:24:
DialogContents() : preferred_size_() {}
On 2013/09/05 23:39:14, sky wrote:
> nit: no need to explicitly initialize preferred_size_.

Done.

[commit-bot: I haz the power, 2013-09-06 00:06:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rfevang@chromium.org/22903022/89001

[commit-bot: I haz the power, 2013-09-06 05:08:45]

Change committed as 221573