Move the concept of Activation to the Shell.

Move the concept of Activation to the Shell.

The Active Window is now stored in a property on the RootWindow. Classes wishing to observe changes to this can implement WindowObserver and attach to the RootWindow to be notified of changes in this property.

We provide an ActivationClient interface in Aura for customers to use to set/get the active window, and deactivate a window. This is because setting the active window involves more than just changing the property, there is some additional book-keeping that must be done. The ActivationClient is stored in a property on the RootWindow.

We also provide an ActivationDelegate interface in Aura that window owners can use to be notified of changes in activation state, and to specify whether or not a window can be activated. The ActivationDelegate should be stored on the relevant window in a property.

I moved a lot of Activation-related functionality out of Aura, including all of the unit tests, now on ActivationController, and the associated WindowDelegate implementations which have now become a single TestActivationDelegate implementation.

BUG=none
TEST=unit tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114095
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114121

[sky, 2011-12-09 23:11:22]

http://codereview.chromium.org/8894018/diff/5015/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/8894018/diff/5015/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_aura.cc:103:
window_->SetProperty(aura::kActivationDelegateKey,
It would be nice if you added a static setter for this on ActivateDelegate. That
way folks can't screw up the cast and don't need to know the implementation
detail of the delegate being stored as a property.

http://codereview.chromium.org/8894018/diff/5015/ui/aura/client/aura_constants.h
File ui/aura/client/aura_constants.h (right):

http://codereview.chromium.org/8894018/diff/5015/ui/aura/client/aura_constant...
ui/aura/client/aura_constants.h:54: AURA_EXPORT extern const char
kRootWindowActivationClient[];
Can you clarify ownership of this and ActivationDelegateKey, both here and in
the header for the classes (maybe about see blah for details). Looks like both
are not owned by the window, so that the destructor should be made protected.

http://codereview.chromium.org/8894018/diff/5015/ui/aura/client/aura_constant...
ui/aura/client/aura_constants.h:58: AURA_EXPORT extern const char
kActivationDelegateKey[];
nit: would be nice if we sorted all the keys in this file.

http://codereview.chromium.org/8894018/diff/5015/ui/aura_shell/activation_con...
File ui/aura_shell/activation_controller.cc (right):

http://codereview.chromium.org/8894018/diff/5015/ui/aura_shell/activation_con...
ui/aura_shell/activation_controller.cc:101: if (!CanActivateWindow(window))
If we end up here because of focus change, wouldn't this mean the focus is left
in an inactive window?

http://codereview.chromium.org/8894018/diff/5015/ui/aura_shell/activation_con...
ui/aura_shell/activation_controller.cc:155: window->AddObserver(this);
It doesn't seem like you ever remove this. Maybe OnWindowDestroyed should remove
the observer?

http://codereview.chromium.org/8894018/diff/5015/ui/views/widget/native_widge...
File ui/views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/8894018/diff/5015/ui/views/widget/native_widge...
ui/views/widget/native_widget_aura.cc:15: #include
"ui/aura/root_window_observer.h"
Can this be removed now?

http://codereview.chromium.org/8894018/diff/5015/ui/views/widget/native_widge...
File ui/views/widget/native_widget_aura.h (right):

http://codereview.chromium.org/8894018/diff/5015/ui/views/widget/native_widge...
ui/views/widget/native_widget_aura.h:14: #include "ui/aura/window_observer.h"
I think you can move this to to the .cc.

[Ben Goodger (Google), 2011-12-12 18:01:13]

All comments done except for question below:

http://codereview.chromium.org/8894018/diff/5015/ui/aura_shell/activation_con...
File ui/aura_shell/activation_controller.cc (right):

http://codereview.chromium.org/8894018/diff/5015/ui/aura_shell/activation_con...
ui/aura_shell/activation_controller.cc:101: if (!CanActivateWindow(window))
On 2011/12/09 23:11:23, sky wrote:
> If we end up here because of focus change, wouldn't this mean the focus is
left
> in an inactive window?

I am not quite sure what you mean by this. I've added a unit test that tests
activation changes triggered by focus changes to both top level windows and
child windows. See activation_controller_unittest.cc.

[sky, 2011-12-12 18:06:11]

LGTM since the issue I raised has likely always been an issue.

http://codereview.chromium.org/8894018/diff/5015/ui/aura_shell/activation_con...
File ui/aura_shell/activation_controller.cc (right):

http://codereview.chromium.org/8894018/diff/5015/ui/aura_shell/activation_con...
ui/aura_shell/activation_controller.cc:101: if (!CanActivateWindow(window))
On 2011/12/12 18:01:16, Ben Goodger (Google) wrote:
> On 2011/12/09 23:11:23, sky wrote:
> > If we end up here because of focus change, wouldn't this mean the focus is
> left
> > in an inactive window?
> 
> I am not quite sure what you mean by this. I've added a unit test that tests
> activation changes triggered by focus changes to both top level windows and
> child windows. See activation_controller_unittest.cc.

Say you had a window that didn't want to become active that has a child. If
child->Focus() is invoked, then I believe we'll set the focus to child, but
because CanActiveWindow(parent) returns false here, the focused window is not in
the active window. This may have always been an issue though. Not sure if we
need to disallow focus going to windows that can't be active.