Separates out accelerators using non-common types

Separates out accelerators using non-common types

These are the set of accelerators that have dependencies on parts not ported. I suspect some of these won't make sense in a mash world, but for now I'm lumping it all together.

BUG=612331
TEST=covered by tests
R=jamescook@chromium.org, reveman@chromium.org

Committed: https://crrev.com/44a098967d846660aea85e1ae5e6758ff3b6589e
Cr-Commit-Position: refs/heads/master@{#406607}

[sky, 2016-07-20 04:07:14]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[sky, 2016-07-20 04:07:28]

Description was changed from

==========
Separates out non-ported accelerators in own file

These are the set of accelerators that have dependencies on parts not
ported. I suspect some of these won't make sense in a mash world, but
for now I'm lumping it all together.

BUG=612331
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Separates out accelerators using non-common types

These are the set of accelerators that have dependencies on parts not ported. I
suspect some of these won't make sense in a mash world, but for now I'm lumping
it all together.

BUG=612331
TEST=covered by tests
R=jamescook@chromium.org
==========

[commit-bot: I haz the power, 2016-07-20 04:07:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 04:20:33]

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

[commit-bot: I haz the power, 2016-07-20 04:20:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-07-20 15:15:05]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 15:15:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2016-07-20 15:15:45]

Description was changed from

==========
Separates out accelerators using non-common types

These are the set of accelerators that have dependencies on parts not ported. I
suspect some of these won't make sense in a mash world, but for now I'm lumping
it all together.

BUG=612331
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Separates out accelerators using non-common types

These are the set of accelerators that have dependencies on parts not ported. I
suspect some of these won't make sense in a mash world, but for now I'm lumping
it all together.

BUG=612331
TEST=covered by tests
R=jamescook@chromium.org, reveman@chromium.org
==========

[sky, 2016-07-20 15:15:46]

sky@chromium.org changed reviewers:
+ reveman@chromium.org

[sky, 2016-07-20 15:15:54]

+reveman for exo

[reveman, 2016-07-20 15:41:35]

components/exo lgtm

[James Cook, 2016-07-20 16:08:04]

If this can wait a day or two there are a bunch of functions that wouldn't have
to move. See below and let me know what you think.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_commands.cc (right):

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_commands.cc:12: #include
"ash/display/display_manager.h"
not needed (and most of the ones below)

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_commands_aura.cc (right):

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_commands_aura.cc:7: #include
"ash/common/wm/mru_window_tracker.h"
not needed (and several below)

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_commands_aura.h (right):

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_commands_aura.h:5: #ifndef
ASH_ACCELERATORS_ACCELERATOR_COMMANDS_AURA_H_
Long-term will this file end up in ash/aura and the other one in ash/common?

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:149: // Rotate the active window.
remove this

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:641: return
delegate_->CanPerformAction(action, accelerator, previous_accelerator);
optional nit: This reads a little funny to me... maybe put the DCHECK outside
the switch, or the CanPerformAction inside?

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_delegate.h (right):

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate.h:46: };
Nice documentation in this class.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_delegate_aura.cc (right):

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:132:
base::RecordAction(base::UserMetricsAction("Accel_Focus_Shelf"));
nit: base:: not needed, here and below

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:180:
Shell::GetInstance()->new_window_delegate()->NewWindow(
NewWindowDelegate has no aura dependencies. What do you think of moving it to
ash/common/ before this CL? That would allow several of these functions not to
move and preserve their git history.

Or are you thinking the implementations of these will be quite different in mus,
like handed by mash/app_driver? It feels natural to me that the window
manager/system UI would handle the sorts of keys that route through
NewWindowDelegate, rather than having a separate app that does it.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:282: void
HandleShowSystemTrayBubble() {
How urgent is it to move all these status area / system tray shortcuts move
over? If they can wait a day or two I could write a CL to provide access to
StatusAreaWidget and SystemTray via WmRootWindowController, then these wouldn't
have to move.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:339:
Shell::GetInstance()->ToggleAppList(nullptr);
If it can wait, I have a CL that will go out for review this morning that moves
this to WmShell, so this and the function above wouldn't have to move.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_delegate_aura.h (right):

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2016

[commit-bot: I haz the power, 2016-07-20 16:11:17]

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

[commit-bot: I haz the power, 2016-07-20 16:11:18]

Dry run: This issue passed the CQ dry run.

[sky, 2016-07-20 16:42:50]

Diff between 4 and 5 to see just comments addressed. Patchset 6 is with a
--similarity=20 so that you can better see
accelerator_controller_delegate_aura.cc is really a copy. I should have done the
--similarity early on, sorry about that.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_commands.cc (right):

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_commands.cc:12: #include
"ash/display/display_manager.h"
On 2016/07/20 16:08:04, James Cook wrote:
> not needed (and most of the ones below)

Done.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_commands_aura.cc (right):

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_commands_aura.cc:7: #include
"ash/common/wm/mru_window_tracker.h"
On 2016/07/20 16:08:04, James Cook wrote:
> not needed (and several below)

Done.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_commands_aura.h (right):

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_commands_aura.h:5: #ifndef
ASH_ACCELERATORS_ACCELERATOR_COMMANDS_AURA_H_
On 2016/07/20 16:08:04, James Cook wrote:
> Long-term will this file end up in ash/aura and the other one in ash/common?

Yes, after I straighten out debug_commands I plan on moving the non aura files.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:149: // Rotate the active window.
On 2016/07/20 16:08:04, James Cook wrote:
> remove this

Done.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:641: return
delegate_->CanPerformAction(action, accelerator, previous_accelerator);
On 2016/07/20 16:08:04, James Cook wrote:
> optional nit: This reads a little funny to me... maybe put the DCHECK outside
> the switch, or the CanPerformAction inside?

I actually had it all in the default: originally with a return false here, but
windows compiler complains about that. I moved the comment.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_delegate_aura.cc (right):

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:132:
base::RecordAction(base::UserMetricsAction("Accel_Focus_Shelf"));
On 2016/07/20 16:08:04, James Cook wrote:
> nit: base:: not needed, here and below

Done.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:180:
Shell::GetInstance()->new_window_delegate()->NewWindow(
On 2016/07/20 16:08:04, James Cook wrote:
> NewWindowDelegate has no aura dependencies. What do you think of moving it to
> ash/common/ before this CL? That would allow several of these functions not to
> move and preserve their git history.
> 
> Or are you thinking the implementations of these will be quite different in
mus,
> like handed by mash/app_driver? It feels natural to me that the window
> manager/system UI would handle the sorts of keys that route through
> NewWindowDelegate, rather than having a separate app that does it.

I did see that NewWindowDelegate has no aura dependencies, but I suspect we
won't want it. And AppDriver is currently handling parts of NewWindowDelegate.
One of the reasons NewWindowDelegate feels awkward is that it means ash has
knowledge of functionality handled by other apps. For example, that ash knows
about creating tabs seems a bit like a layering violation.

With this patch I was hoping to separate things out into a portion that works
with mash. I was saving the deeper analysis of what process should handle
various portions for later. I should have made that clear, sorry.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:282: void
HandleShowSystemTrayBubble() {
On 2016/07/20 16:08:04, James Cook wrote:
> How urgent is it to move all these status area / system tray shortcuts move
> over? If they can wait a day or two I could write a CL to provide access to
> StatusAreaWidget and SystemTray via WmRootWindowController, then these
wouldn't
> have to move.

I would like to land this soon so I can complete the move. I'm happy to shuffle
things around as parts move to common.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:339:
Shell::GetInstance()->ToggleAppList(nullptr);
On 2016/07/20 16:08:04, James Cook wrote:
> If it can wait, I have a CL that will go out for review this morning that
moves
> this to WmShell, so this and the function above wouldn't have to move.

Similar comment to above.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_delegate_aura.h (right):

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2016/07/20 16:08:04, James Cook wrote:
> 2016

Done.

[sky, 2016-07-20 16:42:54]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 16:43:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-07-20 17:11:56]

LGTM

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_delegate_aura.cc (right):

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:180:
Shell::GetInstance()->new_window_delegate()->NewWindow(
On 2016/07/20 16:42:50, sky wrote:
> On 2016/07/20 16:08:04, James Cook wrote:
> > NewWindowDelegate has no aura dependencies. What do you think of moving it
to
> > ash/common/ before this CL? That would allow several of these functions not
to
> > move and preserve their git history.
> > 
> > Or are you thinking the implementations of these will be quite different in
> mus,
> > like handed by mash/app_driver? It feels natural to me that the window
> > manager/system UI would handle the sorts of keys that route through
> > NewWindowDelegate, rather than having a separate app that does it.
> 
> I did see that NewWindowDelegate has no aura dependencies, but I suspect we
> won't want it. And AppDriver is currently handling parts of NewWindowDelegate.
> One of the reasons NewWindowDelegate feels awkward is that it means ash has
> knowledge of functionality handled by other apps. For example, that ash knows
> about creating tabs seems a bit like a layering violation.
> 
> With this patch I was hoping to separate things out into a portion that works
> with mash. I was saving the deeper analysis of what process should handle
> various portions for later. I should have made that clear, sorry.

This is OK for now, but having AppDriver know the details of Chrome also seems
like a layering violation to me. It seems to me that if some piece of the system
has to handle Chrome-y shortcuts when no Chrome window is open, it should be the
wm/sysui. I would like to get rid of AppDriver.

https://codereview.chromium.org/2162193003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:282: void
HandleShowSystemTrayBubble() {
On 2016/07/20 16:42:50, sky wrote:
> On 2016/07/20 16:08:04, James Cook wrote:
> > How urgent is it to move all these status area / system tray shortcuts move
> > over? If they can wait a day or two I could write a CL to provide access to
> > StatusAreaWidget and SystemTray via WmRootWindowController, then these
> wouldn't
> > have to move.
> 
> I would like to land this soon so I can complete the move. I'm happy to
shuffle
> things around as parts move to common.

OK. I'll CC you on the bugs for the applist and system tray bugs so you'll know
when you can shuffle back.

[sky, 2016-07-20 17:28:18]

The CQ bit was unchecked by sky@chromium.org

[sky, 2016-07-20 17:28:24]

The CQ bit was checked by sky@chromium.org

[sky, 2016-07-20 17:28:24]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2162193003/#ps100001
(title: "sim=20")

[commit-bot: I haz the power, 2016-07-20 17:28:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 17:47:13]

Description was changed from

==========
Separates out accelerators using non-common types

These are the set of accelerators that have dependencies on parts not ported. I
suspect some of these won't make sense in a mash world, but for now I'm lumping
it all together.

BUG=612331
TEST=covered by tests
R=jamescook@chromium.org, reveman@chromium.org
==========

to

==========
Separates out accelerators using non-common types

These are the set of accelerators that have dependencies on parts not ported. I
suspect some of these won't make sense in a mash world, but for now I'm lumping
it all together.

BUG=612331
TEST=covered by tests
R=jamescook@chromium.org, reveman@chromium.org
==========

[commit-bot: I haz the power, 2016-07-20 17:47:14]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-20 17:49:21]

Description was changed from

==========
Separates out accelerators using non-common types

These are the set of accelerators that have dependencies on parts not ported. I
suspect some of these won't make sense in a mash world, but for now I'm lumping
it all together.

BUG=612331
TEST=covered by tests
R=jamescook@chromium.org, reveman@chromium.org
==========

to

==========
Separates out accelerators using non-common types

These are the set of accelerators that have dependencies on parts not ported. I
suspect some of these won't make sense in a mash world, but for now I'm lumping
it all together.

BUG=612331
TEST=covered by tests
R=jamescook@chromium.org, reveman@chromium.org

Committed: https://crrev.com/44a098967d846660aea85e1ae5e6758ff3b6589e
Cr-Commit-Position: refs/heads/master@{#406607}
==========

[commit-bot: I haz the power, 2016-07-20 17:49:22]

Patchset 6 (id:??) landed as
https://crrev.com/44a098967d846660aea85e1ae5e6758ff3b6589e
Cr-Commit-Position: refs/heads/master@{#406607}