Check that we have a focus manager in aura before trying to set focus.

Check that we have a focus manager in aura before trying to set focus.
This allows PrintPreviewUITest to run under aura also.

BUG=104284
TEST=ran unit_tests and ui_tests with use_aura=1 chromeos=1


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

[sky, 2011-11-28 23:36:08]

http://codereview.chromium.org/8659014/diff/1/chrome/browser/ui/views/tab_con...
File chrome/browser/ui/views/tab_contents/native_tab_contents_container_aura.cc
(right):

http://codereview.chromium.org/8659014/diff/1/chrome/browser/ui/views/tab_con...
chrome/browser/ui/views/tab_contents/native_tab_contents_container_aura.cc:116:
if (GetFocusManager()) {
This seems fine, but I wonder why we don't have it on the windows side. I would
expect that if we're invoking RequestFocus when this container isn't in a widget
we would do the same on windows. Are we really invoking RequestFocus when not in
a widget?

[alicet1, 2011-12-05 20:21:54]

http://codereview.chromium.org/8659014/diff/4001/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/native_tab_contents_container_aura.cc
(right):

http://codereview.chromium.org/8659014/diff/4001/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/native_tab_contents_container_aura.cc:116:
//  if (GetFocusManager()) {
see the native_tab_contents_view_aura.cc in this CL: if I keep the widget type
to be TYPE_CONTROL, then this native tab content will not be top level and will
not have a focus manager, this line will be needed to check that we have a valud
focus manager.

[sky, 2011-12-05 21:56:41]

http://codereview.chromium.org/8659014/diff/4001/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/native_tab_contents_container_aura.cc
(right):

http://codereview.chromium.org/8659014/diff/4001/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/native_tab_contents_container_aura.cc:116:
//  if (GetFocusManager()) {
On 2011/12/05 20:21:55, alicet1 wrote:
> see the native_tab_contents_view_aura.cc in this CL: if I keep the widget type
> to be TYPE_CONTROL, then this native tab content will not be top level and
will
> not have a focus manager, this line will be needed to check that we have a
valud
> focus manager. 

See my second comment in this thread. A views::Widget of TYPE_CONTROL inherits
the FocusManager from an ancestor Widget as long as it's contained in a widget
hierarchy. For a browser window we create a BrowserFrame, which should be marked
as a top level. The only way this check would matter is if, for some reason,
we're invoking RequestFocus and the NativeTabContentsContainerAura does not have
an ancestor. It should be easy to figure that out. Make this a DCHECK and
inspect the ancestors.

[alicet1, 2011-12-06 06:28:50]

thanks scott. also added another unit test which failed in a similar manner.
will check it in after trybot runs.

thanx,
alice

http://codereview.chromium.org/8659014/diff/4001/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/native_tab_contents_container_aura.cc
(right):

http://codereview.chromium.org/8659014/diff/4001/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/native_tab_contents_container_aura.cc:116:
//  if (GetFocusManager()) {
On 2011/12/05 21:56:41, sky wrote:
> On 2011/12/05 20:21:55, alicet1 wrote:
> > see the native_tab_contents_view_aura.cc in this CL: if I keep the widget
type
> > to be TYPE_CONTROL, then this native tab content will not be top level and
> will
> > not have a focus manager, this line will be needed to check that we have a
> valud
> > focus manager. 
> 
> See my second comment in this thread. A views::Widget of TYPE_CONTROL inherits
> the FocusManager from an ancestor Widget as long as it's contained in a widget
> hierarchy. For a browser window we create a BrowserFrame, which should be
marked
> as a top level. The only way this check would matter is if, for some reason,
> we're invoking RequestFocus and the NativeTabContentsContainerAura does not
have
> an ancestor. It should be easy to figure that out. Make this a DCHECK and
> inspect the ancestors.

updated. per offline discussion, a test browser window in unittest may/may not
have all that we expect from a widget hierarchy, and in the absence of a top
level widget, it is safer to always check for a valid focus manager before
proceed.

[commit-bot: I haz the power, 2011-12-06 14:50:32]

No LGTM from valid reviewers yet.

[alicet1, 2011-12-06 14:52:28]

oops, forgot owners approval. scott can you take another look?

thanx,
alice

[sky, 2011-12-06 16:39:55]

LGTM

[commit-bot: I haz the power, 2011-12-06 17:02:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alicet@chromium.org/8659014/7001

[commit-bot: I haz the power, 2011-12-07 05:17:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alicet@chromium.org/8659014/7001

[commit-bot: I haz the power, 2011-12-07 06:44:53]

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

[Lei Zhang, 2011-12-07 07:02:13]

I call BS and will resubmit to the CQ.

[commit-bot: I haz the power, 2011-12-07 07:02:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alicet@chromium.org/8659014/7001

[commit-bot: I haz the power, 2011-12-07 08:11:47]

Change committed as 113362