Focus launcher if spoken feedback is enabled and no other windows visible.

Focus launcher if spoken feedback is enabled and no other windows visible.

This patch automatically focuses launcher if no other windows are available and spoken feedback is enabled.

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

[mtomasz, 2012-12-05 01:57:46]

This patch automatically focuses launcher if no other windows are available and
spoken feedback is enabled.

BUG=156772

[sky, 2012-12-05 05:01:08]

I'm fine with this approach. You'll need to add test coverage too.

https://codereview.chromium.org/11451002/diff/1002/ash/wm/ash_activation_cont...
File ash/wm/ash_activation_controller.cc (right):

https://codereview.chromium.org/11451002/diff/1002/ash/wm/ash_activation_cont...
ash/wm/ash_activation_controller.cc:43: Launcher* launcher;
Can we do all this if window is null and move to a separate function. I'm
thinking:

if (!window)
  window = PrepareToActivateLauncher();

Where PrepareToActivateLauncher() is all the code you have here. Also, I thought
you only wanted to do this if spoken feedback is enabled.

[mtomasz, 2012-12-07 06:18:47]

As for tests, I don't think I can test it without a browser process since this
solution is quite tricky and depends on lots of classes.

https://codereview.chromium.org/11451002/diff/1002/ash/wm/ash_activation_cont...
File ash/wm/ash_activation_controller.cc (right):

https://codereview.chromium.org/11451002/diff/1002/ash/wm/ash_activation_cont...
ash/wm/ash_activation_controller.cc:43: Launcher* launcher;
On 2012/12/05 05:01:09, sky wrote:
> Can we do all this if window is null and move to a separate function. I'm
> thinking:
> 
> if (!window)
>   window = PrepareToActivateLauncher();
> 
> Where PrepareToActivateLauncher() is all the code you have here. Also, I
thought
> you only wanted to do this if spoken feedback is enabled.

Done.

[sky, 2012-12-07 16:49:42]

Why is it necessary to have a browser to test this? Isn't the key
thing to verify when going from one window to no window the launcher
get focused? And similarly when going from none to one window the
right thing is focused?

  -Scott

On Thu, Dec 6, 2012 at 10:18 PM,  <mtomasz@chromium.org> wrote:
> As for tests, I don't think I can test it without a browser process since
> this
> solution is quite tricky and depends on lots of classes.
>
>
>
>
https://codereview.chromium.org/11451002/diff/1002/ash/wm/ash_activation_cont...
> File ash/wm/ash_activation_controller.cc (right):
>
>
https://codereview.chromium.org/11451002/diff/1002/ash/wm/ash_activation_cont...
> ash/wm/ash_activation_controller.cc:43: Launcher* launcher;
> On 2012/12/05 05:01:09, sky wrote:
>>
>> Can we do all this if window is null and move to a separate function.
>
> I'm
>>
>> thinking:
>
>
>> if (!window)
>>    window = PrepareToActivateLauncher();
>
>
>> Where PrepareToActivateLauncher() is all the code you have here. Also,
>
> I thought
>>
>> you only wanted to do this if spoken feedback is enabled.
>
>
> Done.
>
> https://codereview.chromium.org/11451002/

[mtomasz, 2012-12-10 04:10:24]

You're right. I didn't know that we can easily run whole Shell instance with all
the controllers. I've added unit tests.

On 2012/12/07 16:49:42, sky wrote:
> Why is it necessary to have a browser to test this? Isn't the key
> thing to verify when going from one window to no window the launcher
> get focused? And similarly when going from none to one window the
> right thing is focused?
> 
>   -Scott
> 
> On Thu, Dec 6, 2012 at 10:18 PM,  <mailto:mtomasz@chromium.org> wrote:
> > As for tests, I don't think I can test it without a browser process since
> > this
> > solution is quite tricky and depends on lots of classes.
> >
> >
> >
> >
>
https://codereview.chromium.org/11451002/diff/1002/ash/wm/ash_activation_cont...
> > File ash/wm/ash_activation_controller.cc (right):
> >
> >
>
https://codereview.chromium.org/11451002/diff/1002/ash/wm/ash_activation_cont...
> > ash/wm/ash_activation_controller.cc:43: Launcher* launcher;
> > On 2012/12/05 05:01:09, sky wrote:
> >>
> >> Can we do all this if window is null and move to a separate function.
> >
> > I'm
> >>
> >> thinking:
> >
> >
> >> if (!window)
> >>    window = PrepareToActivateLauncher();
> >
> >
> >> Where PrepareToActivateLauncher() is all the code you have here. Also,
> >
> > I thought
> >>
> >> you only wanted to do this if spoken feedback is enabled.
> >
> >
> > Done.
> >
> > https://codereview.chromium.org/11451002/

[sky, 2012-12-10 15:37:51]

https://codereview.chromium.org/11451002/diff/13001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/11451002/diff/13001/ash/ash.gyp#newcode568
ash/ash.gyp:568: 'wm/ash_activation_controller.cc',
ash_activation_controller.* should be in the ash sources section above.

https://codereview.chromium.org/11451002/diff/13001/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

https://codereview.chromium.org/11451002/diff/13001/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:391: void
Launcher::OnWidgetActivationChanged(views::Widget* widget, bool active) {
This should be beneath GetLauncherViewForTest.

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
File ash/wm/ash_activation_controller_unittest.cc (right):

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
ash/wm/ash_activation_controller_unittest.cc:28: launcher_window_ =
launcher_window_ = launcher_widget_->GetNativeWindow()

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
ash/wm/ash_activation_controller_unittest.cc:30: DCHECK(launcher_ != NULL);
DCHECK->ASSERT_TRUE Also, this should be between 26/27 otheriwe 27 will have
crashed.

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
ash/wm/ash_activation_controller_unittest.cc:44: ash::Launcher* launcher_;
Member initialize these to NULL.

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
ash/wm/ash_activation_controller_unittest.cc:55: aura::Window* result =
ash_activation_controller_->WillActivateWindow(NULL);
Rather than explicitly calling through like this can't you test activation/focus
directly?

[mtomasz, 2012-12-11 05:03:32]

https://codereview.chromium.org/11451002/diff/13001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/11451002/diff/13001/ash/ash.gyp#newcode568
ash/ash.gyp:568: 'wm/ash_activation_controller.cc',
On 2012/12/10 15:37:51, sky wrote:
> ash_activation_controller.* should be in the ash sources section above.

I think I am doing sth wrong. It is already there, but if I remove it from here
then I get linker undefined reference error. Any hint?

https://codereview.chromium.org/11451002/diff/13001/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

https://codereview.chromium.org/11451002/diff/13001/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:391: void
Launcher::OnWidgetActivationChanged(views::Widget* widget, bool active) {
On 2012/12/10 15:37:51, sky wrote:
> This should be beneath GetLauncherViewForTest.

Done.

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
File ash/wm/ash_activation_controller_unittest.cc (right):

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
ash/wm/ash_activation_controller_unittest.cc:28: launcher_window_ =
On 2012/12/10 15:37:51, sky wrote:
> launcher_window_ = launcher_widget_->GetNativeWindow()

Done.

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
ash/wm/ash_activation_controller_unittest.cc:30: DCHECK(launcher_ != NULL);
On 2012/12/10 15:37:51, sky wrote:
> DCHECK->ASSERT_TRUE Also, this should be between 26/27 otheriwe 27 will have
> crashed.

Done.

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
ash/wm/ash_activation_controller_unittest.cc:44: ash::Launcher* launcher_;
On 2012/12/10 15:37:51, sky wrote:
> Member initialize these to NULL.

Done.

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
ash/wm/ash_activation_controller_unittest.cc:55: aura::Window* result =
ash_activation_controller_->WillActivateWindow(NULL);
On 2012/12/10 15:37:51, sky wrote:
> Rather than explicitly calling through like this can't you test
activation/focus
> directly?

I could, but would it be better? We would loose isolation without gaining
anything since ActivationController is already tested. This unit test checks
correctness of AshActivationController class and nothing more while avoiding
calling external dependencies whenever possible.

[sky, 2012-12-11 16:30:47]

https://codereview.chromium.org/11451002/diff/13001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/11451002/diff/13001/ash/ash.gyp#newcode568
ash/ash.gyp:568: 'wm/ash_activation_controller.cc',
On 2012/12/11 05:03:32, mtomasz wrote:
> On 2012/12/10 15:37:51, sky wrote:
> > ash_activation_controller.* should be in the ash sources section above.
> 
> I think I am doing sth wrong. It is already there, but if I remove it from
here
> then I get linker undefined reference error. Any hint?

Maybe add ASH_EXPORT?

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
File ash/wm/ash_activation_controller_unittest.cc (right):

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
ash/wm/ash_activation_controller_unittest.cc:55: aura::Window* result =
ash_activation_controller_->WillActivateWindow(NULL);
On 2012/12/11 05:03:32, mtomasz wrote:
> On 2012/12/10 15:37:51, sky wrote:
> > Rather than explicitly calling through like this can't you test
> activation/focus
> > directly?
> 
> I could, but would it be better? We would loose isolation without gaining
> anything since ActivationController is already tested. This unit test checks
> correctness of AshActivationController class and nothing more while avoiding
> calling external dependencies whenever possible.

I'm for testing actual window activation since in the end that is what you care
about, especially since you can do it via a unit test still.

[mtomasz, 2012-12-13 07:55:29]

https://codereview.chromium.org/11451002/diff/13001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/11451002/diff/13001/ash/ash.gyp#newcode568
ash/ash.gyp:568: 'wm/ash_activation_controller.cc',
On 2012/12/11 16:30:47, sky wrote:
> On 2012/12/11 05:03:32, mtomasz wrote:
> > On 2012/12/10 15:37:51, sky wrote:
> > > ash_activation_controller.* should be in the ash sources section above.
> > 
> > I think I am doing sth wrong. It is already there, but if I remove it from
> here
> > then I get linker undefined reference error. Any hint?
> 
> Maybe add ASH_EXPORT?

Done.

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
File ash/wm/ash_activation_controller_unittest.cc (right):

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
ash/wm/ash_activation_controller_unittest.cc:55: aura::Window* result =
ash_activation_controller_->WillActivateWindow(NULL);
On 2012/12/11 16:30:47, sky wrote:
> On 2012/12/11 05:03:32, mtomasz wrote:
> > On 2012/12/10 15:37:51, sky wrote:
> > > Rather than explicitly calling through like this can't you test
> > activation/focus
> > > directly?
> > 
> > I could, but would it be better? We would loose isolation without gaining
> > anything since ActivationController is already tested. This unit test checks
> > correctness of AshActivationController class and nothing more while avoiding
> > calling external dependencies whenever possible.
> 
> I'm for testing actual window activation since in the end that is what you
care
> about, especially since you can do it via a unit test still.

My concern is that I don't think we want AshActivationController's unit tests
fail if AshActivationController is not broken, but ActivationController (or
Launcher) is. That's my main concern. If I depend on ActivationController
(and/or Launcher), then it would happen. I'll do that if you want, though.

I am not sure if I understand correctly. Do you want me to make an end to end
test, like from deactivating the last window and then checking if the launcher
got finally activated? I think this is an integration test with lots of
dependencies. Don't we have a better place for such tests than *_unittest in wm
directory?

[sky, 2012-12-13 19:16:27]

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
File ash/wm/ash_activation_controller_unittest.cc (right):

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
ash/wm/ash_activation_controller_unittest.cc:55: aura::Window* result =
ash_activation_controller_->WillActivateWindow(NULL);
On 2012/12/13 07:55:29, mtomasz wrote:
> On 2012/12/11 16:30:47, sky wrote:
> > On 2012/12/11 05:03:32, mtomasz wrote:
> > > On 2012/12/10 15:37:51, sky wrote:
> > > > Rather than explicitly calling through like this can't you test
> > > activation/focus
> > > > directly?
> > > 
> > > I could, but would it be better? We would loose isolation without gaining
> > > anything since ActivationController is already tested. This unit test
checks
> > > correctness of AshActivationController class and nothing more while
avoiding
> > > calling external dependencies whenever possible.
> > 
> > I'm for testing actual window activation since in the end that is what you
> care
> > about, especially since you can do it via a unit test still.
> 
> My concern is that I don't think we want AshActivationController's unit tests
> fail if AshActivationController is not broken, but ActivationController (or
> Launcher) is. That's my main concern. If I depend on ActivationController
> (and/or Launcher), then it would happen. I'll do that if you want, though.
> 
> I am not sure if I understand correctly. Do you want me to make an end to end
> test, like from deactivating the last window and then checking if the launcher
> got finally activated?

Exactly.

> I think this is an integration test with lots of
> dependencies. Don't we have a better place for such tests than *_unittest in
wm
> directory?

How so? This test has access to everything. It should just be a matter of
showing the test window, closing it and checking the launcher has focus.

[mtomasz, 2012-12-14 05:31:42]

It would be great if we could commit this before the M25 cut.

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
File ash/wm/ash_activation_controller_unittest.cc (right):

https://codereview.chromium.org/11451002/diff/13001/ash/wm/ash_activation_con...
ash/wm/ash_activation_controller_unittest.cc:55: aura::Window* result =
ash_activation_controller_->WillActivateWindow(NULL);
On 2012/12/13 19:16:27, sky wrote:
> On 2012/12/13 07:55:29, mtomasz wrote:
> > On 2012/12/11 16:30:47, sky wrote:
> > > On 2012/12/11 05:03:32, mtomasz wrote:
> > > > On 2012/12/10 15:37:51, sky wrote:
> > > > > Rather than explicitly calling through like this can't you test
> > > > activation/focus
> > > > > directly?
> > > > 
> > > > I could, but would it be better? We would loose isolation without
gaining
> > > > anything since ActivationController is already tested. This unit test
> checks
> > > > correctness of AshActivationController class and nothing more while
> avoiding
> > > > calling external dependencies whenever possible.
> > > 
> > > I'm for testing actual window activation since in the end that is what you
> > care
> > > about, especially since you can do it via a unit test still.
> > 
> > My concern is that I don't think we want AshActivationController's unit
tests
> > fail if AshActivationController is not broken, but ActivationController (or
> > Launcher) is. That's my main concern. If I depend on ActivationController
> > (and/or Launcher), then it would happen. I'll do that if you want, though.
> > 
> > I am not sure if I understand correctly. Do you want me to make an end to
end
> > test, like from deactivating the last window and then checking if the
launcher
> > got finally activated?
> 
> Exactly.
> 
> > I think this is an integration test with lots of
> > dependencies. Don't we have a better place for such tests than *_unittest in
> wm
> > directory?
> 
> How so? This test has access to everything. It should just be a matter of
> showing the test window, closing it and checking the launcher has focus.

Done.

[sky, 2012-12-14 17:36:09]

LGTM

https://codereview.chromium.org/11451002/diff/23005/ash/wm/ash_activation_con...
File ash/wm/ash_activation_controller_unittest.cc (right):

https://codereview.chromium.org/11451002/diff/23005/ash/wm/ash_activation_con...
ash/wm/ash_activation_controller_unittest.cc:119: ASSERT_EQ(launcher_window_,
GetActiveWindow());
Can you also add a test that that verifies the launcher is not focused in this
scenario when spoken feedback is not enabled?

[mtomasz, 2012-12-15 02:14:08]

Thanks. This case is actually covered by one unit test, but I'll add an end to
end test as you like. I'm away from my workstation until Monday, so I'll do that
in a separate cl asap.

On 2012/12/14 17:36:09, sky wrote:
> LGTM
> 
>
https://codereview.chromium.org/11451002/diff/23005/ash/wm/ash_activation_con...
> File ash/wm/ash_activation_controller_unittest.cc (right):
> 
>
https://codereview.chromium.org/11451002/diff/23005/ash/wm/ash_activation_con...
> ash/wm/ash_activation_controller_unittest.cc:119: ASSERT_EQ(launcher_window_,
> GetActiveWindow());
> Can you also add a test that that verifies the launcher is not focused in this
> scenario when spoken feedback is not enabled?

[commit-bot: I haz the power, 2012-12-15 02:15:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/11451002/23005

[commit-bot: I haz the power, 2012-12-15 02:28:22]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-15 04:35:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/11451002/23005

[commit-bot: I haz the power, 2012-12-15 08:18:04]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-17 01:21:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/11451002/23005

[commit-bot: I haz the power, 2012-12-17 04:09:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/11451002/23005

[commit-bot: I haz the power, 2012-12-17 04:30:57]

Change committed as 173417

[Ryan Sleevi, 2012-12-17 04:37:17]

On 2012/12/17 04:30:57, I haz the power (commit-bot) wrote:
> Change committed as 173417

Can you please make sure commit messages are descriptive? You described this CL
in your first message - this would have been better as the actual commit
message.

Thanks!

[mtomasz1, 2012-12-17 04:39:59]

Ah, sorry for this mistake!

On Mon, Dec 17, 2012 at 1:37 PM, <rsleevi@chromium.org> wrote:

> On 2012/12/17 04:30:57, I haz the power (commit-bot) wrote:
>
>> Change committed as 173417
>>
>
> Can you please make sure commit messages are descriptive? You described
> this CL
> in your first message - this would have been better as the actual commit
> message.
>
> Thanks!
>
>
https://chromiumcodereview.**appspot.com/11451002/<https://chromiumcodereview...
>

[mtomasz, 2012-12-18 03:49:23]

This CL has been reverted.
Fixed version is now here:
https://codereview.chromium.org/11613017/

On 2012/12/17 04:39:59, mtomasz_google.com wrote:
> Ah, sorry for this mistake!
> 
> On Mon, Dec 17, 2012 at 1:37 PM, <mailto:rsleevi@chromium.org> wrote:
> 
> > On 2012/12/17 04:30:57, I haz the power (commit-bot) wrote:
> >
> >> Change committed as 173417
> >>
> >
> > Can you please make sure commit messages are descriptive? You described
> > this CL
> > in your first message - this would have been better as the actual commit
> > message.
> >
> > Thanks!
> >
> >
>
https://chromiumcodereview.**appspot.com/11451002/%3Chttps://chromiumcoderevi...>
> >