mash: Port sysui's WallpaperDelegateMus to mojo:ash.

mash: Port sysui's WallpaperDelegateMus to mojo:ash.

Move wallpaper_delegate_mus.* to ash/mus.
Vend mojom::WallpaperController from WindowManagerApplication.
Enable context menu command.

Workaround NOTIMPLEMENTED WmShellMus::GetDisplayInfo.
TODO: Fix crash on actually setting wallpaper via picker...

BUG=629605
TEST=chrome --mash has default gray wallpaper, can open picker.
R=sky@chromium.org

Committed: https://crrev.com/7d03b20662808183e75e52ad17ddf430a908d44f
Cr-Commit-Position: refs/heads/master@{#417774}

[msw, 2016-09-09 19:05:51]

Description was changed from

==========
mash: Port sysui's WallpaperDelegateMus to mojo:ash.

BUG=629605
==========

to

==========
mash: Port sysui's WallpaperDelegateMus to mojo:ash.

Move wallpaper_delegate_mus.* to ash/mus.
Vend mojom::WallpaperController from WindowManagerApplication.
Enable context menu command.

Add temporary stub WmShellMus::display_info_map_.
TODO: Get actual info from DisplayManager when available.

TODO: Fix crash on actually setting wallpaper...

BUG=629605
TEST=chrome --mash has default gray wallpaper, can open picker.
R=sky@chromium.org
==========

[msw, 2016-09-09 19:05:51]

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

[msw, 2016-09-09 19:07:26]

Description was changed from

==========
mash: Port sysui's WallpaperDelegateMus to mojo:ash.

Move wallpaper_delegate_mus.* to ash/mus.
Vend mojom::WallpaperController from WindowManagerApplication.
Enable context menu command.

Add temporary stub WmShellMus::display_info_map_.
TODO: Get actual info from DisplayManager when available.

TODO: Fix crash on actually setting wallpaper...

BUG=629605
TEST=chrome --mash has default gray wallpaper, can open picker.
R=sky@chromium.org
==========

to

==========
mash: Port sysui's WallpaperDelegateMus to mojo:ash.

Move wallpaper_delegate_mus.* to ash/mus.
Vend mojom::WallpaperController from WindowManagerApplication.
Enable context menu command.

Add temporary stub WmShellMus::display_info_map_.
TODO: Get actual info from DisplayManager when available.

TODO: Fix crash on actually setting wallpaper via picker...

BUG=629605
TEST=chrome --mash has default gray wallpaper, can open picker.
R=sky@chromium.org
==========

[msw, 2016-09-09 19:10:04]

Hey Scott, please take a look; thanks!
FYI Robert and Kyle on the wm_shell_mus.cc workaround/TODO.

https://codereview.chromium.org/2323063003/diff/40001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2323063003/diff/40001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:234: // Return a stub ManagedDisplayInfo object
created from a Display object.
Let me know if there's a better way to do this at the moment...

[msw, 2016-09-09 19:10:10]

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

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

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-09 19:15:07]

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

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

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kylechar, 2016-09-09 20:12:35]

https://codereview.chromium.org/2323063003/diff/40001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2323063003/diff/40001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:234: // Return a stub ManagedDisplayInfo object
created from a Display object.
On 2016/09/09 19:10:04, msw wrote:
> Let me know if there's a better way to do this at the moment...

Hmm, this is probably the wrong thing to do here (although let's see if rjkroege
agrees). ManagedDisplayInfo is very closely coupled to the internals of
ash::DisplayManager and the display state. In cash, other parts of ash just use
whatever is inside ash::DisplayManager.

For mustash, display::PlatformDisplay (which is conceptually the same as
ash::DisplayManager and the two will be merged soon) will be in the mus process.
So ash can't just access the internals without a Mojo interface. We also want a
better API that exposes the absolutely minimum about display state. I've started
a really basic mojom::DisplayController and will be adding more to it.

So I guess I'm wondering what information does ash need from ManagedDisplayInfo
that isn't in the display::Display already? Instead of using ManagedDisplayInfo,
I can add to mojom::DisplayController or possibly some other Mojo interface as
needed.

[rjkroege, 2016-09-09 21:08:58]

On 2016/09/09 20:12:35, kylechar wrote:
>
https://codereview.chromium.org/2323063003/diff/40001/ash/mus/bridge/wm_shell...
> File ash/mus/bridge/wm_shell_mus.cc (right):
> 
>
https://codereview.chromium.org/2323063003/diff/40001/ash/mus/bridge/wm_shell...
> ash/mus/bridge/wm_shell_mus.cc:234: // Return a stub ManagedDisplayInfo object
> created from a Display object.
> On 2016/09/09 19:10:04, msw wrote:
> > Let me know if there's a better way to do this at the moment...
> 
> Hmm, this is probably the wrong thing to do here (although let's see if
rjkroege
> agrees). ManagedDisplayInfo is very closely coupled to the internals of
> ash::DisplayManager and the display state. In cash, other parts of ash just
use
> whatever is inside ash::DisplayManager.

So, ash uses ManagedDisplayInfo all over. But it shouldn't. (Someday, nothing
outside
of ui/display/manager will see ManagedDisplayInfo.) So, I agree with kylechar@
that this is not the right approach. 

> 
> For mustash, display::PlatformDisplay (which is conceptually the same as
> ash::DisplayManager and the two will be merged soon) 

I think that kylechar@ means display::PlatformScreen here. Otherwise, yes.

> will be in the mus process.
> So ash can't just access the internals without a Mojo interface. We also want
a
> better API that exposes the absolutely minimum about display state. I've
started
> a really basic mojom::DisplayController and will be adding more to it.
> 
> So I guess I'm wondering what information does ash need from
ManagedDisplayInfo
> that isn't in the display::Display already? Instead of using
ManagedDisplayInfo,
> I can add to mojom::DisplayController or possibly some other Mojo interface as
> needed.

I agree with kylechar@ here. All ash mus code should to use some kind of Mojom
for
accessing display info. We need to add lots to the display mojom. What do you
need
here to make progress?

[msw, 2016-09-09 21:13:08]

Thanks, please see the new workarounds and my comment.
I should help you guys move info/callers to display::Display.

https://codereview.chromium.org/2323063003/diff/40001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2323063003/diff/40001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:234: // Return a stub ManagedDisplayInfo object
created from a Display object.
On 2016/09/09 20:12:35, kylechar wrote:
> On 2016/09/09 19:10:04, msw wrote:
> > Let me know if there's a better way to do this at the moment...
> 
> Hmm, this is probably the wrong thing to do here (although let's see if
rjkroege
> agrees). ManagedDisplayInfo is very closely coupled to the internals of
> ash::DisplayManager and the display state. In cash, other parts of ash just
use
> whatever is inside ash::DisplayManager.
> 
> For mustash, display::PlatformDisplay (which is conceptually the same as
> ash::DisplayManager and the two will be merged soon) will be in the mus
process.
> So ash can't just access the internals without a Mojo interface. We also want
a
> better API that exposes the absolutely minimum about display state. I've
started
> a really basic mojom::DisplayController and will be adding more to it.
> 
> So I guess I'm wondering what information does ash need from
ManagedDisplayInfo
> that isn't in the display::Display already? Instead of using
ManagedDisplayInfo,
> I can add to mojom::DisplayController or possibly some other Mojo interface as
> needed.

I uploaded an alternative workaround patch to appease the callers needed here:
- WallpaperController needs ManagedDisplayInfo::bounds_in_native().
- WallpaperView needs ManagedDisplayInfo::GetEffectiveUIScale().

The other active mus WmShell::GetDisplayInfo caller is:
- TrayAudio::ChangeInternalSpeakerChannelMode needs GetActiveRotation().

I don't know much offhand about the DisplayManager::GetDisplayInfo callers. It'd
be great if we could start using Display objects obtained from the
display::Screen instance, instead of ManagedDisplayInfo, but I'm not entirely
sure when that's possible (seems like Display::size() !=
ManagedDisplayInfo::bounds_in_native().size(), Display::device_scale_factor() !=
ManagedDisplayInfo::GetEffectiveUIScale(), perhaps Display::rotation() !=
ManagedDisplayInfo::GetRotation()/GetActiveRotation(), and I don't know if
Display does the right thing with callers that use InternalDisplayId, etc.).

[msw, 2016-09-09 21:13:18]

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

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

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

[msw, 2016-09-09 21:25:43]

Description was changed from

==========
mash: Port sysui's WallpaperDelegateMus to mojo:ash.

Move wallpaper_delegate_mus.* to ash/mus.
Vend mojom::WallpaperController from WindowManagerApplication.
Enable context menu command.

Add temporary stub WmShellMus::display_info_map_.
TODO: Get actual info from DisplayManager when available.

TODO: Fix crash on actually setting wallpaper via picker...

BUG=629605
TEST=chrome --mash has default gray wallpaper, can open picker.
R=sky@chromium.org
==========

to

==========
mash: Port sysui's WallpaperDelegateMus to mojo:ash.

Move wallpaper_delegate_mus.* to ash/mus.
Vend mojom::WallpaperController from WindowManagerApplication.
Enable context menu command.

Workaround NOTIMPLEMENTED WmShellMus::GetDisplayInfo.
TODO: Fix crash on actually setting wallpaper via picker...

BUG=629605
TEST=chrome --mash has default gray wallpaper, can open picker.
R=sky@chromium.org
==========

[sky, 2016-09-09 22:15:36]

On 2016/09/09 21:13:08, msw wrote:
> Thanks, please see the new workarounds and my comment.
> I should help you guys move info/callers to display::Display.
> 
>
https://codereview.chromium.org/2323063003/diff/40001/ash/mus/bridge/wm_shell...
> File ash/mus/bridge/wm_shell_mus.cc (right):
> 
>
https://codereview.chromium.org/2323063003/diff/40001/ash/mus/bridge/wm_shell...
> ash/mus/bridge/wm_shell_mus.cc:234: // Return a stub ManagedDisplayInfo object
> created from a Display object.
> On 2016/09/09 20:12:35, kylechar wrote:
> > On 2016/09/09 19:10:04, msw wrote:
> > > Let me know if there's a better way to do this at the moment...
> > 
> > Hmm, this is probably the wrong thing to do here (although let's see if
> rjkroege
> > agrees). ManagedDisplayInfo is very closely coupled to the internals of
> > ash::DisplayManager and the display state. In cash, other parts of ash just
> use
> > whatever is inside ash::DisplayManager.
> > 
> > For mustash, display::PlatformDisplay (which is conceptually the same as
> > ash::DisplayManager and the two will be merged soon) will be in the mus
> process.
> > So ash can't just access the internals without a Mojo interface. We also
want
> a
> > better API that exposes the absolutely minimum about display state. I've
> started
> > a really basic mojom::DisplayController and will be adding more to it.
> > 
> > So I guess I'm wondering what information does ash need from
> ManagedDisplayInfo
> > that isn't in the display::Display already? Instead of using
> ManagedDisplayInfo,
> > I can add to mojom::DisplayController or possibly some other Mojo interface
as
> > needed.
> 
> I uploaded an alternative workaround patch to appease the callers needed here:
> - WallpaperController needs ManagedDisplayInfo::bounds_in_native().
> - WallpaperView needs ManagedDisplayInfo::GetEffectiveUIScale().
> 
> The other active mus WmShell::GetDisplayInfo caller is:
> - TrayAudio::ChangeInternalSpeakerChannelMode needs GetActiveRotation().
> 
> I don't know much offhand about the DisplayManager::GetDisplayInfo callers.
It'd
> be great if we could start using Display objects obtained from the
> display::Screen instance, instead of ManagedDisplayInfo, but I'm not entirely
> sure when that's possible (seems like Display::size() !=
> ManagedDisplayInfo::bounds_in_native().size(), Display::device_scale_factor()
!=
> ManagedDisplayInfo::GetEffectiveUIScale(), perhaps Display::rotation() !=
> ManagedDisplayInfo::GetRotation()/GetActiveRotation(), and I don't know if
> Display does the right thing with callers that use InternalDisplayId, etc.).

We already have DisplayManagerObserver. It seems to me we should communicate the
extra state through that interface. Otherwise we have to use multiple
informations to get this information, which seems unfortunate.

[sky, 2016-09-09 22:17:01]

https://codereview.chromium.org/2323063003/diff/80001/ash/common/wallpaper/wa...
File ash/common/wallpaper/wallpaper_controller.cc (right):

https://codereview.chromium.org/2323063003/diff/80001/ash/common/wallpaper/wa...
ash/common/wallpaper/wallpaper_controller.cc:156: // TODO(mash): Mash returns a
fake ManagedDisplayInfo. crbug.com/622480
Is wallpaper the only place that needs ManagedDisplayInfo? If so, how about
providing methods for the specific data you need on WmShell rather than the more
generic ManagedDisplayInfo.

[commit-bot: I haz the power, 2016-09-09 22:37:26]

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

[commit-bot: I haz the power, 2016-09-09 22:37:27]

Dry run: This issue passed the CQ dry run.

[msw, 2016-09-09 23:09:51]

https://codereview.chromium.org/2323063003/diff/80001/ash/common/wallpaper/wa...
File ash/common/wallpaper/wallpaper_controller.cc (right):

https://codereview.chromium.org/2323063003/diff/80001/ash/common/wallpaper/wa...
ash/common/wallpaper/wallpaper_controller.cc:156: // TODO(mash): Mash returns a
fake ManagedDisplayInfo. crbug.com/622480
On 2016/09/09 22:17:01, sky wrote:
> Is wallpaper the only place that needs ManagedDisplayInfo? If so, how about
> providing methods for the specific data you need on WmShell rather than the
more
> generic ManagedDisplayInfo.

No, many places currently use ManagedDisplayInfo. It seems like the path forward
for these workarounds are probably to move ManagedDisplayInfo::bounds_in_native
to display::Display, and just use display::Display::rotation(). I'd like to
address those in followup patches, unrelated to WallpaperDelegateMus.

[sky, 2016-09-09 23:36:35]

LGTM

https://codereview.chromium.org/2323063003/diff/80001/ash/common/wallpaper/wa...
File ash/common/wallpaper/wallpaper_controller.cc (right):

https://codereview.chromium.org/2323063003/diff/80001/ash/common/wallpaper/wa...
ash/common/wallpaper/wallpaper_controller.cc:156: // TODO(mash): Mash returns a
fake ManagedDisplayInfo. crbug.com/622480
On 2016/09/09 23:09:51, msw wrote:
> On 2016/09/09 22:17:01, sky wrote:
> > Is wallpaper the only place that needs ManagedDisplayInfo? If so, how about
> > providing methods for the specific data you need on WmShell rather than the
> more
> > generic ManagedDisplayInfo.
> 
> No, many places currently use ManagedDisplayInfo. It seems like the path
forward
> for these workarounds are probably to move
ManagedDisplayInfo::bounds_in_native
> to display::Display, and just use display::Display::rotation(). I'd like to
> address those in followup patches, unrelated to WallpaperDelegateMus.

Ok, sounds reasonable to me to address fetching the ManagedDisplayInfo
separately. It may make sense to send it in the WindowManager interface if that
is the only place that needs it.

[msw, 2016-09-10 00:05:22]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2016-09-10 00:06:10]

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-10 00:14:01]

Description was changed from

==========
mash: Port sysui's WallpaperDelegateMus to mojo:ash.

Move wallpaper_delegate_mus.* to ash/mus.
Vend mojom::WallpaperController from WindowManagerApplication.
Enable context menu command.

Workaround NOTIMPLEMENTED WmShellMus::GetDisplayInfo.
TODO: Fix crash on actually setting wallpaper via picker...

BUG=629605
TEST=chrome --mash has default gray wallpaper, can open picker.
R=sky@chromium.org
==========

to

==========
mash: Port sysui's WallpaperDelegateMus to mojo:ash.

Move wallpaper_delegate_mus.* to ash/mus.
Vend mojom::WallpaperController from WindowManagerApplication.
Enable context menu command.

Workaround NOTIMPLEMENTED WmShellMus::GetDisplayInfo.
TODO: Fix crash on actually setting wallpaper via picker...

BUG=629605
TEST=chrome --mash has default gray wallpaper, can open picker.
R=sky@chromium.org
==========

[commit-bot: I haz the power, 2016-09-10 00:14:02]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-10 00:18:32]

Description was changed from

==========
mash: Port sysui's WallpaperDelegateMus to mojo:ash.

Move wallpaper_delegate_mus.* to ash/mus.
Vend mojom::WallpaperController from WindowManagerApplication.
Enable context menu command.

Workaround NOTIMPLEMENTED WmShellMus::GetDisplayInfo.
TODO: Fix crash on actually setting wallpaper via picker...

BUG=629605
TEST=chrome --mash has default gray wallpaper, can open picker.
R=sky@chromium.org
==========

to

==========
mash: Port sysui's WallpaperDelegateMus to mojo:ash.

Move wallpaper_delegate_mus.* to ash/mus.
Vend mojom::WallpaperController from WindowManagerApplication.
Enable context menu command.

Workaround NOTIMPLEMENTED WmShellMus::GetDisplayInfo.
TODO: Fix crash on actually setting wallpaper via picker...

BUG=629605
TEST=chrome --mash has default gray wallpaper, can open picker.
R=sky@chromium.org

Committed: https://crrev.com/7d03b20662808183e75e52ad17ddf430a908d44f
Cr-Commit-Position: refs/heads/master@{#417774}
==========

[commit-bot: I haz the power, 2016-09-10 00:18:33]

Patchset 5 (id:??) landed as
https://crrev.com/7d03b20662808183e75e52ad17ddf430a908d44f
Cr-Commit-Position: refs/heads/master@{#417774}