Trying to activate a window in a workspace other than the current is ignored while a system modal d…

Trying to activate a window in a workspace other than the current is ignored while a system modal dialog is up

Added function "OnAttemptToReactivateWindow" to ActivationChangeObserver.
WorkspaceController will activate the requested window's workspace, if the 
window did not become active because of a system modal window.

BUG=196697
R=sky@chromium.org
TEST=
ash_unittests --gtest_filter='WorkspaceManagerTest.SwitchFromModal'
views_unittests --gtest_filter='FocusControllerApiTest.ReactivationEvents'

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

[sky, 2013-04-23 17:45:39]

https://codereview.chromium.org/14222019/diff/1/ui/views/corewm/focus_control...
File ui/views/corewm/focus_controller.cc (right):

https://codereview.chromium.org/14222019/diff/1/ui/views/corewm/focus_control...
ui/views/corewm/focus_controller.cc:288: if (updating_activation_ || window ==
active_window_)
Looking at this again could you instead add a branch here that invokes something
like OnAttemptToReactivateWindow() here? You would have to make this take the
requested window. I would also make OnAttemptToReactivateWindow() concrete (not
pure virtual) as most folks don't care about this).

[sschmitz, 2013-04-23 18:22:55]

Not sure I follow...
In this case we're not in an updating_focus scenario nor in an reactivation
scenario. It is not the case that  the requested window was activated and
then another window was activated instead. Is that that what you have in
mind with "ReactivateWindow"?

In our case, FocusWindow very early on determines the activatable window to
be different from the requested (because of the system modal window). That
happens in
  aura::Window* focusable = rules_->GetFocusableWindow(window);
  aura::Window* activatable =
      focusable ? rules_->GetActivatableWindow(focusable) : NULL;

Right here we could all something like:
OnWindowActivationOtherThanRequestedWindow? (instead of my earlier proposal
OnWindowActivationRequestCompleted).

Also SetActiveWindow is called from two places. If it took the requested
window as additional argument, that would be meaningless in the other place.

Am I missing something?



On Tue, Apr 23, 2013 at 10:45 AM, <sky@chromium.org> wrote:

>
> https://codereview.chromium.**org/14222019/diff/1/ui/views/**
>
corewm/focus_controller.cc<https://codereview.chromium.org/14222019/diff/1/ui/views/corewm/focus_controller.cc>
> File ui/views/corewm/focus_**controller.cc (right):
>
> https://codereview.chromium.**org/14222019/diff/1/ui/views/**
>
corewm/focus_controller.cc#**newcode288<https://codereview.chromium.org/14222019/diff/1/ui/views/corewm/focus_controller.cc#newcode288>
> ui/views/corewm/focus_**controller.cc:288: if (updating_activation_ ||
> window == active_window_)
> Looking at this again could you instead add a branch here that invokes
> something like OnAttemptToReactivateWindow() here? You would have to
> make this take the requested window. I would also make
> OnAttemptToReactivateWindow() concrete (not pure virtual) as most folks
> don't care about this).
>
>
https://codereview.chromium.**org/14222019/<https://codereview.chromium.org/1...
>

[sschmitz, 2013-04-23 18:41:31]

Now, I think I get it.

Do you propose to restrict (this new logic) to only cases where the
*already active* window has been selected instead of the requested one.

This would be narrower than any time the selected (for activation) window
is different from the requested?



On Tue, Apr 23, 2013 at 11:22 AM, Stefan Schmitz <sschmitz@chromium.org>wrote:

> Not sure I follow...
> In this case we're not in an updating_focus scenario nor in an
> reactivation scenario. It is not the case that  the requested window was
> activated and then another window was activated instead. Is that that what
> you have in mind with "ReactivateWindow"?
>
> In our case, FocusWindow very early on determines the activatable window
> to be different from the requested (because of the system modal window).
> That happens in
>   aura::Window* focusable = rules_->GetFocusableWindow(window);
>   aura::Window* activatable =
>       focusable ? rules_->GetActivatableWindow(focusable) : NULL;
>
> Right here we could all something like:
> OnWindowActivationOtherThanRequestedWindow? (instead of my earlier proposal
> OnWindowActivationRequestCompleted).
>
> Also SetActiveWindow is called from two places. If it took the requested
> window as additional argument, that would be meaningless in the other place.
>
> Am I missing something?
>
>
>
> On Tue, Apr 23, 2013 at 10:45 AM, <sky@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/14222019/diff/1/ui/views/**
>>
corewm/focus_controller.cc<https://codereview.chromium.org/14222019/diff/1/ui/views/corewm/focus_controller.cc>
>> File ui/views/corewm/focus_**controller.cc (right):
>>
>> https://codereview.chromium.**org/14222019/diff/1/ui/views/**
>>
corewm/focus_controller.cc#**newcode288<https://codereview.chromium.org/14222019/diff/1/ui/views/corewm/focus_controller.cc#newcode288>
>> ui/views/corewm/focus_**controller.cc:288: if (updating_activation_ ||
>> window == active_window_)
>> Looking at this again could you instead add a branch here that invokes
>> something like OnAttemptToReactivateWindow() here? You would have to
>> make this take the requested window. I would also make
>> OnAttemptToReactivateWindow() concrete (not pure virtual) as most folks
>> don't care about this).
>>
>>
https://codereview.chromium.**org/14222019/<https://codereview.chromium.org/1...
>>
>
>

[sschmitz, 2013-04-23 19:04:38]

Done.

https://codereview.chromium.org/14222019/diff/1/ui/views/corewm/focus_control...
File ui/views/corewm/focus_controller.cc (right):

https://codereview.chromium.org/14222019/diff/1/ui/views/corewm/focus_control...
ui/views/corewm/focus_controller.cc:288: if (updating_activation_ || window ==
active_window_)
On 2013/04/23 17:45:39, sky wrote:
> Looking at this again could you instead add a branch here that invokes
something
> like OnAttemptToReactivateWindow() here? You would have to make this take the
> requested window. I would also make OnAttemptToReactivateWindow() concrete
(not
> pure virtual) as most folks don't care about this).

Done.

[sky, 2013-04-23 21:36:42]

Also add test coverage in FocusControllerUnittest. By that I mean make sure your
new method is invoked when requesting the active window to change and that it is
not invoked when changing to a new window.

https://codereview.chromium.org/14222019/diff/8001/ash/wm/workspace_controlle...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/14222019/diff/8001/ash/wm/workspace_controlle...
ash/wm/workspace_controller.cc:63: aura::Window* request_active, aura::Window*
actual_active) {
same comment here.

https://codereview.chromium.org/14222019/diff/8001/ash/wm/workspace_controlle...
ash/wm/workspace_controller.cc:64: if (!request_active || request_active ==
actual_active)
Do we really pass in NULL in some case?

https://codereview.chromium.org/14222019/diff/8001/ash/wm/workspace_controlle...
ash/wm/workspace_controller.cc:67: // Handle the case that the requested window
was not activated because
How about something like:
While a system model dialog is showing requests to activate
a window other than that of the modal dialog fail. We only switch the active
workspace when activation changes. The result is the active workspace never
changes while a system modal dialog is showing. This code ensures the active
workspace changes even though activation doesn't change away from the system
model dialog.

https://codereview.chromium.org/14222019/diff/8001/ash/wm/workspace_controller.h
File ash/wm/workspace_controller.h (right):

https://codereview.chromium.org/14222019/diff/8001/ash/wm/workspace_controlle...
ash/wm/workspace_controller.h:55: aura::Window* request_active, aura::Window*
actual_active) OVERRIDE;
When you wrap, each param on its own line.

https://codereview.chromium.org/14222019/diff/8001/ui/aura/client/activation_...
File ui/aura/client/activation_change_observer.h (right):

https://codereview.chromium.org/14222019/diff/8001/ui/aura/client/activation_...
ui/aura/client/activation_change_observer.h:25: // is selected for activation.
This can happen when the a window
'when the a' -> 'when a'

https://codereview.chromium.org/14222019/diff/8001/ui/views/corewm/focus_cont...
File ui/views/corewm/focus_controller.cc (right):

https://codereview.chromium.org/14222019/diff/8001/ui/views/corewm/focus_cont...
ui/views/corewm/focus_controller.cc:288:
FOR_EACH_OBSERVER(aura::client::ActivationChangeObserver,
I think it's only worth invoking this when requested_window is non-null.

https://codereview.chromium.org/14222019/diff/8001/ui/views/corewm/focus_cont...
File ui/views/corewm/focus_controller.h (right):

https://codereview.chromium.org/14222019/diff/8001/ui/views/corewm/focus_cont...
ui/views/corewm/focus_controller.h:91: void SetActiveWindow(aura::Window*
requested_window, aura::Window* window);
Document what requested_window is.

[sschmitz, 2013-04-24 01:23:42]

done

https://codereview.chromium.org/14222019/diff/8001/ash/wm/workspace_controlle...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/14222019/diff/8001/ash/wm/workspace_controlle...
ash/wm/workspace_controller.cc:63: aura::Window* request_active, aura::Window*
actual_active) {
On 2013/04/23 21:36:42, sky wrote:
> same comment here.

Done.

https://codereview.chromium.org/14222019/diff/8001/ash/wm/workspace_controlle...
ash/wm/workspace_controller.cc:64: if (!request_active || request_active ==
actual_active)
On 2013/04/23 21:36:42, sky wrote:
> Do we really pass in NULL in some case?
Not anymore.
Done.

https://codereview.chromium.org/14222019/diff/8001/ash/wm/workspace_controlle...
ash/wm/workspace_controller.cc:67: // Handle the case that the requested window
was not activated because
On 2013/04/23 21:36:42, sky wrote:
> How about something like:
> While a system model dialog is showing requests to activate
> a window other than that of the modal dialog fail. We only switch the active
> workspace when activation changes. The result is the active workspace never
> changes while a system modal dialog is showing. This code ensures the active
> workspace changes even though activation doesn't change away from the system
> model dialog.

Done.

https://codereview.chromium.org/14222019/diff/8001/ash/wm/workspace_controller.h
File ash/wm/workspace_controller.h (right):

https://codereview.chromium.org/14222019/diff/8001/ash/wm/workspace_controlle...
ash/wm/workspace_controller.h:55: aura::Window* request_active, aura::Window*
actual_active) OVERRIDE;
On 2013/04/23 21:36:42, sky wrote:
> When you wrap, each param on its own line.

Done.

https://codereview.chromium.org/14222019/diff/8001/ui/aura/client/activation_...
File ui/aura/client/activation_change_observer.h (right):

https://codereview.chromium.org/14222019/diff/8001/ui/aura/client/activation_...
ui/aura/client/activation_change_observer.h:25: // is selected for activation.
This can happen when the a window
On 2013/04/23 21:36:42, sky wrote:
> 'when the a' -> 'when a'

Done.

https://codereview.chromium.org/14222019/diff/8001/ui/views/corewm/focus_cont...
File ui/views/corewm/focus_controller.cc (right):

https://codereview.chromium.org/14222019/diff/8001/ui/views/corewm/focus_cont...
ui/views/corewm/focus_controller.cc:288:
FOR_EACH_OBSERVER(aura::client::ActivationChangeObserver,
On 2013/04/23 21:36:42, sky wrote:
> I think it's only worth invoking this when requested_window is non-null.

Done.

https://codereview.chromium.org/14222019/diff/8001/ui/views/corewm/focus_cont...
File ui/views/corewm/focus_controller.h (right):

https://codereview.chromium.org/14222019/diff/8001/ui/views/corewm/focus_cont...
ui/views/corewm/focus_controller.h:91: void SetActiveWindow(aura::Window*
requested_window, aura::Window* window);
On 2013/04/23 21:36:42, sky wrote:
> Document what requested_window is.

Done.

[sky, 2013-04-24 14:14:52]

Just nits, LGTM

https://codereview.chromium.org/14222019/diff/16001/ash/wm/workspace_controll...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/14222019/diff/16001/ash/wm/workspace_controll...
ash/wm/workspace_controller.cc:65: if (request_active == actual_active)
Why do we need this check? It seems like the only thing we care about is line
67.

https://codereview.chromium.org/14222019/diff/16001/ui/views/corewm/focus_con...
File ui/views/corewm/focus_controller.h (right):

https://codereview.chromium.org/14222019/diff/16001/ui/views/corewm/focus_con...
ui/views/corewm/focus_controller.h:87: // Internal implementations that set the
focused/active windows, fire events
Now that you've added a comment for 87 it no longer makes sense to have this
comment span the two. Instead make this comment just for focused and expand the
comment for SetActiveWindow to describe this too. Additionlly newline between
90/91.

https://codereview.chromium.org/14222019/diff/16001/ui/views/corewm/focus_con...
File ui/views/corewm/focus_controller_unittest.cc (right):

https://codereview.chromium.org/14222019/diff/16001/ui/views/corewm/focus_con...
ui/views/corewm/focus_controller_unittest.cc:42: int GetReactivationCount() {
methods like this are typically named in unix_hacker_style. So, this should be
reactivation_count() (and const). Same with 45/48.

[sschmitz, 2013-04-24 15:42:03]

all done.

https://codereview.chromium.org/14222019/diff/16001/ash/wm/workspace_controll...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/14222019/diff/16001/ash/wm/workspace_controll...
ash/wm/workspace_controller.cc:65: if (request_active == actual_active)
On 2013/04/24 14:14:53, sky wrote:
> Why do we need this check? It seems like the only thing we care about is line
> 67.

Done.

https://codereview.chromium.org/14222019/diff/16001/ui/views/corewm/focus_con...
File ui/views/corewm/focus_controller.h (right):

https://codereview.chromium.org/14222019/diff/16001/ui/views/corewm/focus_con...
ui/views/corewm/focus_controller.h:87: // Internal implementations that set the
focused/active windows, fire events
On 2013/04/24 14:14:53, sky wrote:
> Now that you've added a comment for 87 it no longer makes sense to have this
> comment span the two. Instead make this comment just for focused and expand
the
> comment for SetActiveWindow to describe this too. Additionlly newline between
> 90/91.

Done.

https://codereview.chromium.org/14222019/diff/16001/ui/views/corewm/focus_con...
File ui/views/corewm/focus_controller_unittest.cc (right):

https://codereview.chromium.org/14222019/diff/16001/ui/views/corewm/focus_con...
ui/views/corewm/focus_controller_unittest.cc:42: int GetReactivationCount() {
On 2013/04/24 14:14:53, sky wrote:
> methods like this are typically named in unix_hacker_style. So, this should be
> reactivation_count() (and const). Same with 45/48.

Done.

[commit-bot: I haz the power, 2013-04-24 15:57:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/14222019/25001

[commit-bot: I haz the power, 2013-04-24 18:49:55]

Retried try job too often on win7_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-04-24 20:41:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/14222019/25001

[commit-bot: I haz the power, 2013-04-24 21:56:34]

Change committed as 196243