Create constrained windows in a hidden state.

Create constrained windows in a hidden state.

BUG=97411
TEST=interactive_ui_tests --gtest_filter=ConstrainedWindowViewTest.FocusTest


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

[asanka, 2011-10-06 00:21:11]

On Windows we are creating native constrained windows with the WS_VISIBLE style
set.  This appears to be from r80386.

Since there can be multiple constrained windows queued up with only one being
shown per tab, the visibility state of the
ConstrainedWindowViews::non_client_view() is used to make sure that only the
visible constrained window is listening for accelerators and requests focus.

This patch is to re-instate initially hidden constrained views and  native
widgets.

The only instance where we set views::Widget::InitParams.child = true without
setting views::Widget::InitParams.type = TYPE_CONTROL is for constrained window.

I added a test to check for regressions in the future.

[asanka, 2011-10-21 18:56:49]

Ping.

[Ben Goodger (Google), 2011-10-26 18:04:58]

http://codereview.chromium.org/8052019/diff/18001/chrome/browser/ui/views/con...
File chrome/browser/ui/views/constrained_window_views.cc (right):

http://codereview.chromium.org/8052019/diff/18001/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_window_views.cc:609: // registration and
focus in a manner consistent with visibility.
Seems like the NCV should just defer registration until its widget is visible,
or that those accelerators should not be reachable for an invisible widget.

[asanka, 2011-10-26 18:46:24]

Thanks for looking!

http://codereview.chromium.org/8052019/diff/18001/chrome/browser/ui/views/con...
File chrome/browser/ui/views/constrained_window_views.cc (right):

http://codereview.chromium.org/8052019/diff/18001/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_window_views.cc:609: // registration and
focus in a manner consistent with visibility.
On 2011/10/26 18:04:58, Ben Goodger (Google) wrote:
> Seems like the NCV should just defer registration until its widget is visible,
> or that those accelerators should not be reachable for an invisible widget.

Yeah. I think that would be better. Views::RegisterPendingAccelerators() already
defers accelerator registration based on IsVisibleInRootView().  Should it
consider the visibility of the widget as well?

In the meantime I'll see if I can get NCV to defer registration.

[Ben Goodger (Google), 2011-10-26 19:07:33]

If there are negative side effects to registering for a hidden Widget, then yes,
Register on View should take this into account also, then be sure to register
when the Widget becomes visible.

[Ben Goodger (Google), 2011-10-26 19:07:57]

Alternatively, the FocusManager could make sure not to run accelerators for
targets inside hidden widgets. That may be simpler.

[asanka, 2011-10-29 21:27:39]

On 2011/10/26 19:07:33, Ben Goodger (Google) wrote:
> If there are negative side effects to registering for a hidden Widget, then
yes,
> Register on View should take this into account also, then be sure to register
> when the Widget becomes visible.

Logic for un-registering and re-registering accelerators when a widget's
visibility state changes was already there.  However,
View::RegisterPendingAccelerators() didn't check the visibility of the widget
when registering.

The FocusManager, unfortunately, can't take the visibility state of a widget
into account when running accelerators since AcceleratorTarget doesn't expose a
way to determine the corresponding widget.

I updated the patchset to:
1. Not touch the visibility state of non_client_view_ in ConstrainedWindowViews.
2. Check for the widget's visibility state in
View::RegisterPendingAccelerators().
3. Create child popups as hidden windows in NativeWidgetWin.

[Ben Goodger (Google), 2011-10-31 18:28:38]

Cool. LGTM.

http://codereview.chromium.org/8052019/diff/30001/chrome/browser/ui/views/con...
File chrome/browser/ui/views/constrained_window_views_browsertest.cc (right):

http://codereview.chromium.org/8052019/diff/30001/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_window_views_browsertest.cc:50: return
contents_->GetInitiallyFocusedView();
You can use a ternary operator for these types of things (here and elsewhere in
this class).

[asanka, 2011-11-01 14:21:52]

Thank you!

http://codereview.chromium.org/8052019/diff/30001/chrome/browser/ui/views/con...
File chrome/browser/ui/views/constrained_window_views_browsertest.cc (right):

http://codereview.chromium.org/8052019/diff/30001/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_window_views_browsertest.cc:50: return
contents_->GetInitiallyFocusedView();
On 2011/10/31 18:28:38, Ben Goodger (Google) wrote:
> You can use a ternary operator for these types of things (here and elsewhere
in
> this class).

Done.

[commit-bot: I haz the power, 2011-11-01 16:55:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/8052019/33001

[commit-bot: I haz the power, 2011-11-01 18:18:23]

Change committed as 108136