Remove --ash-disable-overview-mode

Remove --ash-disable-overview-mode

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

[flackr, 2014-02-27 04:16:13]

Removing the ash disable overview mode flag and all obsolete code.

isherman@chromium.org: Please review changes in
tools/metrics/actions/chromeactions.txt.

oshima@chromium.org: Please review changes in ash. There were still a couple
calls into WindowCycleController which I replaced by activating the second most
recently used window in the mru window list and a test in ExtendedDesktopTest
which seemed redundant with the multi monitor tests in WindowSelectorTest.

[Ilya Sherman, 2014-02-27 23:11:46]

https://codereview.chromium.org/181563004/diff/40001/tools/metrics/actions/ch...
File tools/metrics/actions/chromeactions.txt (left):

https://codereview.chromium.org/181563004/diff/40001/tools/metrics/actions/ch...
tools/metrics/actions/chromeactions.txt:87:
0x5a27559e6846a75e	Accel_NextWindow_F5
Lines should never be removed from this file; only inserted.  This file is
intended to list both all of the current actions and all of the previously used
actions, to ease viewing of older data as well as that of current data.

[flackr, 2014-02-27 23:19:36]

https://codereview.chromium.org/181563004/diff/40001/tools/metrics/actions/ch...
File tools/metrics/actions/chromeactions.txt (left):

https://codereview.chromium.org/181563004/diff/40001/tools/metrics/actions/ch...
tools/metrics/actions/chromeactions.txt:87:
0x5a27559e6846a75e	Accel_NextWindow_F5
On 2014/02/27 23:11:46, Ilya Sherman wrote:
> Lines should never be removed from this file; only inserted.  This file is
> intended to list both all of the current actions and all of the previously
used
> actions, to ease viewing of older data as well as that of current data.

Thanks, I wasn't sure about this as there have been removals from this file in
the git history. Restored.

[Ilya Sherman, 2014-02-27 23:23:44]

https://codereview.chromium.org/181563004/diff/40001/tools/metrics/actions/ch...
File tools/metrics/actions/chromeactions.txt (left):

https://codereview.chromium.org/181563004/diff/40001/tools/metrics/actions/ch...
tools/metrics/actions/chromeactions.txt:87:
0x5a27559e6846a75e	Accel_NextWindow_F5
On 2014/02/27 23:19:37, flackr wrote:
> On 2014/02/27 23:11:46, Ilya Sherman wrote:
> > Lines should never be removed from this file; only inserted.  This file is
> > intended to list both all of the current actions and all of the previously
> used
> > actions, to ease viewing of older data as well as that of current data.
> 
> Thanks, I wasn't sure about this as there have been removals from this file in
> the git history. Restored.

Yeah, sometimes they sneak past the reviewers, alas.  Thanks for checking. 
Looks like there's nothing left for me to review here, so removing myself from
the reviewer list :)

[oshima, 2014-02-27 23:36:10]

https://codereview.chromium.org/181563004/diff/50001/ash/accelerators/acceler...
File ash/accelerators/accelerator_commands.cc (right):

https://codereview.chromium.org/181563004/diff/50001/ash/accelerators/acceler...
ash/accelerators/accelerator_commands.cc:27: }
do we have to activate the top most window in mru? can this be

if (mru_windows.size() > 1)
  wm::GetWindowState(mru_windows[1])->Activate();

instead?

https://codereview.chromium.org/181563004/diff/50001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/181563004/diff/50001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:130:
shell->window_selector_controller()->HandleCycleWindow(
nit: can you just do 

Shell::GetInstance()->... 

and still fit the line?

same for the rest.

[flackr, 2014-02-28 15:37:34]

Sorry I accidentally pulled a merge in with the latest patchset, but
accelerator_controller.cc and accelerator_commands.cc are the only changed
files. PTAL, thanks.

https://codereview.chromium.org/181563004/diff/50001/ash/accelerators/acceler...
File ash/accelerators/accelerator_commands.cc (right):

https://codereview.chromium.org/181563004/diff/50001/ash/accelerators/acceler...
ash/accelerators/accelerator_commands.cc:27: }
On 2014/02/27 23:36:10, oshima wrote:
> do we have to activate the top most window in mru? can this be
> 
> if (mru_windows.size() > 1)
>   wm::GetWindowState(mru_windows[1])->Activate();
> 
> instead?

I'm actually not sure why we used the second window here. In the case that you
have minimized everything, toggle minimize should probably restore the last
window. The 2-cycle that activating the second window allows for is just
strange. Changed to just activate the first window.

https://codereview.chromium.org/181563004/diff/50001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/181563004/diff/50001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:130:
shell->window_selector_controller()->HandleCycleWindow(
On 2014/02/27 23:36:10, oshima wrote:
> nit: can you just do 
> 
> Shell::GetInstance()->... 
> 
> and still fit the line?
> 
> same for the rest.

Done.

[oshima, 2014-02-28 17:54:39]

https://codereview.chromium.org/181563004/diff/50001/ash/accelerators/acceler...
File ash/accelerators/accelerator_commands.cc (right):

https://codereview.chromium.org/181563004/diff/50001/ash/accelerators/acceler...
ash/accelerators/accelerator_commands.cc:27: }
On 2014/02/28 15:37:34, flackr wrote:
> On 2014/02/27 23:36:10, oshima wrote:
> > do we have to activate the top most window in mru? can this be
> > 
> > if (mru_windows.size() > 1)
> >   wm::GetWindowState(mru_windows[1])->Activate();
> > 
> > instead?
> 
> I'm actually not sure why we used the second window here. In the case that you
> have minimized everything, toggle minimize should probably restore the last
> window. The 2-cycle that activating the second window allows for is just
> strange. Changed to just activate the first window.

Ah got it. Do we have a test for this scenario? If not, can you add one?

https://codereview.chromium.org/181563004/diff/70001/ash/focus_cycler.cc
File ash/focus_cycler.cc (right):

https://codereview.chromium.org/181563004/diff/70001/ash/focus_cycler.cc#newc...
ash/focus_cycler.cc:85: wm::GetWindowState(window)->Activate();
So this case, we're activating the top most (if there is only one) anyway
because the window may be minimized?

[flackr, 2014-03-11 19:55:06]

https://codereview.chromium.org/181563004/diff/50001/ash/accelerators/acceler...
File ash/accelerators/accelerator_commands.cc (right):

https://codereview.chromium.org/181563004/diff/50001/ash/accelerators/acceler...
ash/accelerators/accelerator_commands.cc:27: }
On 2014/02/28 17:54:39, oshima wrote:
> On 2014/02/28 15:37:34, flackr wrote:
> > On 2014/02/27 23:36:10, oshima wrote:
> > > do we have to activate the top most window in mru? can this be
> > > 
> > > if (mru_windows.size() > 1)
> > >   wm::GetWindowState(mru_windows[1])->Activate();
> > > 
> > > instead?
> > 
> > I'm actually not sure why we used the second window here. In the case that
you
> > have minimized everything, toggle minimize should probably restore the last
> > window. The 2-cycle that activating the second window allows for is just
> > strange. Changed to just activate the first window.
> 
> Ah got it. Do we have a test for this scenario? If not, can you add one?

Test updated to check this case. Done.

https://codereview.chromium.org/181563004/diff/70001/ash/focus_cycler.cc
File ash/focus_cycler.cc (right):

https://codereview.chromium.org/181563004/diff/70001/ash/focus_cycler.cc#newc...
ash/focus_cycler.cc:85: wm::GetWindowState(window)->Activate();
On 2014/02/28 17:54:39, oshima wrote:
> So this case, we're activating the top most (if there is only one) anyway
> because the window may be minimized?

Looking into this we only ever expect to cycle focus between the status area and
the last active browser. I've updated the test to reflect this and always used
.front().

[oshima, 2014-03-11 23:31:51]

lgtm

https://codereview.chromium.org/181563004/diff/90001/ash/focus_cycler.cc
File ash/focus_cycler.cc (right):

https://codereview.chromium.org/181563004/diff/90001/ash/focus_cycler.cc#newc...
ash/focus_cycler.cc:84: aura::Window* window(mru_windows.front());
nit:

window = 

is more common

[flackr, 2014-03-11 23:36:12]

https://codereview.chromium.org/181563004/diff/90001/ash/focus_cycler.cc
File ash/focus_cycler.cc (right):

https://codereview.chromium.org/181563004/diff/90001/ash/focus_cycler.cc#newc...
ash/focus_cycler.cc:84: aura::Window* window(mru_windows.front());
On 2014/03/11 23:31:52, oshima wrote:
> nit:
> 
> window = 
> 
> is more common

Done.

[flackr, 2014-03-11 23:36:35]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-03-11 23:42:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/181563004/110001

[commit-bot: I haz the power, 2014-03-12 08:27:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-12 08:27:15]

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

[flackr, 2014-03-12 13:32:59]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-03-12 13:33:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/181563004/130001

[commit-bot: I haz the power, 2014-03-12 16:55:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/181563004/130001

[commit-bot: I haz the power, 2014-03-12 19:14:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/181563004/130001

[commit-bot: I haz the power, 2014-03-12 21:13:03]

Change committed as 256645