Enable KeyboardOverlayDialog on aura.

Enable KeyboardOverlayDialog on aura.


BUG=99858
TEST=KeyboardOverlayDialog should show up on aura build.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=119992

[mazda, 2012-01-19 05:03:46]

It looks window frame is shown around the keyboard. Is this removed in the
future?
I also would like you to check browser shortcuts like Ctrl+t, Ctrl+w work with
the keyboard overlay displayed after you fixed keyboard event issue
(http://crbug.com/110606).

[Ben Goodger (Google), 2012-01-19 18:13:34]

We would need some mapping to translate STYLE_FLUSH to
aura::client::TYPE_WINDOW_FRAMELESS.

[jennyz, 2012-01-27 23:17:48]

I made some comments in line for some alternative code options. PTAL.

http://codereview.chromium.org/9250029/diff/9001/chrome/browser/ui/views/wind...
File chrome/browser/ui/views/window.h (right):

http://codereview.chromium.org/9250029/diff/9001/chrome/browser/ui/views/wind...
chrome/browser/ui/views/window.h:27: views::WidgetDelegate* delegate);
I am not sure if KeyboardOverlayDialog is the only dialog that should be
borderless on aura, therefore, I create a separate API
browser::CreateFramelessViewsWindow(). If we should make all dialogs with
STYLE_FLUSH to be borderless, then I can merge the code into
browser::CreateViewsWindow().

http://codereview.chromium.org/9250029/diff/9001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

http://codereview.chromium.org/9250029/diff/9001/ui/views/widget/widget.cc#ne...
ui/views/widget/widget.cc:238:
widget->SetInitialBoundsForFramelessWindow(bounds);
I was thinking for moving SetContentsView(delegate->GetContentsView()) and
SetInitialBoundsForFramelessWindow into Widget::Init() for else part of if
(RequiresNonClientView(params.type)), but I found some existing codes are
calling widget->SetContentsView(xx) after widget->Init(), although most time
delegate and the parameter passing to SetContentsView are the same, but I am a
little concerned if we can assume that all the time, therefore, I left this
outside Widget::Init(), the code maybe cleaner the other way. Any suggestion?

[jennyz, 2012-01-27 23:18:59]

For Mazda's question for ctrl-t and ctrl-w. I tried, when the dialog is open,
ctrl-t will open a new tab and close the dialog. Ctrl-w will close a tab and
close the dialog. Is this behavior expected?

[Ben Goodger (Google), 2012-01-30 21:16:16]

http://codereview.chromium.org/9250029/diff/9001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

http://codereview.chromium.org/9250029/diff/9001/ui/views/widget/widget.cc#ne...
ui/views/widget/widget.cc:227: const gfx::Rect& bounds) {
Rather than do this in Widget, can you just move this code into the window.cc
file in src/chrome/browser... since you only use this in one place.

[mazda, 2012-01-30 22:10:42]

On 2012/01/27 23:18:59, jennyz wrote:
> For Mazda's question for ctrl-t and ctrl-w. I tried, when the dialog is open,
> ctrl-t will open a new tab and close the dialog. Ctrl-w will close a tab and
> close the dialog. Is this behavior expected?

Yes, those are expected behaviors (http://crosbug.com/13016). Browser shortcuts
should work with the keyboard overlay displayed.

[jennyz, 2012-01-30 23:13:28]

http://codereview.chromium.org/9250029/diff/9001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

http://codereview.chromium.org/9250029/diff/9001/ui/views/widget/widget.cc#ne...
ui/views/widget/widget.cc:227: const gfx::Rect& bounds) {
On 2012/01/30 21:16:16, Ben Goodger (Google) wrote:
> Rather than do this in Widget, can you just move this code into the window.cc
> file in src/chrome/browser... since you only use this in one place.

Done.

[mazda, 2012-01-30 23:26:29]

lgtm with a nit

http://codereview.chromium.org/9250029/diff/14003/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

http://codereview.chromium.org/9250029/diff/14003/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:1217: DCHECK(contents_view != NULL);
DCHECK(contents_view);

[Ben Goodger (Google), 2012-01-31 00:14:19]

lgtm

http://codereview.chromium.org/9250029/diff/14003/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

http://codereview.chromium.org/9250029/diff/14003/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:339: } else {
} else if (params.delegate) {

[jennyz, 2012-01-31 00:24:03]

http://codereview.chromium.org/9250029/diff/14003/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

http://codereview.chromium.org/9250029/diff/14003/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:339: } else {
On 2012/01/31 00:14:19, Ben Goodger (Google) wrote:
> } else if (params.delegate) {

Done.

http://codereview.chromium.org/9250029/diff/14003/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:1217: DCHECK(contents_view != NULL);
On 2012/01/30 23:26:29, mazda wrote:
> DCHECK(contents_view);

Done.

[commit-bot: I haz the power, 2012-01-31 00:24:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/9250029/2017

[commit-bot: I haz the power, 2012-01-31 03:07:49]

Try job failure for 9250029-2017 (retry) on win_rel for step "views_unittests".
It's a second try, previously, steps "unit_tests, views_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[jennyz, 2012-01-31 19:55:06]

Check the size returned by GetPreferredSize() in
Widget::SetInitialBoundsForFramelessWindow(). This fixes the DCHECK failure in
BubbleDelegateTest.CreateDelegate() of views_unittests for windows(non-aura).

[jennyz, 2012-01-31 20:10:58]

By the way, the DCHECK fails is from BubbleDelegateTest.CreateDelegate() at:
hwnd_util.cc(110)] Check failed: window && pref.width() > 0 && pref.height() >
0. 
For non-aura windows, the type of widget is TYPE_WINDOW_FRAMELESS,
Widget::Init() calls SetInitialBoundsForFramelessWindow(), but
GetPreferredSize() return zero size.

[mazda, 2012-01-31 21:35:22]

lgtm

[commit-bot: I haz the power, 2012-01-31 21:59:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/9250029/24002

[commit-bot: I haz the power, 2012-02-01 00:07:57]

Change committed as 119992