Expand and split DisplayController mojom.

Expand and split DisplayController mojom.

Add two IPC calls to mojom::DisplayController. The first is to enable a
keyboard shortcut for swapping primary display from ash. This is part of
the existing developer shortcuts and will enable testing primary display
work. Second, add a call to let ash to set the display workarea. The
workarea IPC call needs to be implemented on the ash side.

Split the existing IPC call to toggle adding/removing a fake display
into a new test interface.

Fix the way mus specific keyboard accelerators in ash work. The
accelerators were being triggered from HandlesAction() which is wrong.
Instead trigger accelerators from PerformAction().

BUG=612242
Committed: https://crrev.com/24ff7b7d6909448813aa15c0f9a2da2a4ce32af0
Cr-Commit-Position: refs/heads/master@{#425977}

[kylechar, 2016-10-13 16:14:35]

kylechar@chromium.org changed reviewers:
+ sky@chromium.org, tsepez@chromium.org

[kylechar, 2016-10-13 16:14:36]

sky: ash/* and general review.
tsepez: mojom security review.

[Tom Sepez, 2016-10-13 16:36:11]

On 2016/10/13 16:14:36, kylechar wrote:
> sky: ash/* and general review.
> tsepez: mojom security review.

Which clients are allowed to call this interface?  How is this enforced?

[kylechar, 2016-10-13 16:40:02]

On 2016/10/13 16:36:11, Tom Sepez wrote:
> On 2016/10/13 16:14:36, kylechar wrote:
> > sky: ash/* and general review.
> > tsepez: mojom security review.
> 
> Which clients are allowed to call this interface?  How is this enforced?

This is the privileged interface for clients that are allowed to make changes to
the display state. Right now it's just the wm (ash) that can use it. Whatever
process runs the setting dialog stuff (which as far as I'm aware doesn't exist
yet because no one has started making that work with mustash yet) would also
need to use it. This is enforced by the manifest.

https://cs.chromium.org/chromium/src/services/ui/manifest.json?type=cs&q=Disp...

[sky, 2016-10-13 18:11:23]

https://codereview.chromium.org/2415163002/diff/1/services/ui/public/interfac...
File services/ui/public/interfaces/display/display_controller.mojom (right):

https://codereview.chromium.org/2415163002/diff/1/services/ui/public/interfac...
services/ui/public/interfaces/display/display_controller.mojom:16:
ToggleAddRemoveDisplay();
This is more of a testing api where as the other two aren't, right? Should we
move this into it's own interface to make that clear?

[Tom Sepez, 2016-10-13 18:15:27]

lgtm

[kylechar, 2016-10-14 15:41:32]

https://codereview.chromium.org/2415163002/diff/1/services/ui/public/interfac...
File services/ui/public/interfaces/display/display_controller.mojom (right):

https://codereview.chromium.org/2415163002/diff/1/services/ui/public/interfac...
services/ui/public/interfaces/display/display_controller.mojom:16:
ToggleAddRemoveDisplay();
On 2016/10/13 18:11:23, sky wrote:
> This is more of a testing api where as the other two aren't, right? Should we
> move this into it's own interface to make that clear?

Yes, it's definitely more of a testing API. If I moved it into a new interface,
it would be the only IPC in it though.

I think if I were to move ToggleAddRemoveDisplay into a separate interface I'd
want to change how it works? Have the new API be similar to
VirtualDisplayController, have Ctrl+Shift+D open a dedicated application that
connects to the new interface and has buttons to add/remove/modify displays for
testing. That would make it a bit more powerful. It's also very low priority
work.

[sky, 2016-10-14 16:46:52]

On 2016/10/14 15:41:32, kylechar wrote:
>
https://codereview.chromium.org/2415163002/diff/1/services/ui/public/interfac...
> File services/ui/public/interfaces/display/display_controller.mojom (right):
> 
>
https://codereview.chromium.org/2415163002/diff/1/services/ui/public/interfac...
> services/ui/public/interfaces/display/display_controller.mojom:16:
> ToggleAddRemoveDisplay();
> On 2016/10/13 18:11:23, sky wrote:
> > This is more of a testing api where as the other two aren't, right? Should
we
> > move this into it's own interface to make that clear?
> 
> Yes, it's definitely more of a testing API. If I moved it into a new
interface,
> it would be the only IPC in it though.

It's fine to have an interface with a single function. I also suspect it will
grow with time. If we don't separate out the testing apis then it gets harder to
enforce who can make use of the testing api.

> I think if I were to move ToggleAddRemoveDisplay into a separate interface I'd
> want to change how it works? Have the new API be similar to
> VirtualDisplayController, have Ctrl+Shift+D open a dedicated application that
> connects to the new interface and has buttons to add/remove/modify displays
for
> testing. That would make it a bit more powerful. It's also very low priority
> work.

How about creating the separate interfaces as a first step? Changing what the
testing api actually does can be done later. Sound resonable?

[kylechar, 2016-10-17 15:54:03]

On 2016/10/14 16:46:52, sky wrote:
> On 2016/10/14 15:41:32, kylechar wrote:
> >
>
https://codereview.chromium.org/2415163002/diff/1/services/ui/public/interfac...
> > File services/ui/public/interfaces/display/display_controller.mojom (right):
> > 
> >
>
https://codereview.chromium.org/2415163002/diff/1/services/ui/public/interfac...
> > services/ui/public/interfaces/display/display_controller.mojom:16:
> > ToggleAddRemoveDisplay();
> > On 2016/10/13 18:11:23, sky wrote:
> > > This is more of a testing api where as the other two aren't, right? Should
> we
> > > move this into it's own interface to make that clear?
> > 
> > Yes, it's definitely more of a testing API. If I moved it into a new
> interface,
> > it would be the only IPC in it though.
> 
> It's fine to have an interface with a single function. I also suspect it will
> grow with time. If we don't separate out the testing apis then it gets harder
to
> enforce who can make use of the testing api.
> 
> > I think if I were to move ToggleAddRemoveDisplay into a separate interface
I'd
> > want to change how it works? Have the new API be similar to
> > VirtualDisplayController, have Ctrl+Shift+D open a dedicated application
that
> > connects to the new interface and has buttons to add/remove/modify displays
> for
> > testing. That would make it a bit more powerful. It's also very low priority
> > work.
> 
> How about creating the separate interfaces as a first step? Changing what the
> testing api actually does can be done later. Sound resonable?

Sounds reasonable. Done.

[sky, 2016-10-17 20:05:27]

LGTM - thanks!

[kylechar, 2016-10-17 20:19:30]

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

[commit-bot: I haz the power, 2016-10-17 20:20:08]

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-10-17 21:06:59]

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

[commit-bot: I haz the power, 2016-10-17 21:07:00]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kylechar, 2016-10-18 13:46:10]

Description was changed from

==========
Expand DisplayController mojom.

Add IPC call to enable keyboard shortcut for swapping primary display
from ash. This is part of the existing developer shortcuts and will
enable testing primary display work. Also add IPC call for ash to set
the display workarea. The workarea IPC call needs to be implemented on
the ash side.

Fix the way mus specific keyboard accelerators in ash work. The
accelerators were being triggered from HandlesAction() which is wrong.
Instead trigger accelerators from PerformAction().

BUG=612242
==========

to

==========
Expand and split DisplayController mojom.

Add two IPC calls to mojom::DisplayController. The first is to enable a
keyboard shortcut for swapping primary display from ash. This is part of
the existing developer shortcuts and will enable testing primary display
work. Second, add a call to let ash to set the display workarea. The
workarea IPC call needs to be implemented on the ash side.

Split the existing IPC call to toggle adding/removing a fake display
into a new test interface.

Fix the way mus specific keyboard accelerators in ash work. The
accelerators were being triggered from HandlesAction() which is wrong.
Instead trigger accelerators from PerformAction().

BUG=612242
==========

[kylechar, 2016-10-18 13:46:45]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2016-10-18 13:46:45]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2415163002/#ps40001
(title: "Fix case statements.")

[kylechar, 2016-10-18 13:46:55]

Description was changed from

==========
Expand and split DisplayController mojom.

Add two IPC calls to mojom::DisplayController. The first is to enable a
keyboard shortcut for swapping primary display from ash. This is part of
the existing developer shortcuts and will enable testing primary display
work. Second, add a call to let ash to set the display workarea. The
workarea IPC call needs to be implemented on the ash side.

Split the existing IPC call to toggle adding/removing a fake display
into a new test interface.

Fix the way mus specific keyboard accelerators in ash work. The
accelerators were being triggered from HandlesAction() which is wrong.
Instead trigger accelerators from PerformAction().

BUG=612242
==========

to

==========
Expand and split DisplayController mojom.

Add two IPC calls to mojom::DisplayController. The first is to enable a
keyboard shortcut for swapping primary display from ash. This is part of
the existing developer shortcuts and will enable testing primary display
work. Second, add a call to let ash to set the display workarea. The
workarea IPC call needs to be implemented on the ash side.

Split the existing IPC call to toggle adding/removing a fake display
into a new test interface.

Fix the way mus specific keyboard accelerators in ash work. The
accelerators were being triggered from HandlesAction() which is wrong.
Instead trigger accelerators from PerformAction().

BUG=612242
==========

[commit-bot: I haz the power, 2016-10-18 13:47:21]

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-10-18 14:02:18]

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

[commit-bot: I haz the power, 2016-10-18 14:02:19]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[kylechar, 2016-10-18 14:10:18]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2016-10-18 14:10:19]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2415163002/#ps60001
(title: "Actually fix case statements.")

[commit-bot: I haz the power, 2016-10-18 14:10:37]

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-10-18 15:59:30]

Description was changed from

==========
Expand and split DisplayController mojom.

Add two IPC calls to mojom::DisplayController. The first is to enable a
keyboard shortcut for swapping primary display from ash. This is part of
the existing developer shortcuts and will enable testing primary display
work. Second, add a call to let ash to set the display workarea. The
workarea IPC call needs to be implemented on the ash side.

Split the existing IPC call to toggle adding/removing a fake display
into a new test interface.

Fix the way mus specific keyboard accelerators in ash work. The
accelerators were being triggered from HandlesAction() which is wrong.
Instead trigger accelerators from PerformAction().

BUG=612242
==========

to

==========
Expand and split DisplayController mojom.

Add two IPC calls to mojom::DisplayController. The first is to enable a
keyboard shortcut for swapping primary display from ash. This is part of
the existing developer shortcuts and will enable testing primary display
work. Second, add a call to let ash to set the display workarea. The
workarea IPC call needs to be implemented on the ash side.

Split the existing IPC call to toggle adding/removing a fake display
into a new test interface.

Fix the way mus specific keyboard accelerators in ash work. The
accelerators were being triggered from HandlesAction() which is wrong.
Instead trigger accelerators from PerformAction().

BUG=612242
==========

[commit-bot: I haz the power, 2016-10-18 15:59:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-18 16:03:45]

Description was changed from

==========
Expand and split DisplayController mojom.

Add two IPC calls to mojom::DisplayController. The first is to enable a
keyboard shortcut for swapping primary display from ash. This is part of
the existing developer shortcuts and will enable testing primary display
work. Second, add a call to let ash to set the display workarea. The
workarea IPC call needs to be implemented on the ash side.

Split the existing IPC call to toggle adding/removing a fake display
into a new test interface.

Fix the way mus specific keyboard accelerators in ash work. The
accelerators were being triggered from HandlesAction() which is wrong.
Instead trigger accelerators from PerformAction().

BUG=612242
==========

to

==========
Expand and split DisplayController mojom.

Add two IPC calls to mojom::DisplayController. The first is to enable a
keyboard shortcut for swapping primary display from ash. This is part of
the existing developer shortcuts and will enable testing primary display
work. Second, add a call to let ash to set the display workarea. The
workarea IPC call needs to be implemented on the ash side.

Split the existing IPC call to toggle adding/removing a fake display
into a new test interface.

Fix the way mus specific keyboard accelerators in ash work. The
accelerators were being triggered from HandlesAction() which is wrong.
Instead trigger accelerators from PerformAction().

BUG=612242

Committed: https://crrev.com/24ff7b7d6909448813aa15c0f9a2da2a4ce32af0
Cr-Commit-Position: refs/heads/master@{#425977}
==========

[commit-bot: I haz the power, 2016-10-18 16:03:46]

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