mus: Changes SetDisplayRoot() to create actual display

mus: Changes SetDisplayRoot() to create actual display

It now takes enough information for mus to create the display. Further
we now assume mus has not created the display at the time
SetDisplayRoot() is called.

BUG=708279
TEST=covered by tests
R=kylechar@chromium.org, nasko@chromium.org

Review-Url: https://codereview.chromium.org/2829733002
Cr-Commit-Position: refs/heads/master@{#466057}
Committed: https://chromium.googlesource.com/chromium/src/+/c53add46854c4bd7fa46a6f6515835f7292a2b8f

[sky, 2017-04-19 17:08:18]

Description was changed from

==========
mus: Changes SetDisplayRoot() to create actual display

It now takes enough information for mus to create the display. Further
we now assume mus has not created the display at the time
SetDisplayRoot() is called.

BUG=708279
TEST=covered by tests
R=kylechar@chromium.org, tsepez@chromium.org
==========

to

==========
mus: Changes SetDisplayRoot() to create actual display

It now takes enough information for mus to create the display. Further
we now assume mus has not created the display at the time
SetDisplayRoot() is called.

BUG=708279
TEST=covered by tests
R=kylechar@chromium.org, nasko@chromium.org
==========

[sky, 2017-04-19 17:08:18]

sky@chromium.org changed reviewers:
+ nasko@chromium.org
- tsepez@chromium.org

[sky, 2017-04-19 17:08:38]

nasko: mojom
kylechar: the rest

[sky, 2017-04-19 17:09:18]

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

[commit-bot: I haz the power, 2017-04-19 17:09:37]

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, 2017-04-19 17:24:47]

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

[commit-bot: I haz the power, 2017-04-19 17:24:48]

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

[sky, 2017-04-19 17:31:07]

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

[commit-bot: I haz the power, 2017-04-19 17:33:03]

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, 2017-04-19 17:54:00]

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

[commit-bot: I haz the power, 2017-04-19 17:54:01]

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...)

[kylechar, 2017-04-19 18:06:08]

https://codereview.chromium.org/2829733002/diff/40001/services/ui/public/inte...
File services/ui/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/window_manager.mojom:280:
SetDisplayRoot(display.mojom.Display display,
This gets called initially once for each display right? How do you expect
removing/modifying displays to work?

https://codereview.chromium.org/2829733002/diff/40001/services/ui/service.cc
File services/ui/service.cc (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/service.cc#...
services/ui/service.cc:269: void Service::OnWillCreateTreeForWindowManager(
What's the expected order that StartDisplayInit() vs
OnWillCreateTreeForWindowManager() getting called? They can happen in any order
it looks like?

https://codereview.chromium.org/2829733002/diff/40001/services/ui/service.h
File services/ui/service.h (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/service.h#n...
services/ui/service.h:221: std::unique_ptr<display::ScreenBase> screen_;
ScreenManagerOzoneInternal and ScreenManagerStubInternal both create a
ScreenBase instance already. I wonder if you could move this to
ScreenManagerForwarding and then add virtual ScreenManager::GetScreen()?
ScreenManager is already a singleton, so WindowServerDelegate::GetScreen()
wouldn't be necessary.

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/test_uti...
File services/ui/ws/test_utils.h (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/test_uti...
services/ui/ws/test_utils.h:604: std::unique_ptr<display::ScreenBase> screen_;
TestScreenManager also creates a ScreenBase so some tests will end up with two
of them I think? This isn't installing it as the singleton, so no crashes, but
it might be confusing.

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/window_m...
File services/ui/ws/window_manager_window_tree_factory.cc (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/window_m...
services/ui/ws/window_manager_window_tree_factory.cc:44:
->OnWillCreateTreeForWindowManager(automatically_create_display_roots);
Will this work with multiple connected WMs? I know we don't actually do that,
but I think some of the tests might pretend there are multiple WMs.

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/window_s...
File services/ui/ws/window_server_delegate.h (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/window_s...
services/ui/ws/window_server_delegate.h:21: }  // namespace
Fix namespace close comment.

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:2156: callback.Run(frame_sink_id);
Maybe named |empty_frame_sink_id| or just
callback.Run(base::Optional<cc::FrameSinkId>()) just so it's immediately obvious
what is happening here.

[sky, 2017-04-19 19:30:03]

https://codereview.chromium.org/2829733002/diff/40001/services/ui/public/inte...
File services/ui/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/window_manager.mojom:280:
SetDisplayRoot(display.mojom.Display display,
On 2017/04/19 18:06:08, kylechar wrote:
> This gets called initially once for each display right?

That's right.

> How do you expect
> removing/modifying displays to work?

Removing is implicitly handled when the root window is deleted.

To support modifying I'll need another function. I was hoping to get the add
working first. By which I mean a separate patch will add the modifying logic.

https://codereview.chromium.org/2829733002/diff/40001/services/ui/service.cc
File services/ui/service.cc (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/service.cc#...
services/ui/service.cc:269: void Service::OnWillCreateTreeForWindowManager(
On 2017/04/19 18:06:08, kylechar wrote:
> What's the expected order that StartDisplayInit() vs
> OnWillCreateTreeForWindowManager() getting called? They can happen in any
order
> it looks like?

You are correct. One is not guaranteed to be called before the other.

https://codereview.chromium.org/2829733002/diff/40001/services/ui/service.h
File services/ui/service.h (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/service.h#n...
services/ui/service.h:221: std::unique_ptr<display::ScreenBase> screen_;
On 2017/04/19 18:06:08, kylechar wrote:
> ScreenManagerOzoneInternal and ScreenManagerStubInternal both create a
> ScreenBase instance already. I wonder if you could move this to
> ScreenManagerForwarding and then add virtual ScreenManager::GetScreen()?
> ScreenManager is already a singleton, so WindowServerDelegate::GetScreen()
> wouldn't be necessary.

Done.

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/test_uti...
File services/ui/ws/test_utils.h (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/test_uti...
services/ui/ws/test_utils.h:604: std::unique_ptr<display::ScreenBase> screen_;
On 2017/04/19 18:06:08, kylechar wrote:
> TestScreenManager also creates a ScreenBase so some tests will end up with two
> of them I think? This isn't installing it as the singleton, so no crashes, but
> it might be confusing.

Removed.

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/window_m...
File services/ui/ws/window_manager_window_tree_factory.cc (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/window_m...
services/ui/ws/window_manager_window_tree_factory.cc:44:
->OnWillCreateTreeForWindowManager(automatically_create_display_roots);
On 2017/04/19 18:06:08, kylechar wrote:
> Will this work with multiple connected WMs? I know we don't actually do that,
> but I think some of the tests might pretend there are multiple WMs.

You are correct it would be problematic to have multiple wms connect with
different values. Service::OnWillCreateTreeForWindowManager() ignores the second
call. Will have to think what the right long term solution will be for multiple
wms. One easy solution is do away the boolean and only support the new config.

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/window_s...
File services/ui/ws/window_server_delegate.h (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/window_s...
services/ui/ws/window_server_delegate.h:21: }  // namespace
On 2017/04/19 18:06:08, kylechar wrote:
> Fix namespace close comment.

This is no longer needed.

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:2156: callback.Run(frame_sink_id);
On 2017/04/19 18:06:08, kylechar wrote:
> Maybe named |empty_frame_sink_id| or just
> callback.Run(base::Optional<cc::FrameSinkId>()) just so it's immediately
obvious
> what is happening here.

Done.

[sky, 2017-04-19 19:30:21]

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

[commit-bot: I haz the power, 2017-04-19 19:30:54]

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

[kylechar, 2017-04-19 19:47:09]

lgtm

https://codereview.chromium.org/2829733002/diff/40001/services/ui/public/inte...
File services/ui/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/window_manager.mojom:280:
SetDisplayRoot(display.mojom.Display display,
On 2017/04/19 19:30:02, sky wrote:
> On 2017/04/19 18:06:08, kylechar wrote:
> > This gets called initially once for each display right?
> 
> That's right.
> 
> > How do you expect
> > removing/modifying displays to work?
> 
> Removing is implicitly handled when the root window is deleted.
> 
> To support modifying I'll need another function. I was hoping to get the add
> working first. By which I mean a separate patch will add the modifying logic.

sgtm!

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/window_m...
File services/ui/ws/window_manager_window_tree_factory.cc (right):

https://codereview.chromium.org/2829733002/diff/40001/services/ui/ws/window_m...
services/ui/ws/window_manager_window_tree_factory.cc:44:
->OnWillCreateTreeForWindowManager(automatically_create_display_roots);
On 2017/04/19 19:30:03, sky wrote:
> On 2017/04/19 18:06:08, kylechar wrote:
> > Will this work with multiple connected WMs? I know we don't actually do
that,
> > but I think some of the tests might pretend there are multiple WMs.
> 
> You are correct it would be problematic to have multiple wms connect with
> different values. Service::OnWillCreateTreeForWindowManager() ignores the
second
> call. Will have to think what the right long term solution will be for
multiple
> wms. One easy solution is do away the boolean and only support the new config.

Ah, right it ignores it. Gotcha.

https://codereview.chromium.org/2829733002/diff/60001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2829733002/diff/60001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:2151: ProcessSetDisplayRoot(display,
*(viewport_metrics.get()),
optional nit: *viewport_metrics works instead of *(viewport_metrics.get()) I
think?

[commit-bot: I haz the power, 2017-04-19 19:47:22]

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

[commit-bot: I haz the power, 2017-04-19 19:47:23]

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, 2017-04-19 19:57:59]

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

[commit-bot: I haz the power, 2017-04-19 19:58:37]

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

[sky, 2017-04-19 20:39:15]

https://codereview.chromium.org/2829733002/diff/60001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2829733002/diff/60001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:2151: ProcessSetDisplayRoot(display,
*(viewport_metrics.get()),
On 2017/04/19 19:47:09, kylechar wrote:
> optional nit: *viewport_metrics works instead of *(viewport_metrics.get()) I
> think?

Done.

[sky, 2017-04-19 20:39:26]

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

[commit-bot: I haz the power, 2017-04-19 20:39:40]

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, 2017-04-19 21:41:04]

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

[commit-bot: I haz the power, 2017-04-19 21:41:05]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-04-20 17:36:05]

mojom LGTM

[sky, 2017-04-20 17:40:23]

The CQ bit was checked by sky@chromium.org

[sky, 2017-04-20 17:40:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
kylechar@chromium.org
Link to the patchset: https://codereview.chromium.org/2829733002/#ps100001
(title: "unnecessary get")

[commit-bot: I haz the power, 2017-04-20 17:40:55]

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, 2017-04-20 17:47:53]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492710023246150,
"parent_rev": "b00919686f1472b84a9f928aa22af4d47b0ffd5a", "commit_rev":
"c53add46854c4bd7fa46a6f6515835f7292a2b8f"}

[commit-bot: I haz the power, 2017-04-20 17:48:04]

Description was changed from

==========
mus: Changes SetDisplayRoot() to create actual display

It now takes enough information for mus to create the display. Further
we now assume mus has not created the display at the time
SetDisplayRoot() is called.

BUG=708279
TEST=covered by tests
R=kylechar@chromium.org, nasko@chromium.org
==========

to

==========
mus: Changes SetDisplayRoot() to create actual display

It now takes enough information for mus to create the display. Further
we now assume mus has not created the display at the time
SetDisplayRoot() is called.

BUG=708279
TEST=covered by tests
R=kylechar@chromium.org, nasko@chromium.org

Review-Url: https://codereview.chromium.org/2829733002
Cr-Commit-Position: refs/heads/master@{#466057}
Committed:
https://chromium.googlesource.com/chromium/src/+/c53add46854c4bd7fa46a6f65158...
==========

[commit-bot: I haz the power, 2017-04-20 17:48:06]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c53add46854c4bd7fa46a6f65158...