Refactor SetInOverview

Refactor SetInOverview

BUG=412547, 394895
Committed: https://crrev.com/c0caf9d7cc8e08f781fc81fbe02d30260964f348
Cr-Commit-Position: refs/heads/master@{#299308}

[Greg Levin, 2014-10-09 20:33:26]

glevin@chromium.org changed reviewers:
+ mukai@chromium.org, oshima@chromium.org, pkotwicz@chromium.org

[Greg Levin, 2014-10-09 20:33:27]

Here's the refactor-only portion of https://codereview.chromium.org/601333002/
(no bug fixes).  Please have a look!
(Note: ignore Patch Set #1... it's just there for my own reference when
remerging)

[Jun Mukai, 2014-10-09 21:13:45]

lgtm, but please wait the review from pkotwicz.

[pkotwicz, 2014-10-09 21:13:59]

LGTM with comments

https://codereview.chromium.org/635223004/diff/110001/athena/wm/window_manage...
File athena/wm/window_manager_impl.cc (left):

https://codereview.chromium.org/635223004/diff/110001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:274: 
Nit: Keep the white space.

In this case, it is ambiguous whether removing the whitespace is a positive
change. Given that you are not modifying the code around the whitespace it is
preferable not to modify the whitespace in this CL.

https://codereview.chromium.org/635223004/diff/110001/athena/wm/window_manage...
File athena/wm/window_manager_impl.cc (right):

https://codereview.chromium.org/635223004/diff/110001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:210: // Will reactivate the previously active
window
Nit: This comment should be in the window_manager.h
How about: "Exits overview and activates the previously active activity."

https://codereview.chromium.org/635223004/diff/110001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:233: // Only handles overview mode, will not
actiavte any windows
Nit: The comment should be in the .h file
For the comment, how about: "Exits overview mode without changing activation.
The caller should ensure that a window is active after exiting overview mode."

[Greg Levin, 2014-10-10 21:13:45]

Done.

https://codereview.chromium.org/635223004/diff/110001/athena/wm/window_manage...
File athena/wm/window_manager_impl.cc (left):

https://codereview.chromium.org/635223004/diff/110001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:274: 
On 2014/10/09 21:13:59, pkotwicz wrote:
> Nit: Keep the white space.
> 
> In this case, it is ambiguous whether removing the whitespace is a positive
> change. Given that you are not modifying the code around the whitespace it is
> preferable not to modify the whitespace in this CL.

Done.

https://codereview.chromium.org/635223004/diff/110001/athena/wm/window_manage...
File athena/wm/window_manager_impl.cc (right):

https://codereview.chromium.org/635223004/diff/110001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:210: // Will reactivate the previously active
window
On 2014/10/09 21:13:59, pkotwicz wrote:
> Nit: This comment should be in the window_manager.h
> How about: "Exits overview and activates the previously active activity."

Done.

https://codereview.chromium.org/635223004/diff/110001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:233: // Only handles overview mode, will not
actiavte any windows
On 2014/10/09 21:13:59, pkotwicz wrote:
> Nit: The comment should be in the .h file
> For the comment, how about: "Exits overview mode without changing activation.
> The caller should ensure that a window is active after exiting overview mode."

Done.

[oshima, 2014-10-10 23:01:56]

lgtm

[Mr4D (OOO till 08-26), 2014-10-13 14:08:02]

skuhne@chromium.org changed reviewers:
+ skuhne@chromium.org

[Mr4D (OOO till 08-26), 2014-10-13 14:08:03]

lgtm

[Mr4D (OOO till 08-26), 2014-10-13 14:08:06]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-10-13 14:08:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/635223004/180001

[commit-bot: I haz the power, 2014-10-13 14:44:01]

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

[commit-bot: I haz the power, 2014-10-13 14:44:02]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mr4D (OOO till 08-26), 2014-10-13 16:14:40]

lgtm

[Mr4D (OOO till 08-26), 2014-10-13 16:14:43]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-10-13 16:15:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/635223004/430001

[commit-bot: I haz the power, 2014-10-13 16:48:30]

Committed patchset #4 (id:430001)

[commit-bot: I haz the power, 2014-10-13 16:49:02]

Patchset 4 (id:??) landed as
https://crrev.com/c0caf9d7cc8e08f781fc81fbe02d30260964f348
Cr-Commit-Position: refs/heads/master@{#299308}