Activate the status area only when it's focused using the keyboard

Activate the status area only when it's focused using the keyboard.

ash/status_area/status_area_view.cc:
chrome/browser/chromeos/status/status_area_view.cc:

Override views::WidgetDelegate::CanActivate. Return false unless the area is focused using the keyboard.

ash/system/tray/system_tray.cc:

Handle a mouse event correctly.

ash/wm/activation_controller.cc:

Change the order of kWindowContainerIds[] so that a browser window gets focused when a window in SettingBubbleContainer is closed.

This fix is similar to http://crrev.com/122459 (by sky).

BUG=115815
TEST=aura_shell_unittests

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

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=125368

[sadrul, 2012-03-02 10:17:01]

http://codereview.chromium.org/9570013/diff/4002/ash/wm/activation_controller.cc
File ash/wm/activation_controller.cc (right):

http://codereview.chromium.org/9570013/diff/4002/ash/wm/activation_controller...
ash/wm/activation_controller.cc:211: window && window->type() ==
aura::client::WINDOW_TYPE_MENU)) {
I wonder if you can just use 

if (window && window->type() != aura::client::WINDOW_TYPE_NORMAL)

(and maybe exclude TOOLTIP too)

If so, then you won't need the changes in system_tray

[Yusuke Sato, 2012-03-02 11:51:08]

http://codereview.chromium.org/9570013/diff/4002/ash/wm/activation_controller.cc
File ash/wm/activation_controller.cc (right):

http://codereview.chromium.org/9570013/diff/4002/ash/wm/activation_controller...
ash/wm/activation_controller.cc:211: window && window->type() ==
aura::client::WINDOW_TYPE_MENU)) {
To handle the uber tray here, I tried this:

window && window->type() == aura::client::WINDOW_TYPE_MENU ||
window && window->type() == aura::client::WINDOW_TYPE_POPUP

but it didn't work because when the uber tray (volume) icon in the status area
is clicked to hide the tray bubble, the status area itself is immediately
activated before this function is called via
OnWindowVisibilityChanged(the_bubble, false). So I guess we have to modify the
system tray code anyway.



On 2012/03/02 10:17:01, sadrul wrote:
> I wonder if you can just use 
> 
> if (window && window->type() != aura::client::WINDOW_TYPE_NORMAL)
> 
> (and maybe exclude TOOLTIP too)
> 
> If so, then you won't need the changes in system_tray

[Daniel Erat, 2012-03-02 16:49:19]

cc oshima and alicet in case they have opinions about this

http://codereview.chromium.org/9570013/diff/4002/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/9570013/diff/4002/ash/system/tray/system_tray....
ash/system/tray/system_tray.cc:71:
ash::Shell::GetInstance()->delegate()->GetCycleWindowList(
it seems a bit strange to have this class reach in and do this itself instead of
just asking the shell or some other class within the shell to focus a browser.

[oshima, 2012-03-02 17:38:58]

On 2012/03/02 16:49:19, Daniel Erat wrote:
> cc oshima and alicet in case they have opinions about this

I still wonder why this behavior is different from app list. 
If you click and open app list, browser don't lose focus.

> 
>
http://codereview.chromium.org/9570013/diff/4002/ash/system/tray/system_tray.cc
> File ash/system/tray/system_tray.cc (right):
> 
>
http://codereview.chromium.org/9570013/diff/4002/ash/system/tray/system_tray....
> ash/system/tray/system_tray.cc:71:
> ash::Shell::GetInstance()->delegate()->GetCycleWindowList(
> it seems a bit strange to have this class reach in and do this itself instead
of
> just asking the shell or some other class within the shell to focus a browser.

[Yusuke Sato, 2012-03-05 12:56:14]

On 2012/03/02 17:38:58, oshima wrote:
> On 2012/03/02 16:49:19, Daniel Erat wrote:
> > cc oshima and alicet in case they have opinions about this
> 
> I still wonder why this behavior is different from app list. 
> If you click and open app list, browser don't lose focus.

I found that the reason why the launcher area did not steal focus was this CL:
https://chromiumcodereview.appspot.com/9406031/ (by sky). In short, the launcher
area overrides views::WidgetDelegate::CanActivate()  and the function returns
false in most cases. I added similar code to the status area.

Please take a look at patch set #3.

[Yusuke Sato, 2012-03-05 12:56:24]

http://codereview.chromium.org/9570013/diff/4002/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/9570013/diff/4002/ash/system/tray/system_tray....
ash/system/tray/system_tray.cc:71:
ash::Shell::GetInstance()->delegate()->GetCycleWindowList(
Removed the code. Modified kWindowContainerIds[] in activation_controller.cc
instead.

On 2012/03/02 16:49:19, Daniel Erat wrote:
> it seems a bit strange to have this class reach in and do this itself instead
of
> just asking the shell or some other class within the shell to focus a browser.

[Daniel Erat, 2012-03-05 16:49:53]

lgtm

[Yusuke Sato, 2012-03-06 05:41:21]

+sky, stevenjb
Add chrome/browser/ui/ and chrome/browser/chromeos/status/ owners.

[sky, 2012-03-06 16:49:40]

LGTM

http://codereview.chromium.org/9570013/diff/24001/ash/status_area/status_area...
File ash/status_area/status_area_view.h (right):

http://codereview.chromium.org/9570013/diff/24001/ash/status_area/status_area...
ash/status_area/status_area_view.h:32: // Sets the focus cycler.
Move this above overriden methods.

[stevenjb, 2012-03-06 22:23:59]

owner lgtm for status_area

[Yusuke Sato, 2012-03-07 01:28:46]

http://codereview.chromium.org/9570013/diff/24001/ash/status_area/status_area...
File ash/status_area/status_area_view.h (right):

http://codereview.chromium.org/9570013/diff/24001/ash/status_area/status_area...
ash/status_area/status_area_view.h:32: // Sets the focus cycler.
On 2012/03/06 16:49:40, sky wrote:
> Move this above overriden methods.

Done.

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukes@chromium.org/9570013/28001

[commit-bot: I haz the power, 2012-03-07 04:41:31]

Change committed as 125323

[commit-bot: I haz the power, 2012-03-07 08:26:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukes@chromium.org/9570013/25012