[WIP] ScreenManager: Add an external window mode

ScreenManager: Add an external window mode

An "external window mode" parameter is added to the ScreenManager init
in order to prepare [1]. In that mode, no initial displays are created
or initialized. This mode is used by the WindowServer when the window
manager is not connected to any WindowTree.

Based on the original work of Thomas Anderson in [2].

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=666958
[2] https://codereview.chromium.org/2503923003/

BUG=666958

[fwang, 2017-01-21 14:38:37]

The CQ bit was checked by fwang@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-21 14:38:46]

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

[fwang, 2017-01-21 14:47:56]

fwang@igalia.com changed reviewers:
+ sky@chromium.org, thomasanderson@google.com

[fwang, 2017-01-21 14:47:57]

PTAL

I'm extracting this part from https://codereview.chromium.org/2622103004/ in
order to simplify review. It is essentially the same as in Tom's original patch
except that PlatformScreen has been renamed ScreenManager.

[commit-bot: I haz the power, 2017-01-21 15:28:14]

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

[commit-bot: I haz the power, 2017-01-21 15:28:15]

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

[fwang, 2017-01-21 21:47:35]

Description was changed from

==========
ScreenManager: Add an external window mode

An "external window mode" parameter is added to the ScreenManager init
in order to prepare [1]. In that mode, no initial displays are created
or initialized. This mode is used by the WindowServer when the window
manager is not connected to any WindowTree.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=666958

BUG=666958
==========

to

==========
ScreenManager: Add an external window mode

An "external window mode" parameter is added to the ScreenManager init
in order to prepare [1]. In that mode, no initial displays are created
or initialized. This mode is used by the WindowServer when the window
manager is not connected to any WindowTree.

Based on the original work on Thomas Anderson <thomasanderson@google/chromium>
in [2].

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=666958
[2] https://codereview.chromium.org/2503923003/

BUG=666958
==========

[sky, 2017-01-23 16:16:14]

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

[sky, 2017-01-23 16:16:20]

+kylechar

[rjkroege, 2017-01-23 21:19:42]

rjkroege@chromium.org changed reviewers:
+ rjkroege@chromium.org

[rjkroege, 2017-01-23 21:19:44]

https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
File services/ui/display/screen_manager.h (right):

https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager.h:24: WINDOW_MODE_EXTERNAL,
We've emailed enough to know what these mean but I think it needs a comment.

https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager.h:43: // displays are added, removed or
modified.
Does external mode need a delegate? I don't remember. The comments should say.

https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
File services/ui/display/screen_manager_ozone_unittests.cc (right):

https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_ozone_unittests.cc:171:
screen_manager_->Init(&delegate_, ScreenManager::WINDOW_MODE_INTERNAL);
This is not right in some fashion. One of the goals in this work is to use the
ozone wayland backend to support EXTERNAL window mode yes? So: the test should
be able to run with both INTERNAL and EXTERNAL? Right?

At a minimum: this needs a TODO() to say that the test should made to run with
external mode too?

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

https://codereview.chromium.org/2645093003/diff/1/services/ui/service.cc#newc...
services/ui/service.cc:234:
screen_manager_->Init(window_server_->display_manager(),
I have a mild preference for using if here with the expectation that this might
become more involved.

https://codereview.chromium.org/2645093003/diff/1/services/ui/ws/window_serve...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2645093003/diff/1/services/ui/ws/window_serve...
services/ui/ws/window_server.cc:767: bool window_manager_connected = false;
Are you sure that this does the right thing in INTERNAL mode on Gpu service
restart?

[fwang, 2017-01-24 13:12:00]

Description was changed from

==========
ScreenManager: Add an external window mode

An "external window mode" parameter is added to the ScreenManager init
in order to prepare [1]. In that mode, no initial displays are created
or initialized. This mode is used by the WindowServer when the window
manager is not connected to any WindowTree.

Based on the original work on Thomas Anderson <thomasanderson@google/chromium>
in [2].

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=666958
[2] https://codereview.chromium.org/2503923003/

BUG=666958
==========

to

==========
ScreenManager: Add an external window mode

An "external window mode" parameter is added to the ScreenManager init
in order to prepare [1]. In that mode, no initial displays are created
or initialized. This mode is used by the WindowServer when the window
manager is not connected to any WindowTree.

Based on the original work of Thomas Anderson in [2].

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=666958
[2] https://codereview.chromium.org/2503923003/

BUG=666958
==========

[fwang, 2017-01-24 17:11:51]

https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
File services/ui/display/screen_manager.h (right):

https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager.h:24: WINDOW_MODE_EXTERNAL,
On 2017/01/23 21:19:43, rjkroege wrote:
> We've emailed enough to know what these mean but I think it needs a comment.

Done.

https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager.h:43: // displays are added, removed or
modified.
On 2017/01/23 21:19:43, rjkroege wrote:
> Does external mode need a delegate? I don't remember. The comments should say.

Mmh, code in screen manager seems to assume the delegate_ point is non-null so I
would say yes.

https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
File services/ui/display/screen_manager_ozone_unittests.cc (right):

https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_ozone_unittests.cc:171:
screen_manager_->Init(&delegate_, ScreenManager::WINDOW_MODE_INTERNAL);
On 2017/01/23 21:19:43, rjkroege wrote:
> This is not right in some fashion. One of the goals in this work is to use the
> ozone wayland backend to support EXTERNAL window mode yes? So: the test should
> be able to run with both INTERNAL and EXTERNAL? Right?
> 
> At a minimum: this needs a TODO() to say that the test should made to run with
> external mode too?

I'll check that (BTW, note that this test is only executed on ChromeOS at the
moment and many other tests listed in this commit actually fails on non-ChromeOS
platforms).

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

https://codereview.chromium.org/2645093003/diff/1/services/ui/service.cc#newc...
services/ui/service.cc:234:
screen_manager_->Init(window_server_->display_manager(),
On 2017/01/23 21:19:43, rjkroege wrote:
> I have a mild preference for using if here with the expectation that this
might
> become more involved.

Done.

https://codereview.chromium.org/2645093003/diff/1/services/ui/ws/window_serve...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2645093003/diff/1/services/ui/ws/window_serve...
services/ui/ws/window_server.cc:767: bool window_manager_connected = false;
On 2017/01/23 21:19:43, rjkroege wrote:
> Are you sure that this does the right thing in INTERNAL mode on Gpu service
> restart?

Good point. As I see this commit does not change the fact that StartDisplayInit
may be called multiple times when Gpu is re-inited. However, maybe you mean that
the value of window_manager_connected may change when Gpu is re-inited?

[rjkroege, 2017-01-24 19:29:31]

On 2017/01/24 17:11:51, fwang wrote:
>
https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
> File services/ui/display/screen_manager.h (right):
> 
>
https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
> services/ui/display/screen_manager.h:24: WINDOW_MODE_EXTERNAL,
> On 2017/01/23 21:19:43, rjkroege wrote:
> > We've emailed enough to know what these mean but I think it needs a comment.
> 
> Done.
> 
>
https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
> services/ui/display/screen_manager.h:43: // displays are added, removed or
> modified.
> On 2017/01/23 21:19:43, rjkroege wrote:
> > Does external mode need a delegate? I don't remember. The comments should
say.
> 
> Mmh, code in screen manager seems to assume the delegate_ point is non-null so
I
> would say yes.
> 
>
https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
> File services/ui/display/screen_manager_ozone_unittests.cc (right):
> 
>
https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
> services/ui/display/screen_manager_ozone_unittests.cc:171:
> screen_manager_->Init(&delegate_, ScreenManager::WINDOW_MODE_INTERNAL);
> On 2017/01/23 21:19:43, rjkroege wrote:
> > This is not right in some fashion. One of the goals in this work is to use
the
> > ozone wayland backend to support EXTERNAL window mode yes? So: the test
should
> > be able to run with both INTERNAL and EXTERNAL? Right?
> > 
> > At a minimum: this needs a TODO() to say that the test should made to run
with
> > external mode too?
> 
> I'll check that (BTW, note that this test is only executed on ChromeOS at the
> moment and many other tests listed in this commit actually fails on
non-ChromeOS
> platforms).
> 
> https://codereview.chromium.org/2645093003/diff/1/services/ui/service.cc
> File services/ui/service.cc (right):
> 
>
https://codereview.chromium.org/2645093003/diff/1/services/ui/service.cc#newc...
> services/ui/service.cc:234:
> screen_manager_->Init(window_server_->display_manager(),
> On 2017/01/23 21:19:43, rjkroege wrote:
> > I have a mild preference for using if here with the expectation that this
> might
> > become more involved.
> 
> Done.
> 
>
https://codereview.chromium.org/2645093003/diff/1/services/ui/ws/window_serve...
> File services/ui/ws/window_server.cc (right):
> 
>
https://codereview.chromium.org/2645093003/diff/1/services/ui/ws/window_serve...
> services/ui/ws/window_server.cc:767: bool window_manager_connected = false;
> On 2017/01/23 21:19:43, rjkroege wrote:
> > Are you sure that this does the right thing in INTERNAL mode on Gpu service
> > restart?
> 
> Good point. As I see this commit does not change the fact that
StartDisplayInit
> may be called multiple times when Gpu is re-inited. However, maybe you mean
that
> the value of window_manager_connected may change when Gpu is re-inited?

Yes. I mean the latter. And that would be bad.

[kylechar, 2017-01-25 13:04:48]

https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
File services/ui/display/screen_manager_ozone.h (right):

https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_ozone.h:52: void RequestCloseDisplay(int64_t
display_id) override;
I don't think this is the right thing to do. ScreenManagerOzone is perhaps
poorly named and it should be ScreenManagerChromeOS or ScreenManagerInternal but
it's built for Chrome OS. It will only work for internal window mode and it
controls the size of root drawable surfaces. For external window mode
ScreenManager doesn't control the root drawable surfaces, so there isn't much /
any code overlap.

ScreenManagerOzone won't compile if target_os != "chromeos". I think you'll need
to write a different ScreenManager implementation for external window mode or
maybe you can use ScreenManagerStub for now. 

The bulk of the work to get external window mode working is the WS. Right now
each ws::Display corresponds to a root drawable surface. The ws::Display has a
ws::PlatformDisplay (which again isn't well named) which is basically what
allows you to draw to a surface, it owns the ui::PlatformWindow and
ws::FrameGenerator. The WS needs to be changed so that ws::Display doesn't own a
ws::PlatformDisplay. The top level Chrome ServerWindows need to own
ws::PlatformDisplay, which I guess means that a ServerWindow owns the
ws::PlatformDisplay in both external and internal window mode.

[kylechar, 2017-01-25 13:08:49]

What platform are you trying to target for external window mode first? Is it
something like use_ozone=true and target_os="linux"?

[rjkroege, 2017-01-25 19:33:47]

On 2017/01/25 13:08:49, kylechar wrote:
> What platform are you trying to target for external window mode first? Is it
> something like use_ozone=true and target_os="linux"?

That is my understanding of the intent. In this world, "oxygen" external window
mode would replace our existing linux build and we'd have Chrome for wayland.

[fwang, 2017-02-07 16:41:04]

On 2017/01/25 13:04:48, kylechar wrote:
>
https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
> File services/ui/display/screen_manager_ozone.h (right):
> 
>
https://codereview.chromium.org/2645093003/diff/1/services/ui/display/screen_...
> services/ui/display/screen_manager_ozone.h:52: void
RequestCloseDisplay(int64_t
> display_id) override;
> I don't think this is the right thing to do. ScreenManagerOzone is perhaps
> poorly named and it should be ScreenManagerChromeOS or ScreenManagerInternal
but
> it's built for Chrome OS. It will only work for internal window mode and it
> controls the size of root drawable surfaces. For external window mode
> ScreenManager doesn't control the root drawable surfaces, so there isn't much
/
> any code overlap.

Indeed. This CL was based on Tom's approach for external windows which were
initially intended to be tested on Chrome OS. But after post-BlinkOn discussion
with Antionio, it seems we directly want to try external windows on LinuxOS

> 
> ScreenManagerOzone won't compile if target_os != "chromeos". I think you'll
need
> to write a different ScreenManager implementation for external window mode or
> maybe you can use ScreenManagerStub for now. 
> 

Indeed. Ozone LinuxOS currently uses ScreenManagerStub actually. Antonio has a
CL pending to be uploaded that renames ScreenManagerOzone.

> The bulk of the work to get external window mode working is the WS. Right now
> each ws::Display corresponds to a root drawable surface. The ws::Display has a
> ws::PlatformDisplay (which again isn't well named) which is basically what
> allows you to draw to a surface, it owns the ui::PlatformWindow and
> ws::FrameGenerator. The WS needs to be changed so that ws::Display doesn't own
a
> ws::PlatformDisplay. The top level Chrome ServerWindows need to own
> ws::PlatformDisplay, which I guess means that a ServerWindow owns the
> ws::PlatformDisplay in both external and internal window mode.

I have opened a separate CL to do that:
https://codereview.chromium.org/2684623002/. Not sure it is really enough but at
least it allows to have something to continue the discussion. Please take a
look!

[fwang, 2017-02-07 16:42:58]

On 2017/01/25 19:33:47, rjkroege wrote:
> On 2017/01/25 13:08:49, kylechar wrote:
> > What platform are you trying to target for external window mode first? Is it
> > something like use_ozone=true and target_os="linux"?
> 
> That is my understanding of the intent. In this world, "oxygen" external
window
> mode would replace our existing linux build and we'd have Chrome for wayland.

Yes our goal is Ozone/LinuxOS although before BlinkOn it was not clear whether
we wanted to experiment external mode with ChromeOS first. As I said, now it
seems we directly want to do Ozone/LinuxOS.

[fwang, 2017-02-16 08:33:44]

This commit is obsolete since we decided to not support external mode in
ChromeOS. Antonio implemented a screen manager without display init for
non-ChromeOS ozone in https://codereview.chromium.org/2697693002/