Demonstrate external-window-mode in mus-demo

Demonstrate external-window-mode in mus-demo

This CL
* Adds a new command line switch to enable external window mode
* Modifies mus-demo to use the new flag to create 2 toplevel windows

BUG=666958

[Tom (Use chromium acct), 2016-11-17 19:47:44]

Patchset #2 (id:20001) has been deleted

[Tom (Use chromium acct), 2016-11-17 20:11:16]

Patchset #2 (id:40001) has been deleted

[Tom (Use chromium acct), 2016-11-18 17:25:52]

Patchset #1 (id:1) has been deleted

[Tom (Use chromium acct), 2016-11-18 17:26:17]

Description was changed from

==========
[WIP] [DO NOT LAND] Make mus-demo draw 2 spinning squares

BUG=
==========

to

==========
[WIP] [DO NOT LAND] Make mus-demo draw 2 spinning squares


BUG=
==========

[Tom (Use chromium acct), 2016-11-18 20:25:43]

Patchset #3 (id:100001) has been deleted

[Tom (Use chromium acct), 2016-11-18 20:27:27]

Description was changed from

==========
[WIP] [DO NOT LAND] Make mus-demo draw 2 spinning squares


BUG=
==========

to

==========
Demonstrate external-window-mode in mus-demo


BUG=
==========

[Tom (Use chromium acct), 2016-11-18 20:27:27]

thomasanderson@google.com changed reviewers:
+ rjkroege@chromium.org

[Tom (Use chromium acct), 2016-11-18 20:27:56]

rjkroege@ PTAL

https://codereview.chromium.org/2503923003/diff/120001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.cc (right):

https://codereview.chromium.org/2503923003/diff/120001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.cc:106: AddWindowTree();
how do you propose we fix the chromebook case?  Just don't add the second window
tree if OS_CHROMEOS?

[rjkroege, 2016-11-18 22:47:14]

I think that MusDemo still to register a WindowManagerDelegate on CrOS or else
it won't paint. Maybe there could be a command line flag that enables external
window mode in the demo and then mus would take appropriate action?

https://codereview.chromium.org/2503923003/diff/120001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.cc (left):

https://codereview.chromium.org/2503923003/diff/120001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.cc:124: void MusDemo::OnWmNewDisplay(Window* window,
const display::Display& display) {
afaik, without an implementation of this method, the demo won't paint on CrOS

https://codereview.chromium.org/2503923003/diff/120001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.cc (right):

https://codereview.chromium.org/2503923003/diff/120001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.cc:106: AddWindowTree();
On 2016/11/18 20:27:56, Tom Anderson wrote:
> how do you propose we fix the chromebook case?  Just don't add the second
window
> tree if OS_CHROMEOS?

my thought was via command line flag.

https://codereview.chromium.org/2503923003/diff/120001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.h (left):

https://codereview.chromium.org/2503923003/diff/120001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.h:34: public WindowManagerDelegate {
I think this is going to break the CrOS world.

I think the MusDemo could be modified the way that you changed it. But you will
need a WindowManagerDelegate implementation on CrOS.

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

https://codereview.chromium.org/2503923003/diff/120001/services/ui/display/pl...
services/ui/display/platform_screen_ozone.cc:83: // Don't set up a default
display on non-ChromeOS.
I think that this will break the oxygen build of CrOS mustash. The code in mus
that creates a PlatformScreen needs to configure this appropriately based on
whether or not there is a window manager or not?

[Tom (Use chromium acct), 2016-11-18 23:27:12]

Description was changed from

==========
Demonstrate external-window-mode in mus-demo


BUG=
==========

to

==========
Demonstrate external-window-mode in mus-demo

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

[Tom (Use chromium acct), 2016-11-19 00:47:03]

Description was changed from

==========
Demonstrate external-window-mode in mus-demo

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

to

==========
Demonstrate external-window-mode in mus-demo

This CL
* Adds a new command line switch to enable external window mode
* Modifies mus-demo to use the new flag to create 2 toplevel windows

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

[Tom (Use chromium acct), 2016-11-19 00:48:04]

rjkroege@ PTAL.  I think the latest PS should address all of your concerns about
CrOS

https://codereview.chromium.org/2503923003/diff/140001/ui/ozone/public/ozone_...
File ui/ozone/public/ozone_switches.h (right):

https://codereview.chromium.org/2503923003/diff/140001/ui/ozone/public/ozone_...
ui/ozone/public/ozone_switches.h:17: OZONE_BASE_EXPORT extern const char
kExternalWindowMode[];
Not sure if this is the right place to add this

[tonikitoo, 2016-11-21 16:44:16]

tonikitoo@igalia.com changed reviewers:
+ tonikitoo@igalia.com

[tonikitoo, 2016-11-21 16:44:17]

looking good. One comment:

https://codereview.chromium.org/2503923003/diff/140001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.cc (right):

https://codereview.chromium.org/2503923003/diff/140001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.cc:98: :
external_window_mode_(base::CommandLine::ForCurrentProcess()->HasSwitch(
I see an issue with calling CommandLine::ForCurrentProcess() here:

the call stack of the MusDemo ctor is:

#3 MusDemo::MusDemo
#4 ServiceMain
#5 service_manager::RunServiceInNativeLibrary()
#6 service_manager::(anonymous namespace)::RunNativeLibrary()

see services/ui/demo/main.cc

*however*

CommandLine is only init'ed for this process down the road on the same call

#1 base::CommandLine::Init()
#2 service_manager::ServiceRunner::Run()
#3 service_manager::ServiceRunner::Run()
#4 ServiceMain
#5 service_manager::RunServiceInNativeLibrary()
#6 service_manager::(anonymous namespace)::RunNativeLibrary()

And only after this point, base::CommandLine::ForCurrentProcess returns a valid
instance.

So here you are going to either crash or hit an assert.

I suggest moving the external_window_mode_ initialization when MusDemo::OnStart
is called.

[tonikitoo, 2016-11-21 17:09:57]

additionally, on a regular chrome_os off-device build, I get this:

$ time ninja -C out/chrome_os/  chrome mash:all mash/browser services/ui/demo
ninja: Entering directory `out/chrome_os/'
[992/12278] SOLINK ./libmus_demo_library.so
FAILED: libmus_demo_library.so libmus_demo_library.so.TOC 
python "<path>/src/build/toolchain/gcc_solink_wrapper.py" --readelf="readelf"
--nm="nm"  --sofile="./libmus_demo_library.so"
--tocfile="./libmus_demo_library.so.TOC" --output="./libmus_demo_library.so"  --
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared
-Wl,--fatal-warnings -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro
-Wl,-z,defs -Wl,--no-as-needed -lpthread -Wl,--as-needed -fuse-ld=gold
-B../../third_party/binutils/Linux_x64/Release/bin -Wl,--threads
-Wl,--thread-count=4 -Wl,--icf=all -pthread -m64 -Wl,-O1 -Wl,--gc-sections
-Wl,--export-dynamic -o "./libmus_demo_library.so"
-Wl,-soname="libmus_demo_library.so" @"./libmus_demo_library.so.rsp"
obj/services/ui/demo/lib/mus_demo.o:../../services/ui/demo/mus_demo.cc:function
ui::demo::MusDemo::MusDemo(): error: undefined reference to
'switches::kExternalWindowMode'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
[1033/12278] CXX obj/content/common/mojo_bindings_cpp_sources/indexed_db.mojom.o
ninja: build stopped: subcommand failed.


GN args:

  is_debug = false

  target_os = "chromeos"
  enable_package_mash_services = true

  use_sysroot = false

  enable_nacl = false
  remove_webcore_debug_symbols = true

[Tom (Use chromium acct), 2016-11-21 19:24:33]

On 2016/11/21 17:09:57, tonikitoo wrote:
> additionally, on a regular chrome_os off-device build, I get this:
> 
> $ time ninja -C out/chrome_os/  chrome mash:all mash/browser services/ui/demo
> ninja: Entering directory `out/chrome_os/'
> [992/12278] SOLINK ./libmus_demo_library.so
> FAILED: libmus_demo_library.so libmus_demo_library.so.TOC 
> python "<path>/src/build/toolchain/gcc_solink_wrapper.py" --readelf="readelf"
> --nm="nm"  --sofile="./libmus_demo_library.so"
> --tocfile="./libmus_demo_library.so.TOC" --output="./libmus_demo_library.so" 
--
> ../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared
> -Wl,--fatal-warnings -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro
> -Wl,-z,defs -Wl,--no-as-needed -lpthread -Wl,--as-needed -fuse-ld=gold
> -B../../third_party/binutils/Linux_x64/Release/bin -Wl,--threads
> -Wl,--thread-count=4 -Wl,--icf=all -pthread -m64 -Wl,-O1 -Wl,--gc-sections
> -Wl,--export-dynamic -o "./libmus_demo_library.so"
> -Wl,-soname="libmus_demo_library.so" @"./libmus_demo_library.so.rsp"
>
obj/services/ui/demo/lib/mus_demo.o:../../services/ui/demo/mus_demo.cc:function
> ui::demo::MusDemo::MusDemo(): error: undefined reference to
> 'switches::kExternalWindowMode'
> clang: error: linker command failed with exit code 1 (use -v to see
invocation)
> [1033/12278] CXX
obj/content/common/mojo_bindings_cpp_sources/indexed_db.mojom.o
> ninja: build stopped: subcommand failed.
> 
> 
> GN args:
> 
>   is_debug = false
> 
>   target_os = "chromeos"
>   enable_package_mash_services = true
> 
>   use_sysroot = false
> 
>   enable_nacl = false
>   remove_webcore_debug_symbols = true

Good catch! When I try adding //ui/ozone:ozone_base to //ui/ozone/BUILD.gn, I
get an error at assert(use_ozone), so before I invest time in splitting off the
switches into a separate component, I want to hear from rjkroege@ if this is
even the right place for the new switch.

https://codereview.chromium.org/2503923003/diff/140001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.cc (right):

https://codereview.chromium.org/2503923003/diff/140001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.cc:98: :
external_window_mode_(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2016/11/21 16:44:16, tonikitoo wrote:
> I see an issue with calling CommandLine::ForCurrentProcess() here:
> 
> the call stack of the MusDemo ctor is:
> 
> #3 MusDemo::MusDemo
> #4 ServiceMain
> #5 service_manager::RunServiceInNativeLibrary()
> #6 service_manager::(anonymous namespace)::RunNativeLibrary()
> 
> see services/ui/demo/main.cc
> 
> *however*
> 
> CommandLine is only init'ed for this process down the road on the same call
> 
> #1 base::CommandLine::Init()
> #2 service_manager::ServiceRunner::Run()
> #3 service_manager::ServiceRunner::Run()
> #4 ServiceMain
> #5 service_manager::RunServiceInNativeLibrary()
> #6 service_manager::(anonymous namespace)::RunNativeLibrary()
> 
> And only after this point, base::CommandLine::ForCurrentProcess returns a
valid
> instance.
> 
> So here you are going to either crash or hit an assert.
> 
> I suggest moving the external_window_mode_ initialization when
MusDemo::OnStart
> is called. 

CommandLine::ForCurrentProcess() has a DCHECK to make sure it has a valid
instance, and it's not DCHECKing, so I don't know where it's getting initialized
from, but I moved it to OnStart out of an abundance of caution

[rjkroege, 2016-11-21 21:19:11]

I would expect the external window version to use CreateWindowTree in
WindowTreeFactory while the internal window version would use CreateWindowTree
in WindowManagerWindowTreeFactory (at least from the viewpoint of mojo calls.)

Perhaps WindowServer::OnGpuChannelEstablished should tell
WindowServerDelegate::StartDisplayInit() if it has/doesn't has a window manager.
 window_manager_window_tree_factory_set() knows if there is a window manager.
That way, the PlatformScreen will know how to handle the screen creation for
internal/external window manager operation where the expectation in external
window mode is that some (obviously :-) external window system had addressed the
issue?

https://codereview.chromium.org/2503923003/diff/160001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.cc (right):

https://codereview.chromium.org/2503923003/diff/160001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.cc:187: CreateWindowTreeHost(context()->connector(),
this, &data->host, nullptr);
This seems odd to me. Why doesn't this build a WindowTreeClient instance and use
ConnectViaWindowTreeFactory to connect?

https://codereview.chromium.org/2503923003/diff/160001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.cc:200: data->window = window;
someday data should be a class probably.

this is not refactored the way that I would do it. This is ignorable
bike-shedding.

https://codereview.chromium.org/2503923003/diff/160001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.h (right):

https://codereview.chromium.org/2503923003/diff/160001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.h:91: std::vector<std::unique_ptr<WindowTreeData>>
window_tree_datas_;
one for each top-level window?

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

https://codereview.chromium.org/2503923003/diff/160001/services/ui/display/pl...
services/ui/display/platform_screen_ozone.cc:86: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
This doesn't seem like the right approach. The code invoking Init on the
PlatformScreen should say if it wants a configuration for internal or external
window mode. And it should know this based on how the WindowTree is built yes?
i.e. via WindowTreeFactory of WindowManagerWindowTreeFactory?

[Tom (Use chromium acct), 2016-11-22 02:48:15]

Note that there is still a linker issue related to ozone_switches


On 2016/11/21 21:19:11, rjkroege wrote:
> I would expect the external window version to use CreateWindowTree in
> WindowTreeFactory while the internal window version would use CreateWindowTree
> in WindowManagerWindowTreeFactory (at least from the viewpoint of mojo calls.)
> 

CreateWindowTree isn't necessary when creating a new host

> Perhaps WindowServer::OnGpuChannelEstablished should tell
> WindowServerDelegate::StartDisplayInit() if it has/doesn't has a window
manager.
>  window_manager_window_tree_factory_set() knows if there is a window manager.
> That way, the PlatformScreen will know how to handle the screen creation for
> internal/external window manager operation where the expectation in external
> window mode is that some (obviously :-) external window system had addressed
the
> issue?
> 

Done.

https://codereview.chromium.org/2503923003/diff/160001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.cc (right):

https://codereview.chromium.org/2503923003/diff/160001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.cc:187: CreateWindowTreeHost(context()->connector(),
this, &data->host, nullptr);
On 2016/11/21 21:19:11, rjkroege wrote:
> This seems odd to me. Why doesn't this build a WindowTreeClient instance and
use
> ConnectViaWindowTreeFactory to connect?

CreateWindowTreeHost gives a WindowTreeClient (which I guess is already set up),
unlike the WM use case where we first create a WindowTreeClient and then use it
to ConnectAsWindowManager.

The only thing I'm using right now for the external case is OnEmbed to get the
root window.

https://codereview.chromium.org/2503923003/diff/160001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.cc:200: data->window = window;
On 2016/11/21 21:19:11, rjkroege wrote:
> someday data should be a class probably.
> 
> this is not refactored the way that I would do it. This is ignorable
> bike-shedding.

Acknowledged.

https://codereview.chromium.org/2503923003/diff/160001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.h (right):

https://codereview.chromium.org/2503923003/diff/160001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.h:91: std::vector<std::unique_ptr<WindowTreeData>>
window_tree_datas_;
On 2016/11/21 21:19:11, rjkroege wrote:
> one for each top-level window?

yes, added comment

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

https://codereview.chromium.org/2503923003/diff/160001/services/ui/display/pl...
services/ui/display/platform_screen_ozone.cc:86: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2016/11/21 21:19:11, rjkroege wrote:
> This doesn't seem like the right approach. The code invoking Init on the
> PlatformScreen should say if it wants a configuration for internal or external
> window mode. 

Done.

> And it should know this based on how the WindowTree is built yes?
> i.e. via WindowTreeFactory of WindowManagerWindowTreeFactory?

[tonikitoo, 2016-11-22 03:07:39]

Some (driven by) minor comments.

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

https://codereview.chromium.org/2503923003/diff/180001/services/ui/service.cc...
services/ui/service.cc:232: !window_manager_connected);
nit: in order to self-document the parameter, maybe we could add a local named
variable:

  bool external_window_mode = !window_manager_connected;
  platform_screen_->Init(window_server_->display_manager(),
                         external_window_mode);

https://codereview.chromium.org/2503923003/diff/180001/services/ui/ws/test_ut...
File services/ui/ws/test_utils.h (right):

https://codereview.chromium.org/2503923003/diff/180001/services/ui/ws/test_ut...
services/ui/ws/test_utils.h:70: bool external_window_mode_) override;
nit: trailing "_"

https://codereview.chromium.org/2503923003/diff/180001/services/ui/ws/window_...
File services/ui/ws/window_tree_unittest.cc (right):

https://codereview.chromium.org/2503923003/diff/180001/services/ui/ws/window_...
services/ui/ws/window_tree_unittest.cc:1360:
platform_screen.Init(window_server->display_manager(), false);
for good code reading purposes, a named variable for this boolean or an enum
would be nice.

But it would require replication in varios places, so maybe you added the
boolean literal on purpose.

[Tom (Use chromium acct), 2016-11-22 18:56:09]

Patchset #7 (id:200001) has been deleted

[Tom (Use chromium acct), 2016-11-22 19:19:48]

I reverted the changes is ozone_switches because the "external-window-mode" flag
is really a sister flag to "mash".  The mash flag is just hardcoded
https://cs.chromium.org/chromium/src/chrome/app/chrome_main.cc?rcl=0&l=103
So, I've done the same with the new flag

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

https://codereview.chromium.org/2503923003/diff/180001/services/ui/service.cc...
services/ui/service.cc:232: !window_manager_connected);
On 2016/11/22 03:07:39, tonikitoo wrote:
> nit: in order to self-document the parameter, maybe we could add a local named
> variable:
> 
>   bool external_window_mode = !window_manager_connected;
>   platform_screen_->Init(window_server_->display_manager(),
>                          external_window_mode);

Done.

https://codereview.chromium.org/2503923003/diff/180001/services/ui/ws/test_ut...
File services/ui/ws/test_utils.h (right):

https://codereview.chromium.org/2503923003/diff/180001/services/ui/ws/test_ut...
services/ui/ws/test_utils.h:70: bool external_window_mode_) override;
On 2016/11/22 03:07:39, tonikitoo wrote:
> nit: trailing "_"

Done.

https://codereview.chromium.org/2503923003/diff/180001/services/ui/ws/window_...
File services/ui/ws/window_tree_unittest.cc (right):

https://codereview.chromium.org/2503923003/diff/180001/services/ui/ws/window_...
services/ui/ws/window_tree_unittest.cc:1360:
platform_screen.Init(window_server->display_manager(), false);
On 2016/11/22 03:07:39, tonikitoo wrote:
> for good code reading purposes, a named variable for this boolean or an enum
> would be nice.
> 
> But it would require replication in varios places, so maybe you added the
> boolean literal on purpose.

I've added the enum and also a default argument so that the various callsites
don't need to change

[Tom (Use chromium acct), 2016-11-23 03:24:55]

ping. rjkroege@ this is ready for another review

[rjkroege, 2016-11-24 00:58:28]

Sorry for the slow review.

I think is getting much better. sky@ needs to look at the ws/* stuff. Only minor
stuff.

I am still a bit concerned about what happens with multiple physical Displays
objects but that's something to worry about later.

https://codereview.chromium.org/2503923003/diff/220001/services/ui/display/pl...
File services/ui/display/platform_screen.h (right):

https://codereview.chromium.org/2503923003/diff/220001/services/ui/display/pl...
services/ui/display/platform_screen.h:39: // configuration the connected
displays. Off device this will create one or
maybe fix the grammar while you're here. And the comment is clearly wrong in our
beautiful new world.

https://codereview.chromium.org/2503923003/diff/220001/services/ui/display/pl...
File services/ui/display/platform_screen_stub.h (right):

https://codereview.chromium.org/2503923003/diff/220001/services/ui/display/pl...
services/ui/display/platform_screen_stub.h:30: WindowMode window_mode =
WINDOW_MODE_INTERNAL) override;
I'd prefer if you didn't specify a default argument like this and style guide
suggests that they not be used on virtual functions. Besides, I think you've
touched almost all of the call sites anyway?

https://codereview.chromium.org/2503923003/diff/220001/services/ui/ws/window_...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2503923003/diff/220001/services/ui/ws/window_...
services/ui/ws/window_server.cc:756: bool window_manager_connected = false;
I would have thought that GetFactories().size() > 0 would be sufficient and
safer against races at start? Probably sky@ knows for sure.

[Tom (Use chromium acct), 2016-11-28 20:47:36]

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

[Tom (Use chromium acct), 2016-11-28 20:47:38]

+sky for ws

rjkroege@ and I decided we should try to land these changes rather than just
have them as a proof-of-concept

https://codereview.chromium.org/2503923003/diff/220001/services/ui/display/pl...
File services/ui/display/platform_screen.h (right):

https://codereview.chromium.org/2503923003/diff/220001/services/ui/display/pl...
services/ui/display/platform_screen.h:39: // configuration the connected
displays. Off device this will create one or
On 2016/11/24 00:58:28, rjkroege wrote:
> maybe fix the grammar while you're here. And the comment is clearly wrong in
our
> beautiful new world.

Done.

https://codereview.chromium.org/2503923003/diff/220001/services/ui/display/pl...
File services/ui/display/platform_screen_stub.h (right):

https://codereview.chromium.org/2503923003/diff/220001/services/ui/display/pl...
services/ui/display/platform_screen_stub.h:30: WindowMode window_mode =
WINDOW_MODE_INTERNAL) override;
On 2016/11/24 00:58:28, rjkroege wrote:
> I'd prefer if you didn't specify a default argument like this and style guide
> suggests that they not be used on virtual functions. Besides, I think you've
> touched almost all of the call sites anyway?

Done.

https://codereview.chromium.org/2503923003/diff/220001/services/ui/ws/window_...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2503923003/diff/220001/services/ui/ws/window_...
services/ui/ws/window_server.cc:756: bool window_manager_connected = false;
On 2016/11/24 00:58:28, rjkroege wrote:
> I would have thought that GetFactories().size() > 0 would be sufficient and
> safer against races at start? Probably sky@ knows for sure.

In my testing, there's an initial factory even when no window manager gets
connected.
sky@ can you verify this change?

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

https://codereview.chromium.org/2503923003/diff/260001/services/ui/display/pl...
services/ui/display/platform_screen_ozone.cc:80: if
(!base::SysInfo::IsRunningOnChromeOS()) {
Does anyone know if this should also be wrapped with 
"if (window_mode == WINDOW_MODE_INTERNAL)"?

[sky, 2016-11-28 21:40:05]

WindowTreeHostFactory isn't going to work well with aura-mus. Before adding more
depenendencies on WindowTreeHostFactory it needs to be made to work well with
aura-mus. In particular WindowTreeHostFactory needs to be changed so that there
is a single WindowTreeClient that serves multiple windowtrees.

https://codereview.chromium.org/2503923003/diff/260001/services/ui/demo/manif...
File services/ui/demo/manifest.json (right):

https://codereview.chromium.org/2503923003/diff/260001/services/ui/demo/manif...
services/ui/demo/manifest.json:7: "ui": [ "app", "ui:window_manager",
"ui:window_tree_host_factory" ]
Work is happening now to convert this code to use aura. Mikhail should make sure
this doesn't conflict with what he is doing.

[Tom (Use chromium acct), 2016-12-06 00:51:15]

On 2016/11/28 21:40:05, sky wrote:
> WindowTreeHostFactory isn't going to work well with aura-mus. Before adding
more
> depenendencies on WindowTreeHostFactory it needs to be made to work well with
> aura-mus. In particular WindowTreeHostFactory needs to be changed so that
there
> is a single WindowTreeClient that serves multiple windowtrees.
> 

Btw: tonikitoo@ I'm not exactly actively working on this rn, so if you want to
make the WindowTreeHostFactory change, be my guest.

The mojom definition would need to change to something like this:

diff --git a/services/ui/public/interfaces/window_tree_host.mojom
b/services/ui/public/interfaces/window_tree_host.mojom
index 8a349600..b611a79 100644
--- a/services/ui/public/interfaces/window_tree_host.mojom
+++ b/services/ui/public/interfaces/window_tree_host.mojom
@@ -18,8 +18,6 @@ interface WindowTreeHost {
 };
 
 interface WindowTreeHostFactory {
-  // Creates a new WindowTreeHost. |tree_client| is queried for the
-  // WindowManager.
-  CreateWindowTreeHost(WindowTreeHost& window_tree_host,
-                       WindowTreeClient tree_client);
+  Register(WindowTreeClient tree_client);
+  CreateWindowTreeHost(WindowTreeHost& host, WindowTree& tree_request);
 };

>
https://codereview.chromium.org/2503923003/diff/260001/services/ui/demo/manif...
> File services/ui/demo/manifest.json (right):
> 
>
https://codereview.chromium.org/2503923003/diff/260001/services/ui/demo/manif...
> services/ui/demo/manifest.json:7: "ui": [ "app", "ui:window_manager",
> "ui:window_tree_host_factory" ]
> Work is happening now to convert this code to use aura. Mikhail should make
sure
> this doesn't conflict with what he is doing.

[fwang, 2017-01-10 10:32:53]

On 2016/12/06 00:51:15, Tom Anderson wrote:
> > Work is happening now to convert this code to use aura. Mikhail should make
> sure
> > this doesn't conflict with what he is doing.

tom@: What is your plan for this change? I tried rebasing your patch and in most
files, the conflicts are due to the renaming platform_screen => screen_manager.
However, the mus_demo code has also changed quite a lot, especially the aura
conversion mentioned by sky@ has landed:

https://codereview.chromium.org/2551733002

(see also https://codereview.chromium.org/2581163003 for the bitmap)

[fwang, 2017-01-10 15:16:41]

On 2017/01/10 10:32:53, fwang wrote:
> tom@: What is your plan for this change?

So following-up on this, one of the problem is that it seems that the aura/mus
environment must have a single window_tree_client_ associated via
SetWindowTreeClient. So we should indeed allow that client to server multiples
window trees, instead of creating several window trees...

[sky, 2017-01-10 16:06:07]

For the most part you are right, but you could make aura mostly work
with multiple WindowTreeClients if every time you create an
aura::Window you supply a WindowPortMus pointing to the correct
WindowTreeClient.

None-the-less that isn't really what you want here and would have
subtle other effects that wouldn't work for chrome. What you really
need is the ability, at the mojom level, for WindowTreeHostFactory to
have a single WindowTreeClient and numerous WindowTreeHosts.

  -Scott

On Tue, Jan 10, 2017 at 7:16 AM,  <fwang@igalia.com> wrote:
> On 2017/01/10 10:32:53, fwang wrote:
>> tom@: What is your plan for this change?
>
> So following-up on this, one of the problem is that it seems that the
> aura/mus
> environment must have a single window_tree_client_ associated via
> SetWindowTreeClient. So we should indeed allow that client to server
> multiples
> window trees, instead of creating several window trees...
>
> https://codereview.chromium.org/2503923003/

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

[Tom (Use chromium acct), 2017-01-10 18:44:45]

On 2017/01/10 10:32:53, fwang wrote:
> tom@: What is your plan for this change? I tried rebasing your patch and in
most
> files, the conflicts are due to the renaming platform_screen =>
screen_manager.
> However, the mus_demo code has also changed quite a lot, especially the aura
> conversion mentioned by sky@ has landed:
> 
> https://codereview.chromium.org/2551733002
> 
> (see also https://codereview.chromium.org/2581163003 for the bitmap)

I don't currently have any plans for this CL.  If you'd like to take over this
one, you're more than welcome :)

[fwang, 2017-01-11 17:37:20]

On 2017/01/10 18:44:45, Tom Anderson wrote:
> I don't currently have any plans for this CL.  If you'd like to take over this
> one, you're more than welcome :)

Thanks, I've started to rebase this in
https://codereview.chromium.org/2622103004/