Add mojom::DisplayController.

Add mojom::DisplayController.

Add a new mojo interface mojom::DisplayController to allow privileged
clients to make changes to the display state. The initial interface
contains methods needed by mash to provide display state related
keyboard accelerators that exist in cash currently.

Have PlatformScreenOzone implement this new interface and provide mostly
stub implementations. Right now the interface isn't wired up so that
will happen in a subsequent CL.

Also adds PlatformScreen::GetInstance() so PlatformScreen doesn't need
to be passed so many places.

BUG=611475
Committed: https://crrev.com/b39238a88385b5f0d7e93ff1195c9909e4d6c414
Cr-Commit-Position: refs/heads/master@{#417578}

[kylechar, 2016-09-06 20:11:21]

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

[sky, 2016-09-06 21:25:47]

I think this code should be in ash. See DEBUG_ADD_REMOVE_DISPLAY.

[kylechar, 2016-09-06 21:28:53]

On 2016/09/06 21:25:47, sky wrote:
> I think this code should be in ash. See DEBUG_ADD_REMOVE_DISPLAY.

What part of the code exactly? I modeled this CL after DEBUG_ADD_REMOVE_DISPLAY
in ash. I suppose there is no reason the accelerator couldn't be defined in ash
but it would require an IPC hop to trigger. We've moved all display management
from ash to ui though.

[sky, 2016-09-06 22:15:19]

On 2016/09/06 21:28:53, kylechar wrote:
> On 2016/09/06 21:25:47, sky wrote:
> > I think this code should be in ash. See DEBUG_ADD_REMOVE_DISPLAY.
> 
> What part of the code exactly? I modeled this CL after
DEBUG_ADD_REMOVE_DISPLAY
> in ash. I suppose there is no reason the accelerator couldn't be defined in
ash
> but it would require an IPC hop to trigger. We've moved all display management
> from ash to ui though.

Sorry for not being more descriptive. I was thinking the WindowManager would
have an api that results in creating a new PlatformDisplay. I don't think we
need anything fake.

[kylechar, 2016-09-06 22:42:46]

On 2016/09/06 22:15:19, sky wrote:
> On 2016/09/06 21:28:53, kylechar wrote:
> > On 2016/09/06 21:25:47, sky wrote:
> > > I think this code should be in ash. See DEBUG_ADD_REMOVE_DISPLAY.
> > 
> > What part of the code exactly? I modeled this CL after
> DEBUG_ADD_REMOVE_DISPLAY
> > in ash. I suppose there is no reason the accelerator couldn't be defined in
> ash
> > but it would require an IPC hop to trigger. We've moved all display
management
> > from ash to ui though.
> 
> Sorry for not being more descriptive. I was thinking the WindowManager would
> have an api that results in creating a new PlatformDisplay. I don't think we
> need anything fake.

Well, fake isn't the most descriptive word and maybe it should be
AddRemoveVirtualDisplay(). However, all displays off device are virtual(/fake)
in that they don't correspond to a physical display. So we always have virtual
displays, this is just adding or removing an extra one to make it possible to
test display management (plus WS and WM code) without deploying mustash to a
device.

In terms of the WM having an API to create a new PlatformDisplays, I'm not sure
I totally follow. The WM (ash) process doesn't know what PlatformDisplay is.
It's the WS (ui or mus) that knows about PlatformDisplay and creates one
corresponding to each physical and/or virtual display. The code I added in this
CL creates a new PlatformDisplay for each virtual display.

If you mean have the WS could have an API to create/remove virtual displays then
absolutely that's possible and it's planned as part of mojom::DisplayController.
The WM could then request a new virtual display is created or destroyed in
response to an accelerator instead of having an accelerator in the WS. However,
moving the accelerator further away from display management code (from WS to WM)
adds some async complexity.

[rjkroege, 2016-09-06 23:32:49]

On 2016/09/06 22:42:46, kylechar wrote:
> On 2016/09/06 22:15:19, sky wrote:
> > On 2016/09/06 21:28:53, kylechar wrote:
> > > On 2016/09/06 21:25:47, sky wrote:
> > > > I think this code should be in ash. See DEBUG_ADD_REMOVE_DISPLAY.
> > > 
> > > What part of the code exactly? I modeled this CL after
> > DEBUG_ADD_REMOVE_DISPLAY
> > > in ash. I suppose there is no reason the accelerator couldn't be defined
in
> > ash
> > > but it would require an IPC hop to trigger. We've moved all display
> management
> > > from ash to ui though.
> > 
> > Sorry for not being more descriptive. I was thinking the WindowManager would
> > have an api that results in creating a new PlatformDisplay. I don't think we
> > need anything fake.
> 
> Well, fake isn't the most descriptive word and maybe it should be
> AddRemoveVirtualDisplay(). However, all displays off device are virtual(/fake)
> in that they don't correspond to a physical display. So we always have virtual
> displays, this is just adding or removing an extra one to make it possible to
> test display management (plus WS and WM code) without deploying mustash to a
> device.
> 
> In terms of the WM having an API to create a new PlatformDisplays, I'm not
sure
> I totally follow. The WM (ash) process doesn't know what PlatformDisplay is.
> It's the WS (ui or mus) that knows about PlatformDisplay and creates one
> corresponding to each physical and/or virtual display. The code I added in
this
> CL creates a new PlatformDisplay for each virtual display.
> 
> If you mean have the WS could have an API to create/remove virtual displays
then
> absolutely that's possible and it's planned as part of
mojom::DisplayController.
> The WM could then request a new virtual display is created or destroyed in
> response to an accelerator instead of having an accelerator in the WS.
However,
> moving the accelerator further away from display management code (from WS to
WM)
> adds some async complexity.

I thought the point here was to add a facility to ozone x11 to simulate the code
flow that would happen if a physical display is added to the system. Given that,
why is it necessary to change anything outside of ozone x11? It can already
sniff the event stream and do something appropriate?

[kylechar, 2016-09-07 00:04:31]

On 2016/09/06 23:32:49, rjkroege wrote:
> On 2016/09/06 22:42:46, kylechar wrote:
> > On 2016/09/06 22:15:19, sky wrote:
> > > On 2016/09/06 21:28:53, kylechar wrote:
> > > > On 2016/09/06 21:25:47, sky wrote:
> > > > > I think this code should be in ash. See DEBUG_ADD_REMOVE_DISPLAY.
> > > > 
> > > > What part of the code exactly? I modeled this CL after
> > > DEBUG_ADD_REMOVE_DISPLAY
> > > > in ash. I suppose there is no reason the accelerator couldn't be defined
> in
> > > ash
> > > > but it would require an IPC hop to trigger. We've moved all display
> > management
> > > > from ash to ui though.
> > > 
> > > Sorry for not being more descriptive. I was thinking the WindowManager
would
> > > have an api that results in creating a new PlatformDisplay. I don't think
we
> > > need anything fake.
> > 
> > Well, fake isn't the most descriptive word and maybe it should be
> > AddRemoveVirtualDisplay(). However, all displays off device are
virtual(/fake)
> > in that they don't correspond to a physical display. So we always have
virtual
> > displays, this is just adding or removing an extra one to make it possible
to
> > test display management (plus WS and WM code) without deploying mustash to a
> > device.
> > 
> > In terms of the WM having an API to create a new PlatformDisplays, I'm not
> sure
> > I totally follow. The WM (ash) process doesn't know what PlatformDisplay is.
> > It's the WS (ui or mus) that knows about PlatformDisplay and creates one
> > corresponding to each physical and/or virtual display. The code I added in
> this
> > CL creates a new PlatformDisplay for each virtual display.
> > 
> > If you mean have the WS could have an API to create/remove virtual displays
> then
> > absolutely that's possible and it's planned as part of
> mojom::DisplayController.
> > The WM could then request a new virtual display is created or destroyed in
> > response to an accelerator instead of having an accelerator in the WS.
> However,
> > moving the accelerator further away from display management code (from WS to
> WM)
> > adds some async complexity.
> 
> I thought the point here was to add a facility to ozone x11 to simulate the
code
> flow that would happen if a physical display is added to the system. Given
that,
> why is it necessary to change anything outside of ozone x11? It can already
> sniff the event stream and do something appropriate?

Right now DisplayConfigurator is setup to manage virtual displays (which is
something that exist previous to my work). Ideally, we want to move the virtual
displays lower in the stack so that NativeDisplayDelegate manages them instead.
This would be part of Ozone.

However, there already exists ~4 different ways to manage fake displays for
testing (ash::DisplayManager has it's own system, ui::DisplayConfigurator has
VirtualDisplaySnapshots, Ozone has some command line flags and there are
TestDisplaySnapshots for unit tests) so it seemed counter duplicate it again a
fifth time.

I think in the future it would good to move the management of virtual displays
down the stack into NDD or somewhere similar. The Ozone and ash::DisplayManager
versions could be replaced by virtual displays. Although I don't think that
Ozone X11 should sniff events and manage it's own set of accelerators, instead a
Mojo interface could be made available to control virtual displays.

[kylechar, 2016-09-07 00:18:51]

On 2016/09/07 00:04:31, kylechar wrote:
> On 2016/09/06 23:32:49, rjkroege wrote:
> > On 2016/09/06 22:42:46, kylechar wrote:
> > > On 2016/09/06 22:15:19, sky wrote:
> > > > On 2016/09/06 21:28:53, kylechar wrote:
> > > > > On 2016/09/06 21:25:47, sky wrote:
> > > > > > I think this code should be in ash. See DEBUG_ADD_REMOVE_DISPLAY.
> > > > > 
> > > > > What part of the code exactly? I modeled this CL after
> > > > DEBUG_ADD_REMOVE_DISPLAY
> > > > > in ash. I suppose there is no reason the accelerator couldn't be
defined
> > in
> > > > ash
> > > > > but it would require an IPC hop to trigger. We've moved all display
> > > management
> > > > > from ash to ui though.
> > > > 
> > > > Sorry for not being more descriptive. I was thinking the WindowManager
> would
> > > > have an api that results in creating a new PlatformDisplay. I don't
think
> we
> > > > need anything fake.
> > > 
> > > Well, fake isn't the most descriptive word and maybe it should be
> > > AddRemoveVirtualDisplay(). However, all displays off device are
> virtual(/fake)
> > > in that they don't correspond to a physical display. So we always have
> virtual
> > > displays, this is just adding or removing an extra one to make it possible
> to
> > > test display management (plus WS and WM code) without deploying mustash to
a
> > > device.
> > > 
> > > In terms of the WM having an API to create a new PlatformDisplays, I'm not
> > sure
> > > I totally follow. The WM (ash) process doesn't know what PlatformDisplay
is.
> > > It's the WS (ui or mus) that knows about PlatformDisplay and creates one
> > > corresponding to each physical and/or virtual display. The code I added in
> > this
> > > CL creates a new PlatformDisplay for each virtual display.
> > > 
> > > If you mean have the WS could have an API to create/remove virtual
displays
> > then
> > > absolutely that's possible and it's planned as part of
> > mojom::DisplayController.
> > > The WM could then request a new virtual display is created or destroyed in
> > > response to an accelerator instead of having an accelerator in the WS.
> > However,
> > > moving the accelerator further away from display management code (from WS
to
> > WM)
> > > adds some async complexity.
> > 
> > I thought the point here was to add a facility to ozone x11 to simulate the
> code
> > flow that would happen if a physical display is added to the system. Given
> that,
> > why is it necessary to change anything outside of ozone x11? It can already
> > sniff the event stream and do something appropriate?
> 
> Right now DisplayConfigurator is setup to manage virtual displays (which is
> something that exist previous to my work). Ideally, we want to move the
virtual
> displays lower in the stack so that NativeDisplayDelegate manages them
instead.
> This would be part of Ozone.
> 
> However, there already exists ~4 different ways to manage fake displays for
> testing (ash::DisplayManager has it's own system, ui::DisplayConfigurator has
> VirtualDisplaySnapshots, Ozone has some command line flags and there are
> TestDisplaySnapshots for unit tests) so it seemed counter duplicate it again a
> fifth time.
> 
> I think in the future it would good to move the management of virtual displays
> down the stack into NDD or somewhere similar. The Ozone and
ash::DisplayManager
> versions could be replaced by virtual displays. Although I don't think that
> Ozone X11 should sniff events and manage it's own set of accelerators, instead
a
> Mojo interface could be made available to control virtual displays.

Oh, also it can't be part of Ozone X11. It can be part of Ozone but it needs to
available in headless (where tests are run) and possibly other Ozone platforms.

[sky, 2016-09-07 03:50:10]

I'm suggesting that ash triggers something like CreateWindowTreeHost.
Which I think is something similar to what you have here, but it's ash
calling into a special function on WindowManagerClient to do this, not
baking the accelerator into mus.

  -Scott

On Tue, Sep 6, 2016 at 3:42 PM,  <kylechar@chromium.org> wrote:
> On 2016/09/06 22:15:19, sky wrote:
>> On 2016/09/06 21:28:53, kylechar wrote:
>> > On 2016/09/06 21:25:47, sky wrote:
>> > > I think this code should be in ash. See DEBUG_ADD_REMOVE_DISPLAY.
>> >
>> > What part of the code exactly? I modeled this CL after
>> DEBUG_ADD_REMOVE_DISPLAY
>> > in ash. I suppose there is no reason the accelerator couldn't be defined
>> > in
>> ash
>> > but it would require an IPC hop to trigger. We've moved all display
> management
>> > from ash to ui though.
>>
>> Sorry for not being more descriptive. I was thinking the WindowManager
>> would
>> have an api that results in creating a new PlatformDisplay. I don't think
>> we
>> need anything fake.
>
> Well, fake isn't the most descriptive word and maybe it should be
> AddRemoveVirtualDisplay(). However, all displays off device are
> virtual(/fake)
> in that they don't correspond to a physical display. So we always have
> virtual
> displays, this is just adding or removing an extra one to make it possible
> to
> test display management (plus WS and WM code) without deploying mustash to a
> device.
>
> In terms of the WM having an API to create a new PlatformDisplays, I'm not
> sure
> I totally follow. The WM (ash) process doesn't know what PlatformDisplay is.
> It's the WS (ui or mus) that knows about PlatformDisplay and creates one
> corresponding to each physical and/or virtual display. The code I added in
> this
> CL creates a new PlatformDisplay for each virtual display.
>
> If you mean have the WS could have an API to create/remove virtual displays
> then
> absolutely that's possible and it's planned as part of
> mojom::DisplayController.
> The WM could then request a new virtual display is created or destroyed in
> response to an accelerator instead of having an accelerator in the WS.
> However,
> moving the accelerator further away from display management code (from WS to
> WM)
> adds some async complexity.
>
> https://codereview.chromium.org/2310133002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rjkroege, 2016-09-07 17:31:57]

On Tuesday, September 6, 2016, Scott Violet <sky@chromium.org
<javascript:_e(%7B%7D,'cvml','sky@chromium.org');>> wrote:

> I'm suggesting that ash triggers something like CreateWindowTreeHost.
> Which I think is something similar to what you have here, but it's ash
> calling into a special function on WindowManagerClient to do this, not
> baking the accelerator into mus.
>

Maybe this is worth doing too? But it doesn't address the use case of
display code testing and debugging that I was hoping to see. kylechar@: if
you had intended to address my use case in a different CL, feel free to
tell me to go away here.

What I had wanted is a way (key, button, external command -- doesn't matter
-- but it needs to be exposed programmatically so that tests can use it)
that tells ozone x11 that it should call ui::DisplayObserver::OnDisplayAdded.
And henceforth, all the rest of the code in mus and ash operates exactly as
it would if a user plugged in a new physical display on a Chrome book.
i.e.: all control flow above the ozone API boundary would be identical
between CrOS devices and oxygen in the build bots.



>
>   -Scott
>
> On Tue, Sep 6, 2016 at 3:42 PM,  <kylechar@chromium.org> wrote:
> > On 2016/09/06 22:15:19, sky wrote:
> >> On 2016/09/06 21:28:53, kylechar wrote:
> >> > On 2016/09/06 21:25:47, sky wrote:
> >> > > I think this code should be in ash. See DEBUG_ADD_REMOVE_DISPLAY.
> >> >
> >> > What part of the code exactly? I modeled this CL after
> >> DEBUG_ADD_REMOVE_DISPLAY
> >> > in ash. I suppose there is no reason the accelerator couldn't be
> defined
> >> > in
> >> ash
> >> > but it would require an IPC hop to trigger. We've moved all display
> > management
> >> > from ash to ui though.
> >>
> >> Sorry for not being more descriptive. I was thinking the WindowManager
> >> would
> >> have an api that results in creating a new PlatformDisplay. I don't
> think
> >> we
> >> need anything fake.
> >
> > Well, fake isn't the most descriptive word and maybe it should be
> > AddRemoveVirtualDisplay(). However, all displays off device are
> > virtual(/fake)
> > in that they don't correspond to a physical display. So we always have
> > virtual
> > displays, this is just adding or removing an extra one to make it
> possible
> > to
> > test display management (plus WS and WM code) without deploying mustash
> to a
> > device.
> >
> > In terms of the WM having an API to create a new PlatformDisplays, I'm
> not
> > sure
> > I totally follow. The WM (ash) process doesn't know what PlatformDisplay
> is.
> > It's the WS (ui or mus) that knows about PlatformDisplay and creates one
> > corresponding to each physical and/or virtual display. The code I added
> in
> > this
> > CL creates a new PlatformDisplay for each virtual display.
> >
> > If you mean have the WS could have an API to create/remove virtual
> displays
> > then
> > absolutely that's possible and it's planned as part of
> > mojom::DisplayController.
> > The WM could then request a new virtual display is created or destroyed
> in
> > response to an accelerator instead of having an accelerator in the WS.
> > However,
> > moving the accelerator further away from display management code (from
> WS to
> > WM)
> > adds some async complexity.
> >
> > https://codereview.chromium.org/2310133002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2016-09-07 17:35:29]

On Wed, Sep 7, 2016 at 10:31 AM, Robert Kroeger <rjkroege@chromium.org> wrote:
>
>
> On Tuesday, September 6, 2016, Scott Violet <sky@chromium.org> wrote:
>>
>> I'm suggesting that ash triggers something like CreateWindowTreeHost.
>> Which I think is something similar to what you have here, but it's ash
>> calling into a special function on WindowManagerClient to do this, not
>> baking the accelerator into mus.
>
>
> Maybe this is worth doing too? But it doesn't address the use case of
> display code testing and debugging that I was hoping to see. kylechar@: if
> you had intended to address my use case in a different CL, feel free to tell
> me to go away here.
>
> What I had wanted is a way (key, button, external command -- doesn't matter
> -- but it needs to be exposed programmatically so that tests can use it)

Wouldn't the api I'm suggesting handle the test case?

  -Scott

> that tells ozone x11 that it should call
> ui::DisplayObserver::OnDisplayAdded. And henceforth, all the rest of the
> code in mus and ash operates exactly as it would if a user plugged in a new
> physical display on a Chrome book. i.e.: all control flow above the ozone
> API boundary would be identical between CrOS devices and oxygen in the build
> bots.
>
>
>>
>>
>>   -Scott
>>
>> On Tue, Sep 6, 2016 at 3:42 PM,  <kylechar@chromium.org> wrote:
>> > On 2016/09/06 22:15:19, sky wrote:
>> >> On 2016/09/06 21:28:53, kylechar wrote:
>> >> > On 2016/09/06 21:25:47, sky wrote:
>> >> > > I think this code should be in ash. See DEBUG_ADD_REMOVE_DISPLAY.
>> >> >
>> >> > What part of the code exactly? I modeled this CL after
>> >> DEBUG_ADD_REMOVE_DISPLAY
>> >> > in ash. I suppose there is no reason the accelerator couldn't be
>> >> > defined
>> >> > in
>> >> ash
>> >> > but it would require an IPC hop to trigger. We've moved all display
>> > management
>> >> > from ash to ui though.
>> >>
>> >> Sorry for not being more descriptive. I was thinking the WindowManager
>> >> would
>> >> have an api that results in creating a new PlatformDisplay. I don't
>> >> think
>> >> we
>> >> need anything fake.
>> >
>> > Well, fake isn't the most descriptive word and maybe it should be
>> > AddRemoveVirtualDisplay(). However, all displays off device are
>> > virtual(/fake)
>> > in that they don't correspond to a physical display. So we always have
>> > virtual
>> > displays, this is just adding or removing an extra one to make it
>> > possible
>> > to
>> > test display management (plus WS and WM code) without deploying mustash
>> > to a
>> > device.
>> >
>> > In terms of the WM having an API to create a new PlatformDisplays, I'm
>> > not
>> > sure
>> > I totally follow. The WM (ash) process doesn't know what PlatformDisplay
>> > is.
>> > It's the WS (ui or mus) that knows about PlatformDisplay and creates one
>> > corresponding to each physical and/or virtual display. The code I added
>> > in
>> > this
>> > CL creates a new PlatformDisplay for each virtual display.
>> >
>> > If you mean have the WS could have an API to create/remove virtual
>> > displays
>> > then
>> > absolutely that's possible and it's planned as part of
>> > mojom::DisplayController.
>> > The WM could then request a new virtual display is created or destroyed
>> > in
>> > response to an accelerator instead of having an accelerator in the WS.
>> > However,
>> > moving the accelerator further away from display management code (from
>> > WS to
>> > WM)
>> > adds some async complexity.
>> >
>> > https://codereview.chromium.org/2310133002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kylechar, 2016-09-07 18:38:06]

Let's say the stack of class involved looks roughly like the following, from
highest (mus clients) to lowest (graphics hardware):

<ash process>
------- process boundary -------
ui::ws::WindowServer
ui::ws::DisplayManager
display::PlatformScreen
ui::DisplayConfigurator
ui::NativeDisplayDelegate
------- process boundary -------
<gpu process>

I just want to double check I understand both requests.

sky: You want the accelerator Ctrl+Shift+D to be located higher up in the stack,
in the ash process instead of mus process. All the work relating to displays
still has to be in mus, because that's where all of the display related code
lives, but the accelerator could totally be moved to ash. There just needs to be
a mojo IPC call for it.

The high road approach is make move the accelerator into ash and add some way
for ash to tell mus to add/remove a display. The load road approach is to add a
TODO by the accelerator in mus saying to move it ash later.

rjkroege: You want the location where the virtual displays are managed to be
moved down one level to NDD. This would totally be possible. There is already
excessive code duplication related to test displays. Also, the code I'm using
actually injects the virtual displays into the results received from NDD, so as
far as everything above NDD is concerned the code path should be identical.So it
wouldn't

The high road approach is to unify some of the test display code and have a NDD
implementation that tracks virtual displays instead of real displays. That NDD
could also provide the necessary mojo interface to allow
adding/removing/modifying displays both by ash and in tests. This is the low
road approach, which is nearly identical in terms of what code gets exercised.

[sky, 2016-09-07 21:41:11]

On Wed, Sep 7, 2016 at 11:38 AM,  <kylechar@chromium.org> wrote:
> Let's say the stack of class involved looks roughly like the following, from
> highest (mus clients) to lowest (graphics hardware):
>
> <ash process>
> ------- process boundary -------
> ui::ws::WindowServer
> ui::ws::DisplayManager
> display::PlatformScreen
> ui::DisplayConfigurator
> ui::NativeDisplayDelegate
> ------- process boundary -------
> <gpu process>
>
> I just want to double check I understand both requests.
>
> sky: You want the accelerator Ctrl+Shift+D to be located higher up in the
> stack,
> in the ash process instead of mus process. All the work relating to displays
> still has to be in mus, because that's where all of the display related code
> lives, but the accelerator could totally be moved to ash. There just needs
> to be
> a mojo IPC call for it.

Exactly.

> The high road approach is make move the accelerator into ash and add some
> way
> for ash to tell mus to add/remove a display. The load road approach is to
> add a
> TODO by the accelerator in mus saying to move it ash later.

My rationale for moving to ash is that the accelerator and even
whether there is an accelerator is specific to the window manager.
That's who should be triggering the code, not mus itself.

  -Scott

> rjkroege: You want the location where the virtual displays are managed to be
> moved down one level to NDD. This would totally be possible. There is
> already
> excessive code duplication related to test displays. Also, the code I'm
> using
> actually injects the virtual displays into the results received from NDD, so
> as
> far as everything above NDD is concerned the code path should be
> identical.So it
> wouldn't
>
> The high road approach is to unify some of the test display code and have a
> NDD
> implementation that tracks virtual displays instead of real displays. That
> NDD
> could also provide the necessary mojo interface to allow
> adding/removing/modifying displays both by ash and in tests. This is the low
> road approach, which is nearly identical in terms of what code gets
> exercised.
>
> https://codereview.chromium.org/2310133002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kylechar, 2016-09-08 17:26:42]

Description was changed from

==========
Add keyboard shortcut to add/remove display in mustash.

Add a keyboard shortcut Ctrl+Shift+D that when running mustash off
device adds or removes a "display". If there is only one display then a
new display of the same size is added. If there is more than one display
then the last display is removed. Does nothing when running on device.

Also adds PlatformScreen::GetInstance() so it doesn't need to be passed
around all over the WS.

BUG=611475
==========

to

==========
Add mojom::DisplayController.

Add a new mojo interface mojom::DisplayController to allow privileged
clients to make changes to the display state. The initial interface
contains methods needed by mash to provide display state related
keyboard accelerators that exist in cash currently.

Have PlatformScreenOzone implement this new interface and provide mostly
stub implementations. Right now the interface isn't wired up so that
will happen in a subsequent CL.

Also adds PlatformScreen::GetInstance() so PlatformScreen doesn't need
to be passed so many places.

BUG=611475
==========

[kylechar, 2016-09-08 17:29:32]

Patchset #3 (id:40001) has been deleted

[kylechar, 2016-09-08 17:31:47]

sky: I've added a mojo interface to control adding/removing the display. I've
also removed the accelerator in mus. Wiring up the interface and adding the
keyboard accelerator in mus will have to come in a follow up CL.

rjkroege: I've started on a design doc and implementation of a
NativeDisplayDelegateVirtual that meets your requirements. I'll circulate the
design doc shortly.

[sky, 2016-09-08 18:03:34]

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

[sky, 2016-09-08 18:03:35]

LGTM - I added dcheng for new mojom.

https://codereview.chromium.org/2310133002/diff/60001/services/ui/display/pla...
File services/ui/display/platform_screen.cc (right):

https://codereview.chromium.org/2310133002/diff/60001/services/ui/display/pla...
services/ui/display/platform_screen.cc:11: PlatformScreen*
PlatformScreen::instance_ = nullptr;
Typically we comment with a
// static
on the previous line.

https://codereview.chromium.org/2310133002/diff/60001/services/ui/display/pla...
File services/ui/display/platform_screen.h (right):

https://codereview.chromium.org/2310133002/diff/60001/services/ui/display/pla...
services/ui/display/platform_screen.h:22: // Creates a singleton PlatformScreen
instance that is owned by caller.
'caller' -> 'the caller'.

optional: that this returns a unique_ptr implies it's owned, so consider nuking
that part of the comment.

https://codereview.chromium.org/2310133002/diff/60001/services/ui/display/pla...
File services/ui/display/platform_screen_ozone.cc (right):

https://codereview.chromium.org/2310133002/diff/60001/services/ui/display/pla...
services/ui/display/platform_screen_ozone.cc:87: // TODO(kylechar): Implement.
Use NOTIMPLEMENTED on all of these.

[kylechar, 2016-09-08 18:11:17]

Patchset #4 (id:80001) has been deleted

[kylechar, 2016-09-08 18:12:04]

https://codereview.chromium.org/2310133002/diff/60001/services/ui/display/pla...
File services/ui/display/platform_screen.cc (right):

https://codereview.chromium.org/2310133002/diff/60001/services/ui/display/pla...
services/ui/display/platform_screen.cc:11: PlatformScreen*
PlatformScreen::instance_ = nullptr;
On 2016/09/08 18:03:35, sky wrote:
> Typically we comment with a
> // static
> on the previous line.

Done.

https://codereview.chromium.org/2310133002/diff/60001/services/ui/display/pla...
File services/ui/display/platform_screen.h (right):

https://codereview.chromium.org/2310133002/diff/60001/services/ui/display/pla...
services/ui/display/platform_screen.h:22: // Creates a singleton PlatformScreen
instance that is owned by caller.
On 2016/09/08 18:03:35, sky wrote:
> 'caller' -> 'the caller'.
> 
> optional: that this returns a unique_ptr implies it's owned, so consider
nuking
> that part of the comment.

Done.

https://codereview.chromium.org/2310133002/diff/60001/services/ui/display/pla...
File services/ui/display/platform_screen_ozone.cc (right):

https://codereview.chromium.org/2310133002/diff/60001/services/ui/display/pla...
services/ui/display/platform_screen_ozone.cc:87: // TODO(kylechar): Implement.
On 2016/09/08 18:03:35, sky wrote:
> Use NOTIMPLEMENTED on all of these.

Done.

[dcheng, 2016-09-08 19:38:10]

This might be a dumb question, but why don't we just have a Display interface as
well, instead of plumbing around a display_id?

[kylechar, 2016-09-08 19:42:33]

On 2016/09/08 19:38:10, dcheng wrote:
> This might be a dumb question, but why don't we just have a Display interface
as
> well, instead of plumbing around a display_id?

We do have a mojom struct for display::Display actually and that is what clients
will know about via ScreenMus. I'm not sure you want to pass the whole struct
back to PlatformScreen though? We'd be throwing everything out except the
display id anyways.

[dcheng, 2016-09-08 19:58:43]

On 2016/09/08 19:42:33, kylechar wrote:
> On 2016/09/08 19:38:10, dcheng wrote:
> > This might be a dumb question, but why don't we just have a Display
interface
> as
> > well, instead of plumbing around a display_id?
> 
> We do have a mojom struct for display::Display actually and that is what
clients
> will know about via ScreenMus. I'm not sure you want to pass the whole struct
> back to PlatformScreen though? We'd be throwing everything out except the
> display id anyways.

My understanding is that one of the nice things about mojo is we don't have to
pass around IDs to identify objects: we can just pass the interface handles
directly. I don't think we should pass the current mojom struct, but maybe we
should have something separate as a Display interface, if we're going to have a
set of operations like this for a given display?

[kylechar, 2016-09-08 20:42:50]

Description was changed from

==========
Add mojom::DisplayController.

Add a new mojo interface mojom::DisplayController to allow privileged
clients to make changes to the display state. The initial interface
contains methods needed by mash to provide display state related
keyboard accelerators that exist in cash currently.

Have PlatformScreenOzone implement this new interface and provide mostly
stub implementations. Right now the interface isn't wired up so that
will happen in a subsequent CL.

Also adds PlatformScreen::GetInstance() so PlatformScreen doesn't need
to be passed so many places.

BUG=611475
==========

to

==========
Add mojom::DisplayController.

Add a new mojo interface mojom::DisplayController to allow privileged
clients to make changes to the display state. The initial interface
contains methods needed by mash to provide display state related
keyboard accelerators that exist in cash currently.

Have PlatformScreenOzone implement this new interface and provide mostly
stub implementations. Right now the interface isn't wired up so that
will happen in a subsequent CL.

Also adds PlatformScreen::GetInstance() so PlatformScreen doesn't need
to be passed so many places.

BUG=611475
==========

[dcheng, 2016-09-08 21:02:58]

Briefly talked offline. As background: with legacy IPC, we had no choice but to
plumb IDs and maintain a map of IDs to objects, with the subsequent issues of
making sure we always validate the ID. With Mojo, we should be aiming to
eliminate passing around explicit IDs by creating interfaces for objects that
were formerly identified by IDs: this lets us simplify a lot of the validation
and push it down into the Mojo layer, where it's safely and automatically
handled.

That being said, it seems like it's not practical to have a Display interface
atm, but it would be good if we could figure out a way to move to this model in
the future.

https://codereview.chromium.org/2310133002/diff/100001/services/ui/display/pl...
File services/ui/display/platform_screen_ozone.cc (right):

https://codereview.chromium.org/2310133002/diff/100001/services/ui/display/pl...
services/ui/display/platform_screen_ozone.cc:88: NOTIMPLEMENTED();
Would it be possible to just merge the implementation into this CL? Otherwise,
let's not add these methods to the mojo interface until we're ready to
implement.

[kylechar, 2016-09-08 21:20:53]

https://codereview.chromium.org/2310133002/diff/100001/services/ui/display/pl...
File services/ui/display/platform_screen_ozone.cc (right):

https://codereview.chromium.org/2310133002/diff/100001/services/ui/display/pl...
services/ui/display/platform_screen_ozone.cc:88: NOTIMPLEMENTED();
On 2016/09/08 21:02:57, dcheng wrote:
> Would it be possible to just merge the implementation into this CL? Otherwise,
> let's not add these methods to the mojo interface until we're ready to
> implement.

It's not possible to implement yet. Removed.

[dcheng, 2016-09-08 21:25:27]

mojom lgtm

[kylechar, 2016-09-08 21:28:30]

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

[commit-bot: I haz the power, 2016-09-08 21:29:02]

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-09-08 21:35:11]

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

[commit-bot: I haz the power, 2016-09-08 21:35:12]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  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...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rjkroege, 2016-09-08 22:34:58]

lgtm.

[kylechar, 2016-09-09 13:02:25]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2016-09-09 13:02:26]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2310133002/#ps140001
(title: "Add missing dep.")

[commit-bot: I haz the power, 2016-09-09 13:02: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-09-09 14:07:19]

Description was changed from

==========
Add mojom::DisplayController.

Add a new mojo interface mojom::DisplayController to allow privileged
clients to make changes to the display state. The initial interface
contains methods needed by mash to provide display state related
keyboard accelerators that exist in cash currently.

Have PlatformScreenOzone implement this new interface and provide mostly
stub implementations. Right now the interface isn't wired up so that
will happen in a subsequent CL.

Also adds PlatformScreen::GetInstance() so PlatformScreen doesn't need
to be passed so many places.

BUG=611475
==========

to

==========
Add mojom::DisplayController.

Add a new mojo interface mojom::DisplayController to allow privileged
clients to make changes to the display state. The initial interface
contains methods needed by mash to provide display state related
keyboard accelerators that exist in cash currently.

Have PlatformScreenOzone implement this new interface and provide mostly
stub implementations. Right now the interface isn't wired up so that
will happen in a subsequent CL.

Also adds PlatformScreen::GetInstance() so PlatformScreen doesn't need
to be passed so many places.

BUG=611475
==========

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

Committed patchset #6 (id:140001)

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

Description was changed from

==========
Add mojom::DisplayController.

Add a new mojo interface mojom::DisplayController to allow privileged
clients to make changes to the display state. The initial interface
contains methods needed by mash to provide display state related
keyboard accelerators that exist in cash currently.

Have PlatformScreenOzone implement this new interface and provide mostly
stub implementations. Right now the interface isn't wired up so that
will happen in a subsequent CL.

Also adds PlatformScreen::GetInstance() so PlatformScreen doesn't need
to be passed so many places.

BUG=611475
==========

to

==========
Add mojom::DisplayController.

Add a new mojo interface mojom::DisplayController to allow privileged
clients to make changes to the display state. The initial interface
contains methods needed by mash to provide display state related
keyboard accelerators that exist in cash currently.

Have PlatformScreenOzone implement this new interface and provide mostly
stub implementations. Right now the interface isn't wired up so that
will happen in a subsequent CL.

Also adds PlatformScreen::GetInstance() so PlatformScreen doesn't need
to be passed so many places.

BUG=611475

Committed: https://crrev.com/b39238a88385b5f0d7e93ff1195c9909e4d6c414
Cr-Commit-Position: refs/heads/master@{#417578}
==========

[commit-bot: I haz the power, 2016-09-09 14:09:19]

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