Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2712203002:
c++ / mojo changes for 'external window mode'
|
Created:
3 years, 10 months ago by tonikitoo Modified:
3 years, 9 months ago CC:
chromium-reviews, rjkroege, sadrul, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, kalyank, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionc++ / mojo changes for 'external window mode'
This CL is the first step towards having 'external window mode' support
in Mus. The existing MusDemo is adapted to test the functionality,
and its respective unit test (mus_demo_unittests) is changed to
exercise multiple external windows creation in a follow up CL [2].
[2] https://codereview.chromium.org/2715533005/
New code flow:
* MusDemoExternal creates and holds a WindowTreeClient instance.
Note: In Chrome, it also happens in
c/b/ui/views/chrome_browser_main_extra_parts_views.cc,
::ServiceManagerConnectionStarted, when MusClient is created.
* MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory.
This "enters" WindowTreeClient in 'external window mode', and
creates a WindowTreeHostFactoryRegistrar instance.
Through this object, WindowTreeHostFactoryRegistrar::Register is
called, creating WindowTree and WindowTreeHostFactory instances on
the server side, and acquiring WindowTree and
WindowTreeHostFactory mojo handles on the client side.
Note: In external window mode, the WindowTree and WindowTreeHostFactory
are unique instances, serving 0 or 'n' WindowTreeHostMus instances.
* Yet from MusDemoExternal, WindowTreeHostMus instance(s) are
created, in accordance to the value passed to command line
parameter --external-window-count, or 1 by default.
Note: In Chrome, this happens in
c/b/ui/views/frame/browser_frame_mus.cc, ::GetWidgetParams when
DesktopWindowTreeHostMus is created for each outer platform window.
WindowTreeHostMus ctor takes a WindowTreeClient instance which
works as a "window tree host delegate".
* During the creation chain of WindowTreeHostMus, WindowTreeClient::CreateWindowPortForTopLevel
is called, and this is what calls the newly added method
WindowTreeHostFactory::CreatePlatformWindow, when in 'external
window mode'. That is the entry point of of ws::Display instance
creation.
* In the existing creation flow of ws::Display, the server
actually calls back to the client via WindowTreeClient::OnEmbed
when it is done. In 'external window mode' this callback is kept,
but it passes a 'null' WindowTree object as parameter.
Why? Because in ::ConnectViaWindowTreeHostFactory (above) the
unique WindowTree instance was already created.
* services/ui/ws/window_tree_host_factory_registrar.cc|h:
This class implements WindowTreeHostFactoryRegistrar class.
It exposes a ::Register method, that:
(a) Binds the mojom::WindowTreeHostFactoryRequest
(b) Binds the mojom::WindowTreeRequest
(c) Stores the mojom::WindowTreeClientPtr to call out
WindowTreeClient later on.
(d) A ws::WindowTree instance is also created here. It ensures
single WindowTree and WindowTreeClient instances serving
various WindowTreeHost instances.
The goal of the WindowTreeHostFactoryRegistrar interface is
to ensure a WindowTreeHostFactory::CreatePlatformWindow can not
be called before WindowTreeHostFactory being properly set up.
* services/ui/ws/window_tree_host_factory.cc|h:
Adds ::CreatePlatformWindow method so that it triggers the
creation of per-outer-window ws::Display instances in external
window mode.
TODO (in follow up CLs):
1) Factor non-specific WindowManager specific bits out of
ws::WindowManagerState, so that event dispatching code can be
shared between internal and external window modes.
2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager
path.
3) Clean up mus_demo_external (launch instances in parallel?).
4) Clean up WindowTreeClient::OnEmbed flow.
5) Experiment with launching Chrome in external window mode.
BUG=666958
Patch Set 1 #Patch Set 2 : . #
Total comments: 31
Patch Set 3 : addressing fwang/sky feedback #
Total comments: 2
Patch Set 4 : . #Patch Set 5 : rebased on top of http://crrev.com/2723853004/ #Patch Set 6 : addressed sky's request (take 1) #Patch Set 7 : addressed sky's request (take 2) #Patch Set 8 : addressed sky's request (take 3) #Patch Set 9 : addressed sky's request (take 4) #
Total comments: 22
Patch Set 10 : addressed sky/fwang feedback (take 5), simpler mus_demo changes / passing unittests #
Total comments: 22
Patch Set 11 : addressed sky/fwang/kylechar feedback (take 6), simpler mus_demo changes / passing unittests #
Total comments: 1
Patch Set 12 : rebased #Patch Set 13 : rebased #
Dependent Patchsets: Messages
Total messages: 72 (52 generated)
tonikitoo@igalia.com changed reviewers: + sky@chromium.org
Hi sky@. As described in https://bugs.chromium.org/p/chromium/issues/detail?id=666958#c20 (and your follow up comment https://bugs.chromium.org/p/chromium/issues/detail?id=666958#c22), I prototype my understanding of the mojo/c++ interfaces needed for 'external window mode'. For instance, mus_demo launches fine with it for me. Also, I have some more changes locally, but it would be great to validate with you these bits first, so that I would be sure whether the direction it is taking is proper.
Description was changed from
==========
[WIP] c++ / mojo changes for 'external window mode'
Highlights:
* services/ui/service.cc|h:
It does not create a WindowTreeHostFactory directly anymore.
Instead an WindowTreeHostFactoryRegistrar is created.
It exposes an ::Register method, that takes
(a) a mojom::WindowTreeHostFactory&
(b) a mojom::WindowTree&
(c) a mojom::WindowTreeClient
The goal of the WindowTreeHostFactoryRegistrar interface is to
ensure a WindowTreeHostFactory::CreateWindowTreeHost can not be
called before WindowTreeHostFactory being properly set up.
* services/ui/ws/window_tree_host_factory.cc|h:
Adds a ::Init method that
(a) Binds the mojom::WindowTreeHostFactoryRequest
(b) Binds the mojom::WindowTreeRequest
(c) Stores the mojom::WindowTreeClientPtr to call out
WindowTreeClient later on.
(d) ws::WindowTree instance is also created here. It ensure
we to have one WindowTree and WindowTreeClient instances serving
various WindowTreeHost instances.
Changes ::CreateTreeHost method so that it triggers the creation of
per-outter-window ws::Display instances.
* aura/mus/window_tree_client.cc|h
Adds a ::ConnectViaWindowTreeHostFactory method that does the
WindowTreeHostFactoryRegistrar and WindowTreeHostFactory setup.
Adds a ::CreateHost method that calls out to
WindowTreeHostFactory::CreateWindowTreeHost.
BUG=666958
==========
to
==========
[WIP] c++ / mojo changes for 'external window mode'
Highlights:
* services/ui/service.cc|h:
It does not create a WindowTreeHostFactory directly anymore.
Instead n WindowTreeHostFactoryRegistrar is created.
It exposes n ::Register method, that takes
(a) a mojom::WindowTreeHostFactory&
(b) a mojom::WindowTree&
(c) a mojom::WindowTreeClient
The goal of the WindowTreeHostFactoryRegistrar interface is
ensure a WindowTreeHostFactory::CreateWindowTreeHost can not be
called before WindowTreeHostFactory being properly set up.
* services/ui/ws/window_tree_host_factory.cc|h:
Adds a ::Init method that
(a) Binds the mojom::WindowTreeHostFactoryRequest
(b) Binds the mojom::WindowTreeRequest
(c) Stores the mojom::WindowTreeClientPtr to call out
WindowTreeClient later on.
(d) ws::WindowTree instance is also created here. It ensure
we to have one WindowTree and WindowTreeClient instances serving
various WindowTreeHost instances.
Changes ::CreateTreeHost method so that it triggers the creation of
per-outer-window ws::Display instances.
* aura/mus/window_tree_client.cc|h
Adds a ::ConnectViaWindowTreeHostFactory method that does the
WindowTreeHostFactoryRegistrar and WindowTreeHostFactory setup.
Adds a ::CreateHost method that calls out to
WindowTreeHostFactory::CreateWindowTreeHost.
BUG=666958
==========
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
fwang@igalia.com changed reviewers: + fwang@igalia.com
https://codereview.chromium.org/2712203002/diff/20001/services/ui/demo/mus_de... File services/ui/demo/mus_demo_external.h (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo_external.h:35: mojom::WindowTreeHostFactoryPtr window_tree_host_factory_; You can remove window_tree_host_factory_ too, I believe. https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... File services/ui/ws/window_tree_host_factory.cc (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... services/ui/ws/window_tree_host_factory.cc:44: // Pass nullptr as mojom::WindowTreePtr (3rd paramaneter), because in external parameter
Is this CL still WIP?
On 2017/02/27 20:08:20, fwang wrote: Thank you for the review! > Is this CL still WIP? Yes, the WindowTree::window_manager_internal_ and WindowTree::window_manager_state_ still need to be figured out for 'external window mode'.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I only looked at the mojom and aura changes. https://codereview.chromium.org/2712203002/diff/20001/services/ui/public/inte... File services/ui/public/interfaces/window_tree_host.mojom (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/public/inte... services/ui/public/interfaces/window_tree_host.mojom:12: interface WindowTreeHost { As mentioned earlier this needs to be renamed. WindowTreeHost is confusing in the context of aura given there is already an aura::WindowTreeHost class. https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:255: window_tree_host_factory_ptr_->CreateWindowTreeHost(std::move(host)); I suspect that this function shouldn't be called directly. That is, when client code creates an aura::WindowTreeHostMus the code ends up here. That's assuming the WindowTreeclient was configured using ConnectViaWindowTreeHostFactory. If the WindowTreeClient wasn't configured in that way then the code needs to do what it is doing today.
I have not tested the patch yet, but here are more thoughts. https://codereview.chromium.org/2712203002/diff/20001/services/ui/public/inte... File services/ui/public/interfaces/window_tree_host.mojom (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/public/inte... services/ui/public/interfaces/window_tree_host.mojom:12: interface WindowTreeHost { On 2017/02/28 05:02:22, sky wrote: > As mentioned earlier this needs to be renamed. WindowTreeHost is confusing in > the context of aura given there is already an aura::WindowTreeHost class. The comment should also be fixed as the object can now encapsulate several platform windows corresponding to the tree roots. I think it would be great if the name can make explicit this holds a set of platform windows. Maybe PlatformWindowTree? https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/display.cc File services/ui/ws/display.cc (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/display.... services/ui/ws/display.cc:221: window_tree->DoOnEmbed(nullptr /*mojom::WindowTreePtr*/, Is commenting nullptr parameter something common in chromium style? https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... services/ui/ws/window_tree.cc:103: DoOnEmbed(std::move(tree), nullptr /*ServerWindow*/); ditto about /* */ comment https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... services/ui/ws/window_tree.cc:106: void WindowTree::DoOnEmbed(mojom::WindowTreePtr tree, ServerWindow* r) { what about root_window to make it more explicit than r? https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... services/ui/ws/window_tree.cc:108: CHECK_NE(0u, roots_.size()); These are unsigned integers, but maybe CHECK_LE(1u,..) more clearly shows "we need at least 1 root". You are also removing the check that we want exactly one root in internal window mode. Is is wanted? Or should we add such a check back to WindowTree::Init? https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... services/ui/ws/window_tree.cc:213: root->parent()->IsDrawn()); This is the part that ensures MusDemoInternal is aware of the the display. For now MusDemoExternal still adds its own "fake display" but we'd have to figure out how to handle that case (probably in follow-up CLs). https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... File services/ui/ws/window_tree.h (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... services/ui/ws/window_tree.h:80: // Call WindowTreeClient::OnEmbed. I think it's "Calls" for consistency with other comments. https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... File services/ui/ws/window_tree_client_unittest.cc (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... services/ui/ws/window_tree_client_unittest.cc:677: window_tree_host_factory->CreateWindowTreeHost(MakeRequest(&host_)); As I see, we do not have test coverage for WindowTreeHost::SetSize and WindowTreeHost::SetTitle. Probably something else to add in a follow-up CL... https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... File services/ui/ws/window_tree_host_factory.cc (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... services/ui/ws/window_tree_host_factory.cc:48: // NOTE: This call to ::AddTree calls ::Init. However, it wont trigger a won't https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... services/ui/ws/window_tree_host_factory.cc:60: nullptr /* tree_client */, window_server_)); ditto about /* */ https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (left): https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:508: DCHECK(roots_.empty()); Should this be kept when we do OnEmbed without configuring with ConnectViaWindowManagerHostFactory (if that's ever possible)? https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:255: window_tree_host_factory_ptr_->CreateWindowTreeHost(std::move(host)); On 2017/02/28 05:02:22, sky wrote: > I suspect that this function shouldn't be called directly. That is, when client > code creates an aura::WindowTreeHostMus the code ends up here. That's assuming > the WindowTreeclient was configured using ConnectViaWindowTreeHostFactory. If > the WindowTreeClient wasn't configured in that way then the code needs to do > what it is doing today. Mmmh, see the changes in MusDemo for how it is used. Currently, it is the other way around. After a call to WindowTreeClient::ConnectViaWindowTreeHostFactory(), each WindowTreeClient::CreateHost() will create a ws::Display on the server which will call back OnEmbed on the client (with a nullptr) and creates an aura::WindowTreeHostMus. https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:862: // ConnectViaWindowManagerHostFactory. What happens when we do OnEmbed without configuring with ConnectViaWindowManagerHostFactory? https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.h (right): https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.h:110: void CreateHost(ui::mojom::WindowTreeHostRequest host); You'll have to update the comment and function name after the mojo renaming...
https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:255: window_tree_host_factory_ptr_->CreateWindowTreeHost(std::move(host)); On 2017/02/28 08:23:53, fwang wrote: > On 2017/02/28 05:02:22, sky wrote: > > I suspect that this function shouldn't be called directly. That is, when > client > > code creates an aura::WindowTreeHostMus the code ends up here. That's assuming > > the WindowTreeclient was configured using ConnectViaWindowTreeHostFactory. If > > the WindowTreeClient wasn't configured in that way then the code needs to do > > what it is doing today. > > Mmmh, see the changes in MusDemo for how it is used. Currently, it is the other > way around. After a call to WindowTreeClient::ConnectViaWindowTreeHostFactory(), > each WindowTreeClient::CreateHost() will create a ws::Display on the server > which will call back OnEmbed on the client (with a nullptr) and creates an > aura::WindowTreeHostMus. Sure, but doing that is awkward and inconvenient, by which I mean having to wait for a callback before you can do anything is error prone. It would be much more natural to create the WindowTreeHostMus and have WindowTreeHostMus/WindowTreeClient take care of everything for you. Doing this will work better with how views works.
kylechar@chromium.org changed reviewers: + kylechar@chromium.org
https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/display.cc File services/ui/ws/display.cc (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/display.... services/ui/ws/display.cc:207: void Display::InitWindowManagerDisplayRoots() { In general you probably don't want to ever hit any of the methods with "WindowManager" in the name in external window mode. Of course the code path doesn't work like that now but it would make more sense if you split some of this logic up. If I understand correctly, binding_ == nullptr means internal mode and you could sprinkle DCHECK(!binding_) in a few methods to ensure we only do WM related stuff in internal mode.
Addressed most of the feedback, however the ::CreateWindowTreeHost method is not changed yet. I left a couple of suggestions. please feel free to advice. https://codereview.chromium.org/2712203002/diff/20001/services/ui/demo/mus_de... File services/ui/demo/mus_demo_external.h (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo_external.h:35: mojom::WindowTreeHostFactoryPtr window_tree_host_factory_; On 2017/02/27 20:06:50, fwang wrote: > You can remove window_tree_host_factory_ too, I believe. Done. https://codereview.chromium.org/2712203002/diff/20001/services/ui/public/inte... File services/ui/public/interfaces/window_tree_host.mojom (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/public/inte... services/ui/public/interfaces/window_tree_host.mojom:12: interface WindowTreeHost { On 2017/02/28 05:02:22, sky wrote: > As mentioned earlier this needs to be renamed. WindowTreeHost is confusing in > the context of aura given there is already an aura::WindowTreeHost class. Right. It essentially creates new ws::Display instances, which by definition inherits mojom::WindowTreeHost, so I although it confuses with aura::WindowTreeHost, there some logic behind it. In any case, I am fine with changing it now for the good. On 2017/02/28 08:23:53, fwang wrote: > On 2017/02/28 05:02:22, sky wrote: > > As mentioned earlier this needs to be renamed. WindowTreeHost is confusing in > > the context of aura given there is already an aura::WindowTreeHost class. > > The comment should also be fixed as the object can now encapsulate several > platform windows corresponding to the tree roots. > > I think it would be great if the name can make explicit this holds a set of > platform windows. Maybe PlatformWindowTree? WindowTreeHost reflects an outer platform window, on the server side, and triggers the creation of an Aura/Mus window on the client side. Maybe ::CreateRootWindow or ::CreatePlarformWindow ? https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... services/ui/ws/window_tree.cc:106: void WindowTree::DoOnEmbed(mojom::WindowTreePtr tree, ServerWindow* r) { On 2017/02/28 08:23:53, fwang wrote: > what about root_window to make it more explicit than r? Done. https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... services/ui/ws/window_tree.cc:108: CHECK_NE(0u, roots_.size()); On 2017/02/28 08:23:53, fwang wrote: > These are unsigned integers, but maybe CHECK_LE(1u,..) more clearly shows "we > need at least 1 root". > > You are also removing the check that we want exactly one root in internal window > mode. Is is wanted? Or should we add such a check back to WindowTree::Init? Done. https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... File services/ui/ws/window_tree.h (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... services/ui/ws/window_tree.h:80: // Call WindowTreeClient::OnEmbed. On 2017/02/28 08:23:53, fwang wrote: > I think it's "Calls" for consistency with other comments. Done. https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... File services/ui/ws/window_tree_host_factory.cc (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... services/ui/ws/window_tree_host_factory.cc:44: // Pass nullptr as mojom::WindowTreePtr (3rd paramaneter), because in external On 2017/02/27 20:06:50, fwang wrote: > parameter Done. https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/window_t... services/ui/ws/window_tree_host_factory.cc:48: // NOTE: This call to ::AddTree calls ::Init. However, it wont trigger a On 2017/02/28 08:23:53, fwang wrote: > won't Done. https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (left): https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:508: DCHECK(roots_.empty()); On 2017/02/28 08:23:53, fwang wrote: > Should this be kept when we do OnEmbed without configuring with > ConnectViaWindowManagerHostFactory (if that's ever possible)? From OnEmbedImpl it is a bit hard. I have moved this DCHECK to OnEmbed. https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:255: window_tree_host_factory_ptr_->CreateWindowTreeHost(std::move(host)); On 2017/02/28 08:23:53, fwang wrote: > On 2017/02/28 05:02:22, sky wrote: > > I suspect that this function shouldn't be called directly. That is, when > client > > code creates an aura::WindowTreeHostMus the code ends up here. That's assuming > > the WindowTreeclient was configured using ConnectViaWindowTreeHostFactory. If > > the WindowTreeClient wasn't configured in that way then the code needs to do > > what it is doing today. > > Mmmh, see the changes in MusDemo for how it is used. Currently, it is the other > way around. After a call to WindowTreeClient::ConnectViaWindowTreeHostFactory(), > each WindowTreeClient::CreateHost() will create a ws::Display on the server > which will call back OnEmbed on the client (with a nullptr) and creates an > aura::WindowTreeHostMus. Hum, I believe I understand sky's point. It might require a bit of change on the current host creation flow. sky: Would it be possible to change it as per your request in a follow up CL, so that we expedite moving this one forward? https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:862: // ConnectViaWindowManagerHostFactory. On 2017/02/28 08:23:53, fwang wrote: > What happens when we do OnEmbed without configuring with > ConnectViaWindowManagerHostFactory? Done.
On 2017/02/28 21:17:17, kylechar wrote: > https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/display.cc > File services/ui/ws/display.cc (right): > > https://codereview.chromium.org/2712203002/diff/20001/services/ui/ws/display.... > services/ui/ws/display.cc:207: void Display::InitWindowManagerDisplayRoots() { > In general you probably don't want to ever hit any of the methods with > "WindowManager" in the name in external window mode. Of course the code path > doesn't work like that now but it would make more sense if you split some of > this logic up. This is my understanding too. In ps3, I am trying to avoid all WindowManager classes, but one is at very very least still needed: WindowManagerDisplayRoot (see the newly added ws::Display::InitDisplayRoot). I believe we should rename this class (WindowManagerDisplayRoot), so that it is window manager agnostic. DefaultDisplayRoot or DisplayRoot, for example (wdyt?). BTW kylechar@, I introduced a ws::Display::in_external_window_mode_ class member in ps3. > If I understand correctly, binding_ == nullptr means internal mode and you could > sprinkle DCHECK(!binding_) in a few methods to ensure we only do WM related > stuff in internal mode. I should do that, yes. Or maybe have some class hierarchy for Display (Display, WindowManagerDisplay - with WM-specific methods). I will think about it and follow up tomorrow.
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
https://codereview.chromium.org/2712203002/diff/40001/services/ui/demo/mus_de... File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2712203002/diff/40001/services/ui/demo/mus_de... services/ui/demo/mus_demo_external.cc:38: const size_t kNumberOfWindows = 1; Maybe you want to change this to e.g. kNumberOfWindows = 3 since your changes add support for multiple windows. https://codereview.chromium.org/2712203002/diff/40001/services/ui/ws/window_t... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2712203002/diff/40001/services/ui/ws/window_t... services/ui/ws/window_tree.cc:114: CHECK_EQ(1u, roots_.size()); Does that compile when assertions are disabled?
https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:255: window_tree_host_factory_ptr_->CreateWindowTreeHost(std::move(host)); On 2017/03/01 05:05:44, tonikitoo wrote: > On 2017/02/28 08:23:53, fwang wrote: > > On 2017/02/28 05:02:22, sky wrote: > > > I suspect that this function shouldn't be called directly. That is, when > > client > > > code creates an aura::WindowTreeHostMus the code ends up here. That's > assuming > > > the WindowTreeclient was configured using ConnectViaWindowTreeHostFactory. > If > > > the WindowTreeClient wasn't configured in that way then the code needs to do > > > what it is doing today. > > > > Mmmh, see the changes in MusDemo for how it is used. Currently, it is the > other > > way around. After a call to > WindowTreeClient::ConnectViaWindowTreeHostFactory(), > > each WindowTreeClient::CreateHost() will create a ws::Display on the server > > which will call back OnEmbed on the client (with a nullptr) and creates an > > aura::WindowTreeHostMus. > > Hum, I believe I understand sky's point. It might require a bit of change on the > current host creation flow. > > sky: Would it be possible to change it as per your request in a follow up CL, so > that we expedite moving this one forward? I would prefer we go in the right direction, which I believe is what I outlined.
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by tonikitoo@igalia.com
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from
==========
[WIP] c++ / mojo changes for 'external window mode'
Highlights:
* services/ui/service.cc|h:
It does not create a WindowTreeHostFactory directly anymore.
Instead n WindowTreeHostFactoryRegistrar is created.
It exposes n ::Register method, that takes
(a) a mojom::WindowTreeHostFactory&
(b) a mojom::WindowTree&
(c) a mojom::WindowTreeClient
The goal of the WindowTreeHostFactoryRegistrar interface is
ensure a WindowTreeHostFactory::CreateWindowTreeHost can not be
called before WindowTreeHostFactory being properly set up.
* services/ui/ws/window_tree_host_factory.cc|h:
Adds a ::Init method that
(a) Binds the mojom::WindowTreeHostFactoryRequest
(b) Binds the mojom::WindowTreeRequest
(c) Stores the mojom::WindowTreeClientPtr to call out
WindowTreeClient later on.
(d) ws::WindowTree instance is also created here. It ensure
we to have one WindowTree and WindowTreeClient instances serving
various WindowTreeHost instances.
Changes ::CreateTreeHost method so that it triggers the creation of
per-outer-window ws::Display instances.
* aura/mus/window_tree_client.cc|h
Adds a ::ConnectViaWindowTreeHostFactory method that does the
WindowTreeHostFactoryRegistrar and WindowTreeHostFactory setup.
Adds a ::CreateHost method that calls out to
WindowTreeHostFactory::CreateWindowTreeHost.
BUG=666958
==========
to
==========
c++ / mojo changes for 'external window mode'
Highlights:
* services/ui/service.cc|h:
It does not create a WindowTreeHostFactory directly anymore; instead, a
WindowTreeHostFactoryRegistrar instance is created.
WindowTreeHostFactoryRegistrar exposes a ::Register method, that takes
(a) a mojom::WindowTreeHostFactory&
(b) a mojom::WindowTree&
(c) a mojom::WindowTreeClient
The goal of the WindowTreeHostFactoryRegistrar interface is
ensure a WindowTreeHostFactory::CreateWindowTreeHost can not be
called before WindowTreeHostFactory being properly set up.
* services/ui/ws/window_tree_host_factory.cc|h:
Adds a ::Init method that
(a) Binds the mojom::WindowTreeHostFactoryRequest
(b) Binds the mojom::WindowTreeRequest
(c) Stores the mojom::WindowTreeClientPtr to call out
WindowTreeClient later on.
(d) A ws::WindowTree instance is also created here. It ensures
single WindowTree and WindowTreeClient instances serving
various WindowTreeHost instances.
Changes ::CreateTreeHost method so that it triggers the creation of
per-outer-window ws::Display instances.
* services/ui/demo:
Adapts the existing mus_demo to the new interfaces. Multiple mus_demo
outer windows can be created using the --external-window-count command
line parameter.
* aura/mus/window_tree_client.cc|h
Adds a ::ConnectViaWindowTreeHostFactory method that does the
WindowTreeHostFactoryRegistrar and WindowTreeHostFactory setup.
Adds a ::CreateHost method that calls out to
WindowTreeHostFactory::CreateWindowTreeHost.
TODO:
1) Rename WindowTreeHostFactory and
WindowTreeHostFactory::CreateWindowTreeHost
once the name with confusing with aura::WindowTreeHost.
2) Factor non-specific WindowManager specific bits out of
ws::WindowManagerState, so that event dispatching code can be shared
between internal and external window modes.
3) Rename WindowManagerDisplayRoot since it is used in non-WindowManager
path.
BUG=666958
==========
tonikitoo@igalia.com changed reviewers: + rjkroege@chromium.org
Description was changed from
==========
c++ / mojo changes for 'external window mode'
Highlights:
* services/ui/service.cc|h:
It does not create a WindowTreeHostFactory directly anymore; instead, a
WindowTreeHostFactoryRegistrar instance is created.
WindowTreeHostFactoryRegistrar exposes a ::Register method, that takes
(a) a mojom::WindowTreeHostFactory&
(b) a mojom::WindowTree&
(c) a mojom::WindowTreeClient
The goal of the WindowTreeHostFactoryRegistrar interface is
ensure a WindowTreeHostFactory::CreateWindowTreeHost can not be
called before WindowTreeHostFactory being properly set up.
* services/ui/ws/window_tree_host_factory.cc|h:
Adds a ::Init method that
(a) Binds the mojom::WindowTreeHostFactoryRequest
(b) Binds the mojom::WindowTreeRequest
(c) Stores the mojom::WindowTreeClientPtr to call out
WindowTreeClient later on.
(d) A ws::WindowTree instance is also created here. It ensures
single WindowTree and WindowTreeClient instances serving
various WindowTreeHost instances.
Changes ::CreateTreeHost method so that it triggers the creation of
per-outer-window ws::Display instances.
* services/ui/demo:
Adapts the existing mus_demo to the new interfaces. Multiple mus_demo
outer windows can be created using the --external-window-count command
line parameter.
* aura/mus/window_tree_client.cc|h
Adds a ::ConnectViaWindowTreeHostFactory method that does the
WindowTreeHostFactoryRegistrar and WindowTreeHostFactory setup.
Adds a ::CreateHost method that calls out to
WindowTreeHostFactory::CreateWindowTreeHost.
TODO:
1) Rename WindowTreeHostFactory and
WindowTreeHostFactory::CreateWindowTreeHost
once the name with confusing with aura::WindowTreeHost.
2) Factor non-specific WindowManager specific bits out of
ws::WindowManagerState, so that event dispatching code can be shared
between internal and external window modes.
3) Rename WindowManagerDisplayRoot since it is used in non-WindowManager
path.
BUG=666958
==========
to
==========
c++ / mojo changes for 'external window mode'
Highlights:
* services/ui/service.cc|h:
It does not create a WindowTreeHostFactory directly anymore; instead, a
WindowTreeHostFactoryRegistrar instance is created.
WindowTreeHostFactoryRegistrar exposes a ::Register method, that takes
(a) a mojom::WindowTreeHostFactory&
(b) a mojom::WindowTree&
(c) a mojom::WindowTreeClient
The goal of the WindowTreeHostFactoryRegistrar interface is
ensure a WindowTreeHostFactory::CreateWindowTreeHost can not be
called before WindowTreeHostFactory being properly set up.
* services/ui/ws/window_tree_host_factory.cc|h:
Adds a ::Init method that
(a) Binds the mojom::WindowTreeHostFactoryRequest
(b) Binds the mojom::WindowTreeRequest
(c) Stores the mojom::WindowTreeClientPtr to call out
WindowTreeClient later on.
(d) A ws::WindowTree instance is also created here. It ensures
single WindowTree and WindowTreeClient instances serving
various WindowTreeHost instances.
Changes ::CreateTreeHost method so that it triggers the creation of
per-outer-window ws::Display instances.
* services/ui/demo:
Adapts the existing mus_demo to the new interfaces. Multiple mus_demo
outer windows can be created using the --external-window-count command
line parameter.
* aura/mus/window_tree_client.cc|h
Adds a ::ConnectViaWindowTreeHostFactory method that does the
WindowTreeHostFactoryRegistrar and WindowTreeHostFactory setup.
Adds a ::CreateHost method that calls out to
WindowTreeHostFactory::CreateWindowTreeHost.
TODO (in follow up CLs):
1) Rename WindowTreeHostFactory and
WindowTreeHostFactory::CreateWindowTreeHost
once the name is confusing with aura::WindowTreeHost.
2) Factor non-specific WindowManager specific bits out of
ws::WindowManagerState, so that event dispatching code can be
shared between internal and external window modes.
3) Rename WindowManagerDisplayRoot since it is used in non-WindowManager
path.
4) Clean up mus_demo_external (launch instances in parallel?).
5) Clean up WindowTreeClient::OnEmbed.
BUG=666958
==========
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
c++ / mojo changes for 'external window mode'
Highlights:
* services/ui/service.cc|h:
It does not create a WindowTreeHostFactory directly anymore; instead, a
WindowTreeHostFactoryRegistrar instance is created.
WindowTreeHostFactoryRegistrar exposes a ::Register method, that takes
(a) a mojom::WindowTreeHostFactory&
(b) a mojom::WindowTree&
(c) a mojom::WindowTreeClient
The goal of the WindowTreeHostFactoryRegistrar interface is
ensure a WindowTreeHostFactory::CreateWindowTreeHost can not be
called before WindowTreeHostFactory being properly set up.
* services/ui/ws/window_tree_host_factory.cc|h:
Adds a ::Init method that
(a) Binds the mojom::WindowTreeHostFactoryRequest
(b) Binds the mojom::WindowTreeRequest
(c) Stores the mojom::WindowTreeClientPtr to call out
WindowTreeClient later on.
(d) A ws::WindowTree instance is also created here. It ensures
single WindowTree and WindowTreeClient instances serving
various WindowTreeHost instances.
Changes ::CreateTreeHost method so that it triggers the creation of
per-outer-window ws::Display instances.
* services/ui/demo:
Adapts the existing mus_demo to the new interfaces. Multiple mus_demo
outer windows can be created using the --external-window-count command
line parameter.
* aura/mus/window_tree_client.cc|h
Adds a ::ConnectViaWindowTreeHostFactory method that does the
WindowTreeHostFactoryRegistrar and WindowTreeHostFactory setup.
Adds a ::CreateHost method that calls out to
WindowTreeHostFactory::CreateWindowTreeHost.
TODO (in follow up CLs):
1) Rename WindowTreeHostFactory and
WindowTreeHostFactory::CreateWindowTreeHost
once the name is confusing with aura::WindowTreeHost.
2) Factor non-specific WindowManager specific bits out of
ws::WindowManagerState, so that event dispatching code can be
shared between internal and external window modes.
3) Rename WindowManagerDisplayRoot since it is used in non-WindowManager
path.
4) Clean up mus_demo_external (launch instances in parallel?).
5) Clean up WindowTreeClient::OnEmbed.
BUG=666958
==========
to
==========
c++ / mojo changes for 'external window mode'
Highlights:
* services/ui/service.cc|h:
CL allows the instantiation of a WindowTreeHostFactoryRegistrar
object. WindowTreeHostFactoryRegistrar exposes a ::Register method,
that takes
(a) a mojom::WindowTreeHostFactory&
(b) a mojom::WindowTree&
(c) a mojom::WindowTreeClient
The goal of the WindowTreeHostFactoryRegistrar interface is
ensure a WindowTreeHostFactory::CreatePlatformWindow can not be
called before WindowTreeHostFactory being properly set up.
* services/ui/ws/window_tree_host_factory_registrar.cc|h:
Class has a ::Register method that:
(a) Binds the mojom::WindowTreeHostFactoryRequest
(b) Binds the mojom::WindowTreeRequest
(c) Stores the mojom::WindowTreeClientPtr to call out
WindowTreeClient later on.
(d) A ws::WindowTree instance is also created here. It ensures
single WindowTree and WindowTreeClient instances serving
various WindowTreeHost instances.
Adds ::CreatePlatformWindow method so that it triggers the creation of
per-outer-window ws::Display instances.
* services/ui/demo:
Adapts the existing mus_demo to the new interfaces. Multiple mus_demo
outer windows can be created using the --external-window-count command
line parameter.
MusDemo now creates a WindowTreeHostMus instance directly, similar to how
'chrome' does.
mus_demo_external.cc/h can be cleaned up more, but in order not add noise
to this CL, I left it for a follow up work.
* aura/mus/window_tree_client.cc|h
Adds a ::ConnectViaWindowTreeHostFactory method that does the
WindowTreeHostFactoryRegistrar and WindowTreeHostFactory setup.
This is the entry point of the so called 'external window mode'.
TODO (in follow up CLs):
1) Factor non-specific WindowManager specific bits out of
ws::WindowManagerState, so that event dispatching code can be
shared between internal and external window modes.
2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager
path.
3) Clean up mus_demo_external (launch instances in parallel?).
4) Clean up WindowTreeClient::OnEmbed.
BUG=666958
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Thank you for updating the patch! IIUC, this CL is mostly for c++ / mojo changes so you try and reduce the changeset in MusDemo. I'm proposing some changes minimize changes even more (actually I'll share a patch with you), even if some cleanup for MusDemo will probably be needed in a follow-up CL. Now that you follow sky's suggestion (i.e. just create aura::WindowTreeHostMus and have the windowtreeclient delegate wait for the OnEmbed callback), I see that we will be able to simplify MusDemo a lot, so it deserves a separate CL anyway. The idea is that we could have an OnEmbedRootReady function receiving the original aura::WindowTreeHostMus* and so MusDemo will be able to search the corresponding WindowTreeData instead of picking the last appended data. Then the whole process I made to append window one by one and wait that they are embedded is no longer necessary: they can just all be created at init and their demo will start once OnEmbedRootReady is called. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... services/ui/demo/mus_demo.h:69: This is introduced only for overriding HasPendingWindowTreeData. But you don't really need to. Just make IsInitialized() check window_delegate_ (or a new initialized_ boolean) instead of the window_tree_host_. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... services/ui/demo/mus_demo.h:73: virtual bool HasPendingWindowTreeData() const; Not needed (see above). https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:29: base::MakeUnique<aura::WindowTreeHostMus>(window_tree_client); IIUC, the Chromium style is to use setter for private variables, instead of making them protected. So this would be + SetWindowTreeHost( + base::MakeUnique<aura::WindowTreeHostMus>(window_tree_client)); https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:36: } Let's postpone too much changes in MusDemo and just handle this case in the parent class (checking window_tree_host != nullptr). https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:57: return initialized_windows_count_ != window_tree_data_list().size(); Again overriding HasPendingWindowTreeData is not needed with the trick above. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:96: InitWindowTreeData(nullptr); This is a bit weird, but I guess it's ok if we don't want to modify MusDemo too much for now. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... File services/ui/demo/mus_demo_external.h (right): https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.h:30: bool HasPendingWindowTreeData() const final; Not needed (see above). https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/windo... File services/ui/demo/window_tree_data.cc (right): https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/windo... services/ui/demo/window_tree_data.cc:79: void WindowTreeData::InitImpl(aura::WindowTreeHostMus* window_tree_host) { Let's keep this in one function and only do the above when window_tree_host is nonnull https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/windo... File services/ui/demo/window_tree_data.h (right): https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/windo... services/ui/demo/window_tree_data.h:29: virtual void Init(std::unique_ptr<aura::WindowTreeHostMus> window_tree_host); Not needed. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/windo... services/ui/demo/window_tree_data.h:37: void InitImpl(aura::WindowTreeHostMus* window_tree_host); Not needed. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/windo... services/ui/demo/window_tree_data.h:40: std::unique_ptr<aura::WindowTreeHostMus> window_tree_host_; This should remain private IIUC chromium style. https://codereview.chromium.org/2712203002/diff/160001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2712203002/diff/160001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:893: delegate_->OnEmbed(nullptr); I believe this is the key point. In the future, we should probably extend aura::WindowTreeClientDelegate with OnEmbedRootReady(aura::WindowTreeHostMus* window_tree_host) and pass a non-null pointer so that the delegate can finalize the initialization of the WindowTreeHostMus.
On 2017/03/01 17:08:38, sky wrote: > https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... > File ui/aura/mus/window_tree_client.cc (right): > > https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree... > ui/aura/mus/window_tree_client.cc:255: > window_tree_host_factory_ptr_->CreateWindowTreeHost(std::move(host)); > On 2017/03/01 05:05:44, tonikitoo wrote: > > On 2017/02/28 08:23:53, fwang wrote: > > > On 2017/02/28 05:02:22, sky wrote: > > > > I suspect that this function shouldn't be called directly. That is, when > > > client > > > > code creates an aura::WindowTreeHostMus the code ends up here. That's > > assuming > > > > the WindowTreeclient was configured using ConnectViaWindowTreeHostFactory. > > If > > > > the WindowTreeClient wasn't configured in that way then the code needs to > do > > > > what it is doing today. > > > > > > Mmmh, see the changes in MusDemo for how it is used. Currently, it is the > > other > > > way around. After a call to > > WindowTreeClient::ConnectViaWindowTreeHostFactory(), > > > each WindowTreeClient::CreateHost() will create a ws::Display on the server > > > which will call back OnEmbed on the client (with a nullptr) and creates an > > > aura::WindowTreeHostMus. > > > > Hum, I believe I understand sky's point. It might require a bit of change on > the > > current host creation flow. > > > > sky: Would it be possible to change it as per your request in a follow up CL, > so > > that we expedite moving this one forward? > > I would prefer we go in the right direction, which I believe is what I outlined. Done in ps16. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... services/ui/demo/mus_demo.h:69: On 2017/03/07 09:21:24, fwang wrote: > This is introduced only for overriding HasPendingWindowTreeData. But you don't > really need to. Just make IsInitialized() check window_delegate_ (or a new > initialized_ boolean) instead of the window_tree_host_. Done. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... services/ui/demo/mus_demo.h:73: virtual bool HasPendingWindowTreeData() const; On 2017/03/07 09:21:24, fwang wrote: > Not needed (see above). Done. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:29: base::MakeUnique<aura::WindowTreeHostMus>(window_tree_client); On 2017/03/07 09:21:24, fwang wrote: > IIUC, the Chromium style is to use setter for private variables, instead of > making them protected. So this would be > > + SetWindowTreeHost( > + base::MakeUnique<aura::WindowTreeHostMus>(window_tree_client)); Done. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:57: return initialized_windows_count_ != window_tree_data_list().size(); On 2017/03/07 09:21:24, fwang wrote: > Again overriding HasPendingWindowTreeData is not needed with the trick above. Done. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:96: InitWindowTreeData(nullptr); On 2017/03/07 09:21:24, fwang wrote: > This is a bit weird, but I guess it's ok if we don't want to modify MusDemo too > much for now. This is right. As written in the commit message, there are follow up clean ups planned, which would take mus_demo into account too. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... File services/ui/demo/mus_demo_external.h (right): https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.h:30: bool HasPendingWindowTreeData() const final; On 2017/03/07 09:21:25, fwang wrote: > Not needed (see above). Done. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/windo... File services/ui/demo/window_tree_data.cc (right): https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/windo... services/ui/demo/window_tree_data.cc:79: void WindowTreeData::InitImpl(aura::WindowTreeHostMus* window_tree_host) { On 2017/03/07 09:21:25, fwang wrote: > Let's keep this in one function and only do the above when window_tree_host is > nonnull Done. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/windo... File services/ui/demo/window_tree_data.h (right): https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/windo... services/ui/demo/window_tree_data.h:29: virtual void Init(std::unique_ptr<aura::WindowTreeHostMus> window_tree_host); On 2017/03/07 09:21:25, fwang wrote: > Not needed. Done. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/windo... services/ui/demo/window_tree_data.h:37: void InitImpl(aura::WindowTreeHostMus* window_tree_host); On 2017/03/07 09:21:25, fwang wrote: > Not needed. Done. https://codereview.chromium.org/2712203002/diff/160001/services/ui/demo/windo... services/ui/demo/window_tree_data.h:40: std::unique_ptr<aura::WindowTreeHostMus> window_tree_host_; On 2017/03/07 09:21:25, fwang wrote: > This should remain private IIUC chromium style. Done.
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
c++ / mojo changes for 'external window mode'
Highlights:
* services/ui/service.cc|h:
CL allows the instantiation of a WindowTreeHostFactoryRegistrar
object. WindowTreeHostFactoryRegistrar exposes a ::Register method,
that takes
(a) a mojom::WindowTreeHostFactory&
(b) a mojom::WindowTree&
(c) a mojom::WindowTreeClient
The goal of the WindowTreeHostFactoryRegistrar interface is
ensure a WindowTreeHostFactory::CreatePlatformWindow can not be
called before WindowTreeHostFactory being properly set up.
* services/ui/ws/window_tree_host_factory_registrar.cc|h:
Class has a ::Register method that:
(a) Binds the mojom::WindowTreeHostFactoryRequest
(b) Binds the mojom::WindowTreeRequest
(c) Stores the mojom::WindowTreeClientPtr to call out
WindowTreeClient later on.
(d) A ws::WindowTree instance is also created here. It ensures
single WindowTree and WindowTreeClient instances serving
various WindowTreeHost instances.
Adds ::CreatePlatformWindow method so that it triggers the creation of
per-outer-window ws::Display instances.
* services/ui/demo:
Adapts the existing mus_demo to the new interfaces. Multiple mus_demo
outer windows can be created using the --external-window-count command
line parameter.
MusDemo now creates a WindowTreeHostMus instance directly, similar to how
'chrome' does.
mus_demo_external.cc/h can be cleaned up more, but in order not add noise
to this CL, I left it for a follow up work.
* aura/mus/window_tree_client.cc|h
Adds a ::ConnectViaWindowTreeHostFactory method that does the
WindowTreeHostFactoryRegistrar and WindowTreeHostFactory setup.
This is the entry point of the so called 'external window mode'.
TODO (in follow up CLs):
1) Factor non-specific WindowManager specific bits out of
ws::WindowManagerState, so that event dispatching code can be
shared between internal and external window modes.
2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager
path.
3) Clean up mus_demo_external (launch instances in parallel?).
4) Clean up WindowTreeClient::OnEmbed.
BUG=666958
==========
to
==========
c++ / mojo changes for 'external window mode'
Highlights:
* services/ui/service.cc|h:
CL allows the instantiation of a WindowTreeHostFactoryRegistrar
object. WindowTreeHostFactoryRegistrar exposes a ::Register method,
that takes
(a) a mojom::WindowTreeHostFactory&
(b) a mojom::WindowTree&
(c) a mojom::WindowTreeClient
The goal of the WindowTreeHostFactoryRegistrar interface is
to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be
called before WindowTreeHostFactory being properly set up.
* services/ui/ws/window_tree_host_factory_registrar.cc|h:
Class has a ::Register method that:
(a) Binds the mojom::WindowTreeHostFactoryRequest
(b) Binds the mojom::WindowTreeRequest
(c) Stores the mojom::WindowTreeClientPtr to call out
WindowTreeClient later on.
(d) A ws::WindowTree instance is also created here. It ensures
single WindowTree and WindowTreeClient instances serving
various WindowTreeHost instances.
Adds ::CreatePlatformWindow method so that it triggers the creation of
per-outer-window ws::Display instances.
* services/ui/demo:
Adapts the existing mus_demo to the new interfaces. Multiple mus_demo
outer windows can be created using the --external-window-count command
line parameter.
MusDemo now creates a WindowTreeHostMus instance directly, similar to how
'chrome' does.
mus_demo_external.cc/h can be cleaned up more, but in order not to add
noise to this CL, it is left for a follow up work.
* aura/mus/window_tree_client.cc|h
Adds a ::ConnectViaWindowTreeHostFactory method that does the
WindowTreeHostFactoryRegistrar and WindowTreeHostFactory setup.
This is the entry point of the so called 'external window mode'.
TODO (in follow up CLs):
1) Factor non-specific WindowManager specific bits out of
ws::WindowManagerState, so that event dispatching code can be
shared between internal and external window modes.
2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager
path.
3) Clean up mus_demo_external (launch instances in parallel?).
4) Clean up WindowTreeClient::OnEmbed flow.
BUG=666958
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've looked over MusDemo and WS code a bit and have some questions. i didn't really look over the client lib code as I'm not familiar with it. https://codereview.chromium.org/2712203002/diff/180001/services/ui/demo/windo... File services/ui/demo/window_tree_data.cc (right): https://codereview.chromium.org/2712203002/diff/180001/services/ui/demo/windo... services/ui/demo/window_tree_data.cc:71: if (window_tree_host) { When would |window_tree_host| be null? https://codereview.chromium.org/2712203002/diff/180001/services/ui/manifest.json File services/ui/manifest.json (right): https://codereview.chromium.org/2712203002/diff/180001/services/ui/manifest.j... services/ui/manifest.json:62: "window_tree_host_factory": [ Is this still used? https://codereview.chromium.org/2712203002/diff/180001/services/ui/public/int... File services/ui/public/interfaces/window_tree_host.mojom (right): https://codereview.chromium.org/2712203002/diff/180001/services/ui/public/int... services/ui/public/interfaces/window_tree_host.mojom:30: // Delete empty comment line. https://codereview.chromium.org/2712203002/diff/180001/services/ui/public/int... services/ui/public/interfaces/window_tree_host.mojom:47: CreatePlatformWindow(WindowTreeHost& window_tree_host, uint32 client_id); What is |client_id| supposed to be? https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display.cc File services/ui/ws/display.cc (right): https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display... services/ui/ws/display.cc:67: DCHECK(window_server_->window_tree_host_factory() && binding); DCHECKs are compiled out in release builds, that might cause problems with the if statement. https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display... services/ui/ws/display.cc:218: window_manager_display_root_map_[service_manager::mojom::kRootUserID] = Once you aren't using WindowManagerDisplayRoot anymore in external mode, you also wouldn't use this map, right? https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display... services/ui/ws/display.cc:229: // Tests can create ws::Display instances, directly by-passing The if (binding_) path is only for tests now? https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display.h File services/ui/ws/display.h (right): https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display... services/ui/ws/display.h:211: std::unique_ptr<WindowManagerDisplayRoot> external_window_mode_display_root_; |external_mode_root_| maybe? https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/window_... File services/ui/ws/window_server.h (right): https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/window_... services/ui/ws/window_server.h:396: std::unique_ptr<WindowTreeHostFactory> window_tree_host_factory_; There is already a WindowTreeHostFactory in ui::Service for each user? Why add this? https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/window_... File services/ui/ws/window_tree_host_factory_registrar.cc (right): https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/window_... services/ui/ws/window_tree_host_factory_registrar.cc:26: std::unique_ptr<WindowTreeHostFactory> host_factory( It's preferred to use MakeUnique, so something like: auto host_factory = base::MakeUnique<WIndowTreeHostFactory>(window_server_, user_id_); https://codereview.chromium.org/2712203002/diff/180001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.h (right): https://codereview.chromium.org/2712203002/diff/180001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.h:231: void ConfigureWindowDataFromServer(WindowTreeHostMus* window_tree_host, What does this do?
https://codereview.chromium.org/2712203002/diff/180001/services/ui/demo/windo... File services/ui/demo/window_tree_data.cc (right): https://codereview.chromium.org/2712203002/diff/180001/services/ui/demo/windo... services/ui/demo/window_tree_data.cc:71: if (window_tree_host) { On 2017/03/07 18:19:36, kylechar wrote: > When would |window_tree_host| be null? Before this CL, all the ws::Display creation happened on the server side first, and only upon finishing it, WindowTreeClient::OnEmbed was called. In sequence, WindowTreeClient::OnEmbedImpl was called, and then WindowTreeClient::CreateWindowTreeHost. Only at this point, with the WindowClientHostMus instance available, that MusDemo::OnEmbed was called, taking the WindowTreeHostMus instance as parameter. See https://cs.chromium.org/chromium/src/ui/aura/mus/window_tree_client.cc?q=Wind... In 'external window mode', it is MusDemoExternal that actually creates on WindowTreeHostMus instances. So, when MusDemo::OnEmbed is called, it does not need to pass an WindowTreeHostMus instance because it was already created. This can be clearer when we introduce ::OnEmbedRootReady to be called here, instead of ::OnEmbed. https://codereview.chromium.org/2712203002/diff/180001/services/ui/manifest.json File services/ui/manifest.json (right): https://codereview.chromium.org/2712203002/diff/180001/services/ui/manifest.j... services/ui/manifest.json:62: "window_tree_host_factory": [ On 2017/03/07 18:19:36, kylechar wrote: > Is this still used? In comment https://codereview.chromium.org/2712203002/#msg12 (from sky), I understood that this code path should still exist for when we are not in 'external window mode'. There are tests that use it, so I suggest that if we are going to deprecate it, we do in a follow up step. https://codereview.chromium.org/2712203002/diff/180001/services/ui/public/int... File services/ui/public/interfaces/window_tree_host.mojom (right): https://codereview.chromium.org/2712203002/diff/180001/services/ui/public/int... services/ui/public/interfaces/window_tree_host.mojom:30: // On 2017/03/07 18:19:37, kylechar wrote: > Delete empty comment line. Will fix. https://codereview.chromium.org/2712203002/diff/180001/services/ui/public/int... services/ui/public/interfaces/window_tree_host.mojom:47: CreatePlatformWindow(WindowTreeHost& window_tree_host, uint32 client_id); On 2017/03/07 18:19:36, kylechar wrote: > What is |client_id| supposed to be? WindowHostTreeMus is created on the client side before its associated root ws::ServerWindow. Since it already has an 'id', we need to inform the server about it. So, this client_id is the 'id' of this WindowHostTreeMus instance. Later on, in WindowTree::AddRoot, we associated this 'id' with the respective server window id (see below). 219 void WindowTree::AddRoot(const ServerWindow* root) { 220 DCHECK(pending_client_id_ != kInvalidClientId); 221 222 const ClientWindowId client_window_id(pending_client_id_); 223 DCHECK_EQ(0u, client_id_to_window_id_map_.count(client_window_id)); 224 225 client_id_to_window_id_map_[client_window_id] = root->id(); 226 window_id_to_client_id_map_[root->id()] = client_window_id; 227 pending_client_id_ = kInvalidClientId; (...) 233 } https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display.cc File services/ui/ws/display.cc (right): https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display... services/ui/ws/display.cc:67: DCHECK(window_server_->window_tree_host_factory() && binding); On 2017/03/07 18:19:37, kylechar wrote: > DCHECKs are compiled out in release builds, that might cause problems with the > if statement. BuildBots (including release bots) seem happy, but if you think it looks odd, I can try it differently. https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display... services/ui/ws/display.cc:218: window_manager_display_root_map_[service_manager::mojom::kRootUserID] = On 2017/03/07 18:19:37, kylechar wrote: > Once you aren't using WindowManagerDisplayRoot anymore in external mode, you > also wouldn't use this map, right? This is still needed, but should be clean up indeed. Will add a TODO. When WindowTree::AddWindow is called, this calling out one the methods in ws::Display that still need this map to be filled up. https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display... services/ui/ws/display.cc:229: // Tests can create ws::Display instances, directly by-passing On 2017/03/07 18:19:37, kylechar wrote: > The if (binding_) path is only for tests now? Yes and No. Yes, because I believe only tests exercise this path today. No, because one can still call WindowTreeHostFactory::CreateWindowTreeHost directly, and end up here. This won't be in 'external window mode' though. I left this existing code path untouched not to break existing tests, and if we decide to change/remove it in the future, we should do it altogether. https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display.h File services/ui/ws/display.h (right): https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display... services/ui/ws/display.h:211: std::unique_ptr<WindowManagerDisplayRoot> external_window_mode_display_root_; On 2017/03/07 18:19:37, kylechar wrote: > |external_mode_root_| maybe? Will fix. https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/window_... File services/ui/ws/window_server.h (right): https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/window_... services/ui/ws/window_server.h:396: std::unique_ptr<WindowTreeHostFactory> window_tree_host_factory_; On 2017/03/07 18:19:37, kylechar wrote: > There is already a WindowTreeHostFactory in ui::Service for each user? Why add > this? I tried to avoid it, and saw that service has a WindowTreeHostFactory instance. I added it to access the WindowTree of this WindowTreeHostFactory, and I could not find my way to get it through ws::Service. Will see if I can improve it. https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/window_... File services/ui/ws/window_tree_host_factory_registrar.cc (right): https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/window_... services/ui/ws/window_tree_host_factory_registrar.cc:26: std::unique_ptr<WindowTreeHostFactory> host_factory( On 2017/03/07 18:19:37, kylechar wrote: > It's preferred to use MakeUnique, so something like: > > auto host_factory = base::MakeUnique<WIndowTreeHostFactory>(window_server_, > user_id_); Will fix. https://codereview.chromium.org/2712203002/diff/180001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.h (right): https://codereview.chromium.org/2712203002/diff/180001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.h:231: void ConfigureWindowDataFromServer(WindowTreeHostMus* window_tree_host, On 2017/03/07 18:19:39, kylechar wrote: > What does this do? There is some 'window data' info coming from the server that we need to 'configure' WindowTreeHostMus instances with. this is already the case in std::unique_ptr<WindowTreeHostMus> WindowTreeClient::CreateWindowTreeHost [1]. [1] https://cs.chromium.org/chromium/src/ui/aura/mus/window_tree_client.cc?q=wind... I just factored the code out into a method, so that we could call in external window mode too. See the branch in WindowTreeClient::OnEmbed.
Description was changed from
==========
c++ / mojo changes for 'external window mode'
Highlights:
* services/ui/service.cc|h:
CL allows the instantiation of a WindowTreeHostFactoryRegistrar
object. WindowTreeHostFactoryRegistrar exposes a ::Register method,
that takes
(a) a mojom::WindowTreeHostFactory&
(b) a mojom::WindowTree&
(c) a mojom::WindowTreeClient
The goal of the WindowTreeHostFactoryRegistrar interface is
to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be
called before WindowTreeHostFactory being properly set up.
* services/ui/ws/window_tree_host_factory_registrar.cc|h:
Class has a ::Register method that:
(a) Binds the mojom::WindowTreeHostFactoryRequest
(b) Binds the mojom::WindowTreeRequest
(c) Stores the mojom::WindowTreeClientPtr to call out
WindowTreeClient later on.
(d) A ws::WindowTree instance is also created here. It ensures
single WindowTree and WindowTreeClient instances serving
various WindowTreeHost instances.
Adds ::CreatePlatformWindow method so that it triggers the creation of
per-outer-window ws::Display instances.
* services/ui/demo:
Adapts the existing mus_demo to the new interfaces. Multiple mus_demo
outer windows can be created using the --external-window-count command
line parameter.
MusDemo now creates a WindowTreeHostMus instance directly, similar to how
'chrome' does.
mus_demo_external.cc/h can be cleaned up more, but in order not to add
noise to this CL, it is left for a follow up work.
* aura/mus/window_tree_client.cc|h
Adds a ::ConnectViaWindowTreeHostFactory method that does the
WindowTreeHostFactoryRegistrar and WindowTreeHostFactory setup.
This is the entry point of the so called 'external window mode'.
TODO (in follow up CLs):
1) Factor non-specific WindowManager specific bits out of
ws::WindowManagerState, so that event dispatching code can be
shared between internal and external window modes.
2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager
path.
3) Clean up mus_demo_external (launch instances in parallel?).
4) Clean up WindowTreeClient::OnEmbed flow.
BUG=666958
==========
to
==========
c++ / mojo changes for 'external window mode'
This CL is the first step towards having 'external window mode' support
in Mus. The existing MusDemo is adapted to test the functionality, and
its respective unittests (mus_demo_unittests) is changed to exercise
multiple external windows creation in a follow up CL [2].
[2] https://codereview.chromium.org/2715533005/
New code flow:
* MusDemoExternal creates and holds a WindowTreeClient instance.
* MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory.
This "enters" WindowTreeClient in 'external window mode', and creates a
WindowTreeHostFactoryRegistrar instance.
Through this instance, we call WindowTreeHostFactoryRegistrar::Register,
creating WindowTree and WindowTreeHostFactory instances on the server
side, and acquiring WindowTree and WindowTreeHostFactory mojo handles on the
client side.
Note: In external window mode, the WindowTree and WindowTreeHostFactory
are unique instances, serving 0 or 'n' WindowTreeHostMus instances.
* Yet from MusDemoExternal, WindowTreeHostMus instance(s) are created,
passing its WindowTreeClient instance as a parameter to the ctor of
WindowTreeHostMus, so that WindowTreeClient works as a "window tree
host delegate".
During the WindowTreeHostMus creation chain,
WindowTreeClient::CreateWindowPortForTopLevel
is called. This is what calls WindowTreeHostFactory::CreatePlatformWindow,
when in 'external window mode', which actually triggers the creation
of ws::Display instance(s).
At this point of the process we have:
** WindowTreeClient connected to UI service, in 'external window mode'.
** on the server ws::WindowTree and ws::WindowTreeHostFactory instances, and
theirs respective
mojo handles available on the client side.
The MusDemoExternal flow is now similar to what the 'chrome' browser
does (see DesktopWindowTreeHostMus being created from BrowserFrameMus,
chrome/browser/ui/views/frame/browser_frame_mus.cc).
* In the existing creation flow of ws::Display, the server actually
calls back to the client via WindlwtreeClient::OnEmbed when it is
done. In 'external window mode', this callback is kept, but it passes a
'null' WindowTree object as parameter.
Why? Because in ::ConnectViaWindowTreeHostFactory above we already
created our unique WindowTree instance.
Other highlights:
* aura/mus/window_tree_client.cc|h
Adds a ::ConnectViaWindowTreeHostFactory method that does the
WindowTreeHostFactoryRegistrar and WindowTreeHostFactory setup.
This is the entry point of the so called 'external window mode'.
* services/ui/service.cc|h:
CL allows the instantiation of a WindowTreeHostFactoryRegistrar
object. WindowTreeHostFactoryRegistrar exposes a ::Register method,
that takes
(a) a mojom::WindowTreeHostFactory&
(b) a mojom::WindowTree&
(c) a mojom::WindowTreeClient
The goal of the WindowTreeHostFactoryRegistrar interface is
to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be
called before WindowTreeHostFactory being properly set up.
* services/ui/ws/window_tree_host_factory_registrar.cc|h:
Class has a ::Register method that:
(a) Binds the mojom::WindowTreeHostFactoryRequest
(b) Binds the mojom::WindowTreeRequest
(c) Stores the mojom::WindowTreeClientPtr to call out
WindowTreeClient later on.
(d) A ws::WindowTree instance is also created here. It ensures
single WindowTree and WindowTreeClient instances serving
various WindowTreeHost instances.
Adds ::CreatePlatformWindow method so that it triggers the creation
of per-outer-window ws::Display instances.
* services/ui/demo:
Adapts the existing mus_demo to the new interfaces. Multiple mus_demo
outer windows can be created using the --external-window-count
command line parameter.
MusDemo now creates a WindowTreeHostMus instance directly, similar to
how 'chrome' does.
TODO (in follow up CLs):
1) Factor non-specific WindowManager specific bits out of
ws::WindowManagerState, so that event dispatching code can be
shared between internal and external window modes.
2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager
path.
3) Clean up mus_demo_external (launch instances in parallel?).
4) Clean up WindowTreeClient::OnEmbed flow.
BUG=666958
==========
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== c++ / mojo changes for 'external window mode' This CL is the first step towards having 'external window mode' support in Mus. The existing MusDemo is adapted to test the functionality, and its respective unittests (mus_demo_unittests) is changed to exercise multiple external windows creation in a follow up CL [2]. [2] https://codereview.chromium.org/2715533005/ New code flow: * MusDemoExternal creates and holds a WindowTreeClient instance. * MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory. This "enters" WindowTreeClient in 'external window mode', and creates a WindowTreeHostFactoryRegistrar instance. Through this instance, we call WindowTreeHostFactoryRegistrar::Register, creating WindowTree and WindowTreeHostFactory instances on the server side, and acquiring WindowTree and WindowTreeHostFactory mojo handles on the client side. Note: In external window mode, the WindowTree and WindowTreeHostFactory are unique instances, serving 0 or 'n' WindowTreeHostMus instances. * Yet from MusDemoExternal, WindowTreeHostMus instance(s) are created, passing its WindowTreeClient instance as a parameter to the ctor of WindowTreeHostMus, so that WindowTreeClient works as a "window tree host delegate". During the WindowTreeHostMus creation chain, WindowTreeClient::CreateWindowPortForTopLevel is called. This is what calls WindowTreeHostFactory::CreatePlatformWindow, when in 'external window mode', which actually triggers the creation of ws::Display instance(s). At this point of the process we have: ** WindowTreeClient connected to UI service, in 'external window mode'. ** on the server ws::WindowTree and ws::WindowTreeHostFactory instances, and theirs respective mojo handles available on the client side. The MusDemoExternal flow is now similar to what the 'chrome' browser does (see DesktopWindowTreeHostMus being created from BrowserFrameMus, chrome/browser/ui/views/frame/browser_frame_mus.cc). * In the existing creation flow of ws::Display, the server actually calls back to the client via WindlwtreeClient::OnEmbed when it is done. In 'external window mode', this callback is kept, but it passes a 'null' WindowTree object as parameter. Why? Because in ::ConnectViaWindowTreeHostFactory above we already created our unique WindowTree instance. Other highlights: * aura/mus/window_tree_client.cc|h Adds a ::ConnectViaWindowTreeHostFactory method that does the WindowTreeHostFactoryRegistrar and WindowTreeHostFactory setup. This is the entry point of the so called 'external window mode'. * services/ui/service.cc|h: CL allows the instantiation of a WindowTreeHostFactoryRegistrar object. WindowTreeHostFactoryRegistrar exposes a ::Register method, that takes (a) a mojom::WindowTreeHostFactory& (b) a mojom::WindowTree& (c) a mojom::WindowTreeClient The goal of the WindowTreeHostFactoryRegistrar interface is to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be called before WindowTreeHostFactory being properly set up. * services/ui/ws/window_tree_host_factory_registrar.cc|h: Class has a ::Register method that: (a) Binds the mojom::WindowTreeHostFactoryRequest (b) Binds the mojom::WindowTreeRequest (c) Stores the mojom::WindowTreeClientPtr to call out WindowTreeClient later on. (d) A ws::WindowTree instance is also created here. It ensures single WindowTree and WindowTreeClient instances serving various WindowTreeHost instances. Adds ::CreatePlatformWindow method so that it triggers the creation of per-outer-window ws::Display instances. * services/ui/demo: Adapts the existing mus_demo to the new interfaces. Multiple mus_demo outer windows can be created using the --external-window-count command line parameter. MusDemo now creates a WindowTreeHostMus instance directly, similar to how 'chrome' does. TODO (in follow up CLs): 1) Factor non-specific WindowManager specific bits out of ws::WindowManagerState, so that event dispatching code can be shared between internal and external window modes. 2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager path. 3) Clean up mus_demo_external (launch instances in parallel?). 4) Clean up WindowTreeClient::OnEmbed flow. BUG=666958 ========== to ========== c++ / mojo changes for 'external window mode' This CL is the first step towards having 'external window mode' support in Mus. The existing MusDemo is adapted to test the functionality, and its respective unittests (mus_demo_unittests) is changed to exercise multiple external windows creation in a follow up CL [2]. [2] https://codereview.chromium.org/2715533005/ New code flow: * MusDemoExternal creates and holds a WindowTreeClient instance. * MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory. This "enters" WindowTreeClient in 'external window mode', and creates a WindowTreeHostFactoryRegistrar instance. Through this instance, we call WindowTreeHostFactoryRegistrar::Register, creating WindowTree and WindowTreeHostFactory instances on the server side, and acquiring WindowTree and WindowTreeHostFactory mojo handles on the client side. Note: In external window mode, the WindowTree and WindowTreeHostFactory are unique instances, serving 0 or 'n' WindowTreeHostMus instances. * Yet from MusDemoExternal, WindowTreeHostMus instance(s) are created, in accordance to the --external-window-count parameter. WindowTreeHostMus ctor takes a WindowTreeClient instance which works as a "window tree host delegate". During the creation chain of WindowTreeHostMus, WindowTreeClient::CreateWindowPortForTopLevel is called, and this is what calls WindowTreeHostFactory::CreatePlatformWindow, when in 'external window mode'. That is the entry point of of ws::Display instance creation. At this point of the process we have: ** WindowTreeClient connected to UI service, in 'external window mode'. ** on the server ws::WindowTree and ws::WindowTreeHostFactory instances, and theirs respective mojo handles available on the client side. The MusDemoExternal flow is now similar to what the 'chrome' browser does (see DesktopWindowTreeHostMus being created from BrowserFrameMus, chrome/browser/ui/views/frame/browser_frame_mus.cc). * In the existing creation flow of ws::Display, the server actually calls back to the client via WindlwtreeClient::OnEmbed when it is done. In 'external window mode', this callback is kept, but it passes a 'null' WindowTree object as parameter. Why? Because in ::ConnectViaWindowTreeHostFactory (above) the unique WindowTree instance was already created. Other highlights: * aura/mus/window_tree_client.cc|h Adds a ::ConnectViaWindowTreeHostFactory method that does the WindowTreeHostFactoryRegistrar and WindowTreeHostFactory setup. This is the entry point of the so called 'external window mode'. * services/ui/service.cc|h: CL allows the instantiation of a WindowTreeHostFactoryRegistrar object. WindowTreeHostFactoryRegistrar exposes a ::Register method, that: (a) Binds the mojom::WindowTreeHostFactoryRequest (b) Binds the mojom::WindowTreeRequest (c) Stores the mojom::WindowTreeClientPtr to call out WindowTreeClient later on. (d) A ws::WindowTree instance is also created here. It ensures single WindowTree and WindowTreeClient instances serving various WindowTreeHost instances. The goal of the WindowTreeHostFactoryRegistrar interface is to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be called before WindowTreeHostFactory being properly set up. * services/ui/ws/window_tree_host_factory.cc|h: Adds ::CreatePlatformWindow method so that it triggers the creation of per-outer-window ws::Display instances in external window mode. TODO (in follow up CLs): 1) Factor non-specific WindowManager specific bits out of ws::WindowManagerState, so that event dispatching code can be shared between internal and external window modes. 2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager path. 3) Clean up mus_demo_external (launch instances in parallel?). 4) Clean up WindowTreeClient::OnEmbed flow. BUG=666958 ==========
On 2017/03/07 20:09:36, tonikitoo wrote: > https://codereview.chromium.org/2712203002/diff/180001/services/ui/demo/windo... > File services/ui/demo/window_tree_data.cc (right): > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/demo/windo... > services/ui/demo/window_tree_data.cc:71: if (window_tree_host) { > On 2017/03/07 18:19:36, kylechar wrote: > > When would |window_tree_host| be null? > > Before this CL, all the ws::Display creation happened on the server side first, > and only upon finishing it, WindowTreeClient::OnEmbed was called. > In sequence, WindowTreeClient::OnEmbedImpl was called, and then > WindowTreeClient::CreateWindowTreeHost. > > Only at this point, with the WindowClientHostMus instance available, that > MusDemo::OnEmbed was called, taking the WindowTreeHostMus instance as parameter. > > See > https://cs.chromium.org/chromium/src/ui/aura/mus/window_tree_client.cc?q=Wind... > > > In 'external window mode', it is MusDemoExternal that actually creates on > WindowTreeHostMus instances. > So, when MusDemo::OnEmbed is called, it does not need to pass an > WindowTreeHostMus instance because it was already created. > > This can be clearer when we introduce ::OnEmbedRootReady to be called here, > instead of ::OnEmbed. > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/manifest.json > File services/ui/manifest.json (right): > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/manifest.j... > services/ui/manifest.json:62: "window_tree_host_factory": [ > On 2017/03/07 18:19:36, kylechar wrote: > > Is this still used? > > In comment https://codereview.chromium.org/2712203002/#msg12 (from sky), I > understood that this code path should still exist for when we are not in > 'external window mode'. > > There are tests that use it, so I suggest that if we are going to deprecate it, > we do in a follow up step. > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/public/int... > File services/ui/public/interfaces/window_tree_host.mojom (right): > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/public/int... > services/ui/public/interfaces/window_tree_host.mojom:30: // > On 2017/03/07 18:19:37, kylechar wrote: > > Delete empty comment line. > > Will fix. > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/public/int... > services/ui/public/interfaces/window_tree_host.mojom:47: > CreatePlatformWindow(WindowTreeHost& window_tree_host, uint32 client_id); > On 2017/03/07 18:19:36, kylechar wrote: > > What is |client_id| supposed to be? > > WindowHostTreeMus is created on the client side before its associated root > ws::ServerWindow. Since it already has an 'id', we need to inform the server > about it. > So, this client_id is the 'id' of this WindowHostTreeMus instance. > > Later on, in WindowTree::AddRoot, we associated this 'id' with the respective > server window id (see below). > > > 219 void WindowTree::AddRoot(const ServerWindow* root) { > 220 DCHECK(pending_client_id_ != kInvalidClientId); > 221 > 222 const ClientWindowId client_window_id(pending_client_id_); > 223 DCHECK_EQ(0u, client_id_to_window_id_map_.count(client_window_id)); > 224 > 225 client_id_to_window_id_map_[client_window_id] = root->id(); > 226 window_id_to_client_id_map_[root->id()] = client_window_id; > 227 pending_client_id_ = kInvalidClientId; > (...) > 233 } > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display.cc > File services/ui/ws/display.cc (right): > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display... > services/ui/ws/display.cc:67: DCHECK(window_server_->window_tree_host_factory() > && binding); > On 2017/03/07 18:19:37, kylechar wrote: > > DCHECKs are compiled out in release builds, that might cause problems with the > > if statement. > > BuildBots (including release bots) seem happy, but if you think it looks odd, I > can try it differently. > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display... > services/ui/ws/display.cc:218: > window_manager_display_root_map_[service_manager::mojom::kRootUserID] = > On 2017/03/07 18:19:37, kylechar wrote: > > Once you aren't using WindowManagerDisplayRoot anymore in external mode, you > > also wouldn't use this map, right? > > This is still needed, but should be clean up indeed. Will add a TODO. > > When WindowTree::AddWindow is called, this calling out one the methods in > ws::Display that still need this map to be filled up. > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display... > services/ui/ws/display.cc:229: // Tests can create ws::Display instances, > directly by-passing > On 2017/03/07 18:19:37, kylechar wrote: > > The if (binding_) path is only for tests now? > > Yes and No. > > Yes, because I believe only tests exercise this path today. > No, because one can still call WindowTreeHostFactory::CreateWindowTreeHost > directly, and end up here. > This won't be in 'external window mode' though. > > I left this existing code path untouched not to break existing tests, and if we > decide to change/remove it in the future, we should do it altogether. > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display.h > File services/ui/ws/display.h (right): > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/display... > services/ui/ws/display.h:211: std::unique_ptr<WindowManagerDisplayRoot> > external_window_mode_display_root_; > On 2017/03/07 18:19:37, kylechar wrote: > > |external_mode_root_| maybe? > > Will fix. > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/window_... > File services/ui/ws/window_server.h (right): > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/window_... > services/ui/ws/window_server.h:396: std::unique_ptr<WindowTreeHostFactory> > window_tree_host_factory_; > On 2017/03/07 18:19:37, kylechar wrote: > > There is already a WindowTreeHostFactory in ui::Service for each user? Why add > > this? > > I tried to avoid it, and saw that service has a WindowTreeHostFactory instance. > > I added it to access the WindowTree of this WindowTreeHostFactory, and I could > not find my way to get it through ws::Service. Will see if I can improve it. > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/window_... > File services/ui/ws/window_tree_host_factory_registrar.cc (right): > > https://codereview.chromium.org/2712203002/diff/180001/services/ui/ws/window_... > services/ui/ws/window_tree_host_factory_registrar.cc:26: > std::unique_ptr<WindowTreeHostFactory> host_factory( > On 2017/03/07 18:19:37, kylechar wrote: > > It's preferred to use MakeUnique, so something like: > > > > auto host_factory = base::MakeUnique<WIndowTreeHostFactory>(window_server_, > > user_id_); > > Will fix. > > https://codereview.chromium.org/2712203002/diff/180001/ui/aura/mus/window_tre... > File ui/aura/mus/window_tree_client.h (right): > > https://codereview.chromium.org/2712203002/diff/180001/ui/aura/mus/window_tre... > ui/aura/mus/window_tree_client.h:231: void > ConfigureWindowDataFromServer(WindowTreeHostMus* window_tree_host, > On 2017/03/07 18:19:39, kylechar wrote: > > What does this do? > > There is some 'window data' info coming from the server that we need to > 'configure' WindowTreeHostMus instances with. > > this is already the case in std::unique_ptr<WindowTreeHostMus> > WindowTreeClient::CreateWindowTreeHost [1]. > > [1] > https://cs.chromium.org/chromium/src/ui/aura/mus/window_tree_client.cc?q=wind... > > I just factored the code out into a method, so that we could call in external > window mode too. See the branch in WindowTreeClient::OnEmbed. This CL (for me at least) is pretty difficult to follow whats going on. If it's possible to do it in smaller steps, that might be helpful. The two different ways WindowTreeHostFactory/mojom::WindowTreeHost/DisplayBinding work are especially confusing. Maybe it would be possible to refactor the tests that still use DisplayBinding to not use it and then have DisplayBinding only work with external mode. Then instead of having multiple places with mojom::WindowTreeHost are stored we would only have one? Let's see what sky@ thinks though, since I'm not an OWNER and can't approve this anyways.
I'm not going to be able to get to this for a couple of days. On Wed, Mar 8, 2017 at 1:08 PM, <kylechar@chromium.org> wrote: > On 2017/03/07 20:09:36, tonikitoo wrote: > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/demo/window_tree_data.cc > > File services/ui/demo/window_tree_data.cc (right): > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/demo/window_tree_data.cc#newcode71 > > services/ui/demo/window_tree_data.cc:71: if (window_tree_host) { > > On 2017/03/07 18:19:36, kylechar wrote: > > > When would |window_tree_host| be null? > > > > Before this CL, all the ws::Display creation happened on the server side > first, > > and only upon finishing it, WindowTreeClient::OnEmbed was called. > > In sequence, WindowTreeClient::OnEmbedImpl was called, and then > > WindowTreeClient::CreateWindowTreeHost. > > > > Only at this point, with the WindowClientHostMus instance available, that > > MusDemo::OnEmbed was called, taking the WindowTreeHostMus instance as > parameter. > > > > See > > > https://cs.chromium.org/chromium/src/ui/aura/mus/window_tree_client.cc?q= > WindowTreeClient::OnEmbedImpl+package:%5Echromium$&l=499 > > > > > > In 'external window mode', it is MusDemoExternal that actually creates on > > WindowTreeHostMus instances. > > So, when MusDemo::OnEmbed is called, it does not need to pass an > > WindowTreeHostMus instance because it was already created. > > > > This can be clearer when we introduce ::OnEmbedRootReady to be called > here, > > instead of ::OnEmbed. > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/manifest.json > > File services/ui/manifest.json (right): > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/manifest.json#newcode62 > > services/ui/manifest.json:62: "window_tree_host_factory": [ > > On 2017/03/07 18:19:36, kylechar wrote: > > > Is this still used? > > > > In comment https://codereview.chromium.org/2712203002/#msg12 (from > sky), I > > understood that this code path should still exist for when we are not in > > 'external window mode'. > > > > There are tests that use it, so I suggest that if we are going to > deprecate > it, > > we do in a follow up step. > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/public/interfaces/window_tree_host.mojom > > File services/ui/public/interfaces/window_tree_host.mojom (right): > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/public/interfaces/window_tree_host.mojom#newcode30 > > services/ui/public/interfaces/window_tree_host.mojom:30: // > > On 2017/03/07 18:19:37, kylechar wrote: > > > Delete empty comment line. > > > > Will fix. > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/public/interfaces/window_tree_host.mojom#newcode47 > > services/ui/public/interfaces/window_tree_host.mojom:47: > > CreatePlatformWindow(WindowTreeHost& window_tree_host, uint32 > client_id); > > On 2017/03/07 18:19:36, kylechar wrote: > > > What is |client_id| supposed to be? > > > > WindowHostTreeMus is created on the client side before its associated > root > > ws::ServerWindow. Since it already has an 'id', we need to inform the > server > > about it. > > So, this client_id is the 'id' of this WindowHostTreeMus instance. > > > > Later on, in WindowTree::AddRoot, we associated this 'id' with the > respective > > server window id (see below). > > > > > > 219 void WindowTree::AddRoot(const ServerWindow* root) { > > 220 DCHECK(pending_client_id_ != kInvalidClientId); > > 221 > > 222 const ClientWindowId client_window_id(pending_client_id_); > > 223 DCHECK_EQ(0u, client_id_to_window_id_map_.count(client_window_id)); > > 224 > > 225 client_id_to_window_id_map_[client_window_id] = root->id(); > > 226 window_id_to_client_id_map_[root->id()] = client_window_id; > > 227 pending_client_id_ = kInvalidClientId; > > (...) > > 233 } > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/ws/display.cc > > File services/ui/ws/display.cc (right): > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/ws/display.cc#newcode67 > > services/ui/ws/display.cc:67: > DCHECK(window_server_->window_tree_host_factory() > > && binding); > > On 2017/03/07 18:19:37, kylechar wrote: > > > DCHECKs are compiled out in release builds, that might cause problems > with > the > > > if statement. > > > > BuildBots (including release bots) seem happy, but if you think it looks > odd, > I > > can try it differently. > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/ws/display.cc#newcode218 > > services/ui/ws/display.cc:218: > > window_manager_display_root_map_[service_manager::mojom::kRootUserID] = > > On 2017/03/07 18:19:37, kylechar wrote: > > > Once you aren't using WindowManagerDisplayRoot anymore in external > mode, you > > > also wouldn't use this map, right? > > > > This is still needed, but should be clean up indeed. Will add a TODO. > > > > When WindowTree::AddWindow is called, this calling out one the methods in > > ws::Display that still need this map to be filled up. > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/ws/display.cc#newcode229 > > services/ui/ws/display.cc:229: // Tests can create ws::Display instances, > > directly by-passing > > On 2017/03/07 18:19:37, kylechar wrote: > > > The if (binding_) path is only for tests now? > > > > Yes and No. > > > > Yes, because I believe only tests exercise this path today. > > No, because one can still call WindowTreeHostFactory:: > CreateWindowTreeHost > > directly, and end up here. > > This won't be in 'external window mode' though. > > > > I left this existing code path untouched not to break existing tests, > and if > we > > decide to change/remove it in the future, we should do it altogether. > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/ws/display.h > > File services/ui/ws/display.h (right): > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/ws/display.h#newcode211 > > services/ui/ws/display.h:211: std::unique_ptr<WindowManagerDisplayRoot> > > external_window_mode_display_root_; > > On 2017/03/07 18:19:37, kylechar wrote: > > > |external_mode_root_| maybe? > > > > Will fix. > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/ws/window_server.h > > File services/ui/ws/window_server.h (right): > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/ws/window_server.h#newcode396 > > services/ui/ws/window_server.h:396: std::unique_ptr< > WindowTreeHostFactory> > > window_tree_host_factory_; > > On 2017/03/07 18:19:37, kylechar wrote: > > > There is already a WindowTreeHostFactory in ui::Service for each user? > Why > add > > > this? > > > > I tried to avoid it, and saw that service has a WindowTreeHostFactory > instance. > > > > I added it to access the WindowTree of this WindowTreeHostFactory, and I > could > > not find my way to get it through ws::Service. Will see if I can improve > it. > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/ws/window_tree_host_factory_registrar.cc > > File services/ui/ws/window_tree_host_factory_registrar.cc (right): > > > > > https://codereview.chromium.org/2712203002/diff/180001/ > services/ui/ws/window_tree_host_factory_registrar.cc#newcode26 > > services/ui/ws/window_tree_host_factory_registrar.cc:26: > > std::unique_ptr<WindowTreeHostFactory> host_factory( > > On 2017/03/07 18:19:37, kylechar wrote: > > > It's preferred to use MakeUnique, so something like: > > > > > > auto host_factory = base::MakeUnique<WIndowTreeHostFactory>(window_ > server_, > > > user_id_); > > > > Will fix. > > > > > https://codereview.chromium.org/2712203002/diff/180001/ui/ > aura/mus/window_tree_client.h > > File ui/aura/mus/window_tree_client.h (right): > > > > > https://codereview.chromium.org/2712203002/diff/180001/ui/ > aura/mus/window_tree_client.h#newcode231 > > ui/aura/mus/window_tree_client.h:231: void > > ConfigureWindowDataFromServer(WindowTreeHostMus* window_tree_host, > > On 2017/03/07 18:19:39, kylechar wrote: > > > What does this do? > > > > There is some 'window data' info coming from the server that we need to > > 'configure' WindowTreeHostMus instances with. > > > > this is already the case in std::unique_ptr<WindowTreeHostMus> > > WindowTreeClient::CreateWindowTreeHost [1]. > > > > [1] > > > https://cs.chromium.org/chromium/src/ui/aura/mus/window_tree_client.cc?q= > window_tree_client.cc+package:%5Echromium$&dr&l=413 > > > > I just factored the code out into a method, so that we could call in > external > > window mode too. See the branch in WindowTreeClient::OnEmbed. > > This CL (for me at least) is pretty difficult to follow whats going on. If > it's > possible to do it in smaller steps, that might be helpful. > > The two different ways > WindowTreeHostFactory/mojom::WindowTreeHost/DisplayBinding work are > especially > confusing. Maybe it would be possible to refactor the tests that still use > DisplayBinding to not use it and then have DisplayBinding only work with > external mode. Then instead of having multiple places with > mojom::WindowTreeHost > are stored we would only have one? > > Let's see what sky@ thinks though, since I'm not an OWNER and can't > approve this > anyways. > > https://codereview.chromium.org/2712203002/ > -- 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.
On 2017/03/08 21:08:26, kylechar wrote: > This CL (for me at least) is pretty difficult to follow whats going on. If it's > possible to do it in smaller steps, that might be helpful. > I believe this is what is already tried, keeping MusDemo changes minimal for now and delaying mus_demo_unitests adjustments in https://codereview.chromium.org/2715533005/. One more split that may be done is to move the introduction of WindowTreeHostFactoryRegistrar into a preliminary patch (ignoring WindowTreeHostFactory::CreatePlatformWindow for now). This part should not be controversial and it would help to see which changes are specific to 'external mode'. @Antonio: Do you think you can do that? https://codereview.chromium.org/2712203002/diff/200001/services/ui/public/int... File services/ui/public/interfaces/window_tree_host.mojom (right): https://codereview.chromium.org/2712203002/diff/200001/services/ui/public/int... services/ui/public/interfaces/window_tree_host.mojom:41: WindowTreeClient tree_client); Do you think you can remove the tree_client parameter and used the one registered by WindowTreeHostFactoryRegistrar?
Description was changed from ========== c++ / mojo changes for 'external window mode' This CL is the first step towards having 'external window mode' support in Mus. The existing MusDemo is adapted to test the functionality, and its respective unittests (mus_demo_unittests) is changed to exercise multiple external windows creation in a follow up CL [2]. [2] https://codereview.chromium.org/2715533005/ New code flow: * MusDemoExternal creates and holds a WindowTreeClient instance. * MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory. This "enters" WindowTreeClient in 'external window mode', and creates a WindowTreeHostFactoryRegistrar instance. Through this instance, we call WindowTreeHostFactoryRegistrar::Register, creating WindowTree and WindowTreeHostFactory instances on the server side, and acquiring WindowTree and WindowTreeHostFactory mojo handles on the client side. Note: In external window mode, the WindowTree and WindowTreeHostFactory are unique instances, serving 0 or 'n' WindowTreeHostMus instances. * Yet from MusDemoExternal, WindowTreeHostMus instance(s) are created, in accordance to the --external-window-count parameter. WindowTreeHostMus ctor takes a WindowTreeClient instance which works as a "window tree host delegate". During the creation chain of WindowTreeHostMus, WindowTreeClient::CreateWindowPortForTopLevel is called, and this is what calls WindowTreeHostFactory::CreatePlatformWindow, when in 'external window mode'. That is the entry point of of ws::Display instance creation. At this point of the process we have: ** WindowTreeClient connected to UI service, in 'external window mode'. ** on the server ws::WindowTree and ws::WindowTreeHostFactory instances, and theirs respective mojo handles available on the client side. The MusDemoExternal flow is now similar to what the 'chrome' browser does (see DesktopWindowTreeHostMus being created from BrowserFrameMus, chrome/browser/ui/views/frame/browser_frame_mus.cc). * In the existing creation flow of ws::Display, the server actually calls back to the client via WindlwtreeClient::OnEmbed when it is done. In 'external window mode', this callback is kept, but it passes a 'null' WindowTree object as parameter. Why? Because in ::ConnectViaWindowTreeHostFactory (above) the unique WindowTree instance was already created. Other highlights: * aura/mus/window_tree_client.cc|h Adds a ::ConnectViaWindowTreeHostFactory method that does the WindowTreeHostFactoryRegistrar and WindowTreeHostFactory setup. This is the entry point of the so called 'external window mode'. * services/ui/service.cc|h: CL allows the instantiation of a WindowTreeHostFactoryRegistrar object. WindowTreeHostFactoryRegistrar exposes a ::Register method, that: (a) Binds the mojom::WindowTreeHostFactoryRequest (b) Binds the mojom::WindowTreeRequest (c) Stores the mojom::WindowTreeClientPtr to call out WindowTreeClient later on. (d) A ws::WindowTree instance is also created here. It ensures single WindowTree and WindowTreeClient instances serving various WindowTreeHost instances. The goal of the WindowTreeHostFactoryRegistrar interface is to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be called before WindowTreeHostFactory being properly set up. * services/ui/ws/window_tree_host_factory.cc|h: Adds ::CreatePlatformWindow method so that it triggers the creation of per-outer-window ws::Display instances in external window mode. TODO (in follow up CLs): 1) Factor non-specific WindowManager specific bits out of ws::WindowManagerState, so that event dispatching code can be shared between internal and external window modes. 2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager path. 3) Clean up mus_demo_external (launch instances in parallel?). 4) Clean up WindowTreeClient::OnEmbed flow. BUG=666958 ========== to ========== c++ / mojo changes for 'external window mode' This CL is the first step towards having 'external window mode' support in Mus. The existing MusDemo is adapted to test the functionality, and its respective unittests (mus_demo_unittests) is changed to exercise multiple external windows creation in a follow up CL [2]. Additionally, in [3] tests that assume an 'WindowManager' (and are irrelevant) to 'external window mode', are also being disabled. [2] https://codereview.chromium.org/2715533005/ [3] https://codereview.chromium.org/2740123002/ New code flow: * MusDemoExternal creates and holds a WindowTreeClient instance. Note: In the case of Chrome, it also happens in c/b/ui/views/chrome_browser_main_extra_parts_views.cc, ::ServiceManagerConnectionStarted, when MusClient is created. * MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory. This "enters" WindowTreeClient in 'external window mode', and creates a WindowTreeHostFactoryRegistrar instance. Through this instance, we call WindowTreeHostFactoryRegistrar::Register, creating WindowTree and WindowTreeHostFactory instances on the server side, and acquiring WindowTree and WindowTreeHostFactory mojo handles on the client side. Note: In external window mode, the WindowTree and WindowTreeHostFactory are unique instances, serving 0 or 'n' WindowTreeHostMus instances. * Yet from MusDemoExternal, WindowTreeHostMus instance(s) are created, in accordance to the --external-window-count parameter. WindowTreeHostMus ctor takes a WindowTreeClient instance which works as a "window tree host delegate". Note: In the case of Chrome, this happens from c/b/ui/views/frame/browser_frame_mus.cc, ::GetWidgetParams when DesktopWindowTreeHostMus is created. * During the creation chain of WindowTreeHostMus, WindowTreeClient::CreateWindowPortForTopLevel is called, and this is what calls WindowTreeHostFactory::CreatePlatformWindow, when in 'external window mode'. That is the entry point of of ws::Display instance creation. At this point of the process we have: ** WindowTreeClient connected to UI service, in 'external window mode'. ** on the server ws::WindowTree and ws::WindowTreeHostFactory instances, and theirs respective mojo handles available on the client side. The MusDemoExternal flow is now similar to what the 'chrome' browser does (see DesktopWindowTreeHostMus being created from BrowserFrameMus, chrome/browser/ui/views/frame/browser_frame_mus.cc). * In the existing creation flow of ws::Display, the server actually calls back to the client via WindlwtreeClient::OnEmbed when it is done. In 'external window mode', this callback is kept, but it passes a 'null' WindowTree object as parameter. Why? Because in ::ConnectViaWindowTreeHostFactory (above) the unique WindowTree instance was already created. * services/ui/ws/window_tree_host_factory_registrar.cc|h: This class implements WindowTreeHostFactoryRegistrar class. It exposes a ::Register method, that: (a) Binds the mojom::WindowTreeHostFactoryRequest (b) Binds the mojom::WindowTreeRequest (c) Stores the mojom::WindowTreeClientPtr to call out WindowTreeClient later on. (d) A ws::WindowTree instance is also created here. It ensures single WindowTree and WindowTreeClient instances serving various WindowTreeHost instances. The goal of the WindowTreeHostFactoryRegistrar interface is to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be called before WindowTreeHostFactory being properly set up. * services/ui/ws/window_tree_host_factory.cc|h: Adds ::CreatePlatformWindow method so that it triggers the creation of per-outer-window ws::Display instances in external window mode. TODO (in follow up CLs): 1) Factor non-specific WindowManager specific bits out of ws::WindowManagerState, so that event dispatching code can be shared between internal and external window modes. 2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager path. 3) Clean up mus_demo_external (launch instances in parallel?). 4) Clean up WindowTreeClient::OnEmbed flow. BUG=666958 ==========
Description was changed from ========== c++ / mojo changes for 'external window mode' This CL is the first step towards having 'external window mode' support in Mus. The existing MusDemo is adapted to test the functionality, and its respective unittests (mus_demo_unittests) is changed to exercise multiple external windows creation in a follow up CL [2]. Additionally, in [3] tests that assume an 'WindowManager' (and are irrelevant) to 'external window mode', are also being disabled. [2] https://codereview.chromium.org/2715533005/ [3] https://codereview.chromium.org/2740123002/ New code flow: * MusDemoExternal creates and holds a WindowTreeClient instance. Note: In the case of Chrome, it also happens in c/b/ui/views/chrome_browser_main_extra_parts_views.cc, ::ServiceManagerConnectionStarted, when MusClient is created. * MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory. This "enters" WindowTreeClient in 'external window mode', and creates a WindowTreeHostFactoryRegistrar instance. Through this instance, we call WindowTreeHostFactoryRegistrar::Register, creating WindowTree and WindowTreeHostFactory instances on the server side, and acquiring WindowTree and WindowTreeHostFactory mojo handles on the client side. Note: In external window mode, the WindowTree and WindowTreeHostFactory are unique instances, serving 0 or 'n' WindowTreeHostMus instances. * Yet from MusDemoExternal, WindowTreeHostMus instance(s) are created, in accordance to the --external-window-count parameter. WindowTreeHostMus ctor takes a WindowTreeClient instance which works as a "window tree host delegate". Note: In the case of Chrome, this happens from c/b/ui/views/frame/browser_frame_mus.cc, ::GetWidgetParams when DesktopWindowTreeHostMus is created. * During the creation chain of WindowTreeHostMus, WindowTreeClient::CreateWindowPortForTopLevel is called, and this is what calls WindowTreeHostFactory::CreatePlatformWindow, when in 'external window mode'. That is the entry point of of ws::Display instance creation. At this point of the process we have: ** WindowTreeClient connected to UI service, in 'external window mode'. ** on the server ws::WindowTree and ws::WindowTreeHostFactory instances, and theirs respective mojo handles available on the client side. The MusDemoExternal flow is now similar to what the 'chrome' browser does (see DesktopWindowTreeHostMus being created from BrowserFrameMus, chrome/browser/ui/views/frame/browser_frame_mus.cc). * In the existing creation flow of ws::Display, the server actually calls back to the client via WindlwtreeClient::OnEmbed when it is done. In 'external window mode', this callback is kept, but it passes a 'null' WindowTree object as parameter. Why? Because in ::ConnectViaWindowTreeHostFactory (above) the unique WindowTree instance was already created. * services/ui/ws/window_tree_host_factory_registrar.cc|h: This class implements WindowTreeHostFactoryRegistrar class. It exposes a ::Register method, that: (a) Binds the mojom::WindowTreeHostFactoryRequest (b) Binds the mojom::WindowTreeRequest (c) Stores the mojom::WindowTreeClientPtr to call out WindowTreeClient later on. (d) A ws::WindowTree instance is also created here. It ensures single WindowTree and WindowTreeClient instances serving various WindowTreeHost instances. The goal of the WindowTreeHostFactoryRegistrar interface is to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be called before WindowTreeHostFactory being properly set up. * services/ui/ws/window_tree_host_factory.cc|h: Adds ::CreatePlatformWindow method so that it triggers the creation of per-outer-window ws::Display instances in external window mode. TODO (in follow up CLs): 1) Factor non-specific WindowManager specific bits out of ws::WindowManagerState, so that event dispatching code can be shared between internal and external window modes. 2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager path. 3) Clean up mus_demo_external (launch instances in parallel?). 4) Clean up WindowTreeClient::OnEmbed flow. BUG=666958 ========== to ========== c++ / mojo changes for 'external window mode' This CL is the first step towards having 'external window mode' support in Mus. The existing MusDemo is adapted to test the functionality, and its respective unittests (mus_demo_unittests) is changed to exercise multiple external windows creation in a follow up CL [2]. Additionally, in [3] tests that assume an 'WindowManager' (and hence are irrelevant to 'external window mode'), are also disabled. [2] https://codereview.chromium.org/2715533005/ [3] https://codereview.chromium.org/2740123002/ New code flow: * MusDemoExternal creates and holds a WindowTreeClient instance. Note: In the case of Chrome, it also happens in c/b/ui/views/chrome_browser_main_extra_parts_views.cc, ::ServiceManagerConnectionStarted, when MusClient is created. * MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory. This "enters" WindowTreeClient in 'external window mode', and creates a WindowTreeHostFactoryRegistrar instance. Through this instance, we call WindowTreeHostFactoryRegistrar::Register, creating WindowTree and WindowTreeHostFactory instances on the server side, and acquiring WindowTree and WindowTreeHostFactory mojo handles on the client side. Note: In external window mode, the WindowTree and WindowTreeHostFactory are unique instances, serving 0 or 'n' WindowTreeHostMus instances. * Yet from MusDemoExternal, WindowTreeHostMus instance(s) are created, in accordance to the --external-window-count parameter. WindowTreeHostMus ctor takes a WindowTreeClient instance which works as a "window tree host delegate". Note: In the case of Chrome, this happens from c/b/ui/views/frame/browser_frame_mus.cc, ::GetWidgetParams when DesktopWindowTreeHostMus is created. * During the creation chain of WindowTreeHostMus, WindowTreeClient::CreateWindowPortForTopLevel is called, and this is what calls WindowTreeHostFactory::CreatePlatformWindow, when in 'external window mode'. That is the entry point of of ws::Display instance creation. At this point of the process we have: ** WindowTreeClient connected to UI service, in 'external window mode'. ** on the server ws::WindowTree and ws::WindowTreeHostFactory instances, and theirs respective mojo handles available on the client side. The MusDemoExternal flow is now similar to what the 'chrome' browser does (see DesktopWindowTreeHostMus being created from BrowserFrameMus, chrome/browser/ui/views/frame/browser_frame_mus.cc). * In the existing creation flow of ws::Display, the server actually calls back to the client via WindlwtreeClient::OnEmbed when it is done. In 'external window mode', this callback is kept, but it passes a 'null' WindowTree object as parameter. Why? Because in ::ConnectViaWindowTreeHostFactory (above) the unique WindowTree instance was already created. * services/ui/ws/window_tree_host_factory_registrar.cc|h: This class implements WindowTreeHostFactoryRegistrar class. It exposes a ::Register method, that: (a) Binds the mojom::WindowTreeHostFactoryRequest (b) Binds the mojom::WindowTreeRequest (c) Stores the mojom::WindowTreeClientPtr to call out WindowTreeClient later on. (d) A ws::WindowTree instance is also created here. It ensures single WindowTree and WindowTreeClient instances serving various WindowTreeHost instances. The goal of the WindowTreeHostFactoryRegistrar interface is to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be called before WindowTreeHostFactory being properly set up. * services/ui/ws/window_tree_host_factory.cc|h: Adds ::CreatePlatformWindow method so that it triggers the creation of per-outer-window ws::Display instances in external window mode. TODO (in follow up CLs): 1) Factor non-specific WindowManager specific bits out of ws::WindowManagerState, so that event dispatching code can be shared between internal and external window modes. 2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager path. 3) Clean up mus_demo_external (launch instances in parallel?). 4) Clean up WindowTreeClient::OnEmbed flow. BUG=666958 ==========
Description was changed from ========== c++ / mojo changes for 'external window mode' This CL is the first step towards having 'external window mode' support in Mus. The existing MusDemo is adapted to test the functionality, and its respective unittests (mus_demo_unittests) is changed to exercise multiple external windows creation in a follow up CL [2]. Additionally, in [3] tests that assume an 'WindowManager' (and hence are irrelevant to 'external window mode'), are also disabled. [2] https://codereview.chromium.org/2715533005/ [3] https://codereview.chromium.org/2740123002/ New code flow: * MusDemoExternal creates and holds a WindowTreeClient instance. Note: In the case of Chrome, it also happens in c/b/ui/views/chrome_browser_main_extra_parts_views.cc, ::ServiceManagerConnectionStarted, when MusClient is created. * MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory. This "enters" WindowTreeClient in 'external window mode', and creates a WindowTreeHostFactoryRegistrar instance. Through this instance, we call WindowTreeHostFactoryRegistrar::Register, creating WindowTree and WindowTreeHostFactory instances on the server side, and acquiring WindowTree and WindowTreeHostFactory mojo handles on the client side. Note: In external window mode, the WindowTree and WindowTreeHostFactory are unique instances, serving 0 or 'n' WindowTreeHostMus instances. * Yet from MusDemoExternal, WindowTreeHostMus instance(s) are created, in accordance to the --external-window-count parameter. WindowTreeHostMus ctor takes a WindowTreeClient instance which works as a "window tree host delegate". Note: In the case of Chrome, this happens from c/b/ui/views/frame/browser_frame_mus.cc, ::GetWidgetParams when DesktopWindowTreeHostMus is created. * During the creation chain of WindowTreeHostMus, WindowTreeClient::CreateWindowPortForTopLevel is called, and this is what calls WindowTreeHostFactory::CreatePlatformWindow, when in 'external window mode'. That is the entry point of of ws::Display instance creation. At this point of the process we have: ** WindowTreeClient connected to UI service, in 'external window mode'. ** on the server ws::WindowTree and ws::WindowTreeHostFactory instances, and theirs respective mojo handles available on the client side. The MusDemoExternal flow is now similar to what the 'chrome' browser does (see DesktopWindowTreeHostMus being created from BrowserFrameMus, chrome/browser/ui/views/frame/browser_frame_mus.cc). * In the existing creation flow of ws::Display, the server actually calls back to the client via WindlwtreeClient::OnEmbed when it is done. In 'external window mode', this callback is kept, but it passes a 'null' WindowTree object as parameter. Why? Because in ::ConnectViaWindowTreeHostFactory (above) the unique WindowTree instance was already created. * services/ui/ws/window_tree_host_factory_registrar.cc|h: This class implements WindowTreeHostFactoryRegistrar class. It exposes a ::Register method, that: (a) Binds the mojom::WindowTreeHostFactoryRequest (b) Binds the mojom::WindowTreeRequest (c) Stores the mojom::WindowTreeClientPtr to call out WindowTreeClient later on. (d) A ws::WindowTree instance is also created here. It ensures single WindowTree and WindowTreeClient instances serving various WindowTreeHost instances. The goal of the WindowTreeHostFactoryRegistrar interface is to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be called before WindowTreeHostFactory being properly set up. * services/ui/ws/window_tree_host_factory.cc|h: Adds ::CreatePlatformWindow method so that it triggers the creation of per-outer-window ws::Display instances in external window mode. TODO (in follow up CLs): 1) Factor non-specific WindowManager specific bits out of ws::WindowManagerState, so that event dispatching code can be shared between internal and external window modes. 2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager path. 3) Clean up mus_demo_external (launch instances in parallel?). 4) Clean up WindowTreeClient::OnEmbed flow. BUG=666958 ========== to ========== c++ / mojo changes for 'external window mode' This CL is the first step towards having 'external window mode' support in Mus. The existing MusDemo is adapted to test the functionality, and its respective unittests (mus_demo_unittests) is changed to exercise multiple external windows creation in a follow up CL [2]. Additionally, in [3] tests that assume an 'WindowManager' (and hence are irrelevant to 'external window mode'), are also disabled. [2] https://codereview.chromium.org/2715533005/ [3] https://codereview.chromium.org/2740123002/ New code flow: * MusDemoExternal creates and holds a WindowTreeClient instance. Note: In the case of Chrome, it also happens in c/b/ui/views/chrome_browser_main_extra_parts_views.cc, ::ServiceManagerConnectionStarted, when MusClient is created. * MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory. This "enters" WindowTreeClient in 'external window mode', and creates a WindowTreeHostFactoryRegistrar instance. Through this object, one call WindowTreeHostFactoryRegistrar::Register, creating WindowTree and WindowTreeHostFactory instances on the server side, and acquiring WindowTree and WindowTreeHostFactory mojo handles on the client side. Note: In external window mode, the WindowTree and WindowTreeHostFactory are unique instances, serving 0 or 'n' WindowTreeHostMus instances. * Yet from MusDemoExternal, WindowTreeHostMus instance(s) are created, in accordance to the --external-window-count parameter. Note: In the case of Chrome, this happens from c/b/ui/views/frame/browser_frame_mus.cc, ::GetWidgetParams when DesktopWindowTreeHostMus is created for each outer platform window. WindowTreeHostMus ctor takes a WindowTreeClient instance which works as a "window tree host delegate". * During the creation chain of WindowTreeHostMus, WindowTreeClient::CreateWindowPortForTopLevel is called, and this is what calls the newly added WindowTreeHostFactory::CreatePlatformWindow, when in 'external window mode'. That is the entry point of of ws::Display instance creation. At this point of the process we have: ** WindowTreeClient connected to UI service, in 'external window mode'. ** on the server ws::WindowTree and ws::WindowTreeHostFactory instances, and theirs respective mojo handles available on the client side. * In the existing creation flow of ws::Display, the server actually calls back to the client via WindlwtreeClient::OnEmbed when it is done. In 'external window mode' this callback is kept, but it passes a 'null' WindowTree object as parameter. Why? Because in ::ConnectViaWindowTreeHostFactory (above) the unique WindowTree instance was already created. * services/ui/ws/window_tree_host_factory_registrar.cc|h: This class implements WindowTreeHostFactoryRegistrar class. It exposes a ::Register method, that: (a) Binds the mojom::WindowTreeHostFactoryRequest (b) Binds the mojom::WindowTreeRequest (c) Stores the mojom::WindowTreeClientPtr to call out WindowTreeClient later on. (d) A ws::WindowTree instance is also created here. It ensures single WindowTree and WindowTreeClient instances serving various WindowTreeHost instances. The goal of the WindowTreeHostFactoryRegistrar interface is to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be called before WindowTreeHostFactory being properly set up. * services/ui/ws/window_tree_host_factory.cc|h: Adds ::CreatePlatformWindow method so that it triggers the creation of per-outer-window ws::Display instances in external window mode. TODO (in follow up CLs): 1) Factor non-specific WindowManager specific bits out of ws::WindowManagerState, so that event dispatching code can be shared between internal and external window modes. 2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager path. 3) Clean up mus_demo_external (launch instances in parallel?). 4) Clean up WindowTreeClient::OnEmbed flow. BUG=666958 ==========
Description was changed from ========== c++ / mojo changes for 'external window mode' This CL is the first step towards having 'external window mode' support in Mus. The existing MusDemo is adapted to test the functionality, and its respective unittests (mus_demo_unittests) is changed to exercise multiple external windows creation in a follow up CL [2]. Additionally, in [3] tests that assume an 'WindowManager' (and hence are irrelevant to 'external window mode'), are also disabled. [2] https://codereview.chromium.org/2715533005/ [3] https://codereview.chromium.org/2740123002/ New code flow: * MusDemoExternal creates and holds a WindowTreeClient instance. Note: In the case of Chrome, it also happens in c/b/ui/views/chrome_browser_main_extra_parts_views.cc, ::ServiceManagerConnectionStarted, when MusClient is created. * MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory. This "enters" WindowTreeClient in 'external window mode', and creates a WindowTreeHostFactoryRegistrar instance. Through this object, one call WindowTreeHostFactoryRegistrar::Register, creating WindowTree and WindowTreeHostFactory instances on the server side, and acquiring WindowTree and WindowTreeHostFactory mojo handles on the client side. Note: In external window mode, the WindowTree and WindowTreeHostFactory are unique instances, serving 0 or 'n' WindowTreeHostMus instances. * Yet from MusDemoExternal, WindowTreeHostMus instance(s) are created, in accordance to the --external-window-count parameter. Note: In the case of Chrome, this happens from c/b/ui/views/frame/browser_frame_mus.cc, ::GetWidgetParams when DesktopWindowTreeHostMus is created for each outer platform window. WindowTreeHostMus ctor takes a WindowTreeClient instance which works as a "window tree host delegate". * During the creation chain of WindowTreeHostMus, WindowTreeClient::CreateWindowPortForTopLevel is called, and this is what calls the newly added WindowTreeHostFactory::CreatePlatformWindow, when in 'external window mode'. That is the entry point of of ws::Display instance creation. At this point of the process we have: ** WindowTreeClient connected to UI service, in 'external window mode'. ** on the server ws::WindowTree and ws::WindowTreeHostFactory instances, and theirs respective mojo handles available on the client side. * In the existing creation flow of ws::Display, the server actually calls back to the client via WindlwtreeClient::OnEmbed when it is done. In 'external window mode' this callback is kept, but it passes a 'null' WindowTree object as parameter. Why? Because in ::ConnectViaWindowTreeHostFactory (above) the unique WindowTree instance was already created. * services/ui/ws/window_tree_host_factory_registrar.cc|h: This class implements WindowTreeHostFactoryRegistrar class. It exposes a ::Register method, that: (a) Binds the mojom::WindowTreeHostFactoryRequest (b) Binds the mojom::WindowTreeRequest (c) Stores the mojom::WindowTreeClientPtr to call out WindowTreeClient later on. (d) A ws::WindowTree instance is also created here. It ensures single WindowTree and WindowTreeClient instances serving various WindowTreeHost instances. The goal of the WindowTreeHostFactoryRegistrar interface is to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be called before WindowTreeHostFactory being properly set up. * services/ui/ws/window_tree_host_factory.cc|h: Adds ::CreatePlatformWindow method so that it triggers the creation of per-outer-window ws::Display instances in external window mode. TODO (in follow up CLs): 1) Factor non-specific WindowManager specific bits out of ws::WindowManagerState, so that event dispatching code can be shared between internal and external window modes. 2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager path. 3) Clean up mus_demo_external (launch instances in parallel?). 4) Clean up WindowTreeClient::OnEmbed flow. BUG=666958 ========== to ========== c++ / mojo changes for 'external window mode' This CL is the first step towards having 'external window mode' support in Mus. The existing MusDemo is adapted to test the functionality, and its respective unittests (mus_demo_unittests) is changed to exercise multiple external windows creation in a follow up CL [2]. Additionally, in [3] tests that assume an 'WindowManager' (and hence are irrelevant to 'external window mode'), are also disabled. [2] https://codereview.chromium.org/2715533005/ [3] https://codereview.chromium.org/2740123002/ New code flow: * MusDemoExternal creates and holds a WindowTreeClient instance. Note: In the case of Chrome, it also happens in c/b/ui/views/chrome_browser_main_extra_parts_views.cc, ::ServiceManagerConnectionStarted, when MusClient is created. * MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory. This "enters" WindowTreeClient in 'external window mode', and creates a WindowTreeHostFactoryRegistrar instance. Through this object, WindowTreeHostFactoryRegistrar::Register is called, creating WindowTree and WindowTreeHostFactory instances on the server side, and acquiring WindowTree and WindowTreeHostFactory mojo handles on the client side. At this point of the process we have: ** WindowTreeClient connected to UI service, in 'external window mode'. ** on the server ws::WindowTree and ws::WindowTreeHostFactory instances, and theirs respective mojo handles available on the client side. Note: In external window mode, the WindowTree and WindowTreeHostFactory are unique instances, serving 0 or 'n' WindowTreeHostMus instances. * Yet from MusDemoExternal, WindowTreeHostMus instance(s) are created, in accordance to the value passed to command line parameter --external-window-count, or 1 by default. Note: In the case of Chrome, this happens from c/b/ui/views/frame/browser_frame_mus.cc, ::GetWidgetParams when DesktopWindowTreeHostMus is created for each outer platform window. WindowTreeHostMus ctor takes a WindowTreeClient instance which works as a "window tree host delegate". * During the creation chain of WindowTreeHostMus, WindowTreeClient::CreateWindowPortForTopLevel is called, and this is what calls the newly added method WindowTreeHostFactory::CreatePlatformWindow, when in 'external window mode'. That is the entry point of of ws::Display instance creation. * In the existing creation flow of ws::Display, the server actually calls back to the client via WindlwtreeClient::OnEmbed when it is done. In 'external window mode' this callback is kept, but it passes a 'null' WindowTree object as parameter. Why? Because in ::ConnectViaWindowTreeHostFactory (above) the unique WindowTree instance was already created. * services/ui/ws/window_tree_host_factory_registrar.cc|h: This class implements WindowTreeHostFactoryRegistrar class. It exposes a ::Register method, that: (a) Binds the mojom::WindowTreeHostFactoryRequest (b) Binds the mojom::WindowTreeRequest (c) Stores the mojom::WindowTreeClientPtr to call out WindowTreeClient later on. (d) A ws::WindowTree instance is also created here. It ensures single WindowTree and WindowTreeClient instances serving various WindowTreeHost instances. The goal of the WindowTreeHostFactoryRegistrar interface is to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be called before WindowTreeHostFactory being properly set up. * services/ui/ws/window_tree_host_factory.cc|h: Adds ::CreatePlatformWindow method so that it triggers the creation of per-outer-window ws::Display instances in external window mode. TODO (in follow up CLs): 1) Factor non-specific WindowManager specific bits out of ws::WindowManagerState, so that event dispatching code can be shared between internal and external window modes. 2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager path. 3) Clean up mus_demo_external (launch instances in parallel?). 4) Clean up WindowTreeClient::OnEmbed flow. BUG=666958 ==========
Description was changed from ========== c++ / mojo changes for 'external window mode' This CL is the first step towards having 'external window mode' support in Mus. The existing MusDemo is adapted to test the functionality, and its respective unittests (mus_demo_unittests) is changed to exercise multiple external windows creation in a follow up CL [2]. Additionally, in [3] tests that assume an 'WindowManager' (and hence are irrelevant to 'external window mode'), are also disabled. [2] https://codereview.chromium.org/2715533005/ [3] https://codereview.chromium.org/2740123002/ New code flow: * MusDemoExternal creates and holds a WindowTreeClient instance. Note: In the case of Chrome, it also happens in c/b/ui/views/chrome_browser_main_extra_parts_views.cc, ::ServiceManagerConnectionStarted, when MusClient is created. * MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory. This "enters" WindowTreeClient in 'external window mode', and creates a WindowTreeHostFactoryRegistrar instance. Through this object, WindowTreeHostFactoryRegistrar::Register is called, creating WindowTree and WindowTreeHostFactory instances on the server side, and acquiring WindowTree and WindowTreeHostFactory mojo handles on the client side. At this point of the process we have: ** WindowTreeClient connected to UI service, in 'external window mode'. ** on the server ws::WindowTree and ws::WindowTreeHostFactory instances, and theirs respective mojo handles available on the client side. Note: In external window mode, the WindowTree and WindowTreeHostFactory are unique instances, serving 0 or 'n' WindowTreeHostMus instances. * Yet from MusDemoExternal, WindowTreeHostMus instance(s) are created, in accordance to the value passed to command line parameter --external-window-count, or 1 by default. Note: In the case of Chrome, this happens from c/b/ui/views/frame/browser_frame_mus.cc, ::GetWidgetParams when DesktopWindowTreeHostMus is created for each outer platform window. WindowTreeHostMus ctor takes a WindowTreeClient instance which works as a "window tree host delegate". * During the creation chain of WindowTreeHostMus, WindowTreeClient::CreateWindowPortForTopLevel is called, and this is what calls the newly added method WindowTreeHostFactory::CreatePlatformWindow, when in 'external window mode'. That is the entry point of of ws::Display instance creation. * In the existing creation flow of ws::Display, the server actually calls back to the client via WindlwtreeClient::OnEmbed when it is done. In 'external window mode' this callback is kept, but it passes a 'null' WindowTree object as parameter. Why? Because in ::ConnectViaWindowTreeHostFactory (above) the unique WindowTree instance was already created. * services/ui/ws/window_tree_host_factory_registrar.cc|h: This class implements WindowTreeHostFactoryRegistrar class. It exposes a ::Register method, that: (a) Binds the mojom::WindowTreeHostFactoryRequest (b) Binds the mojom::WindowTreeRequest (c) Stores the mojom::WindowTreeClientPtr to call out WindowTreeClient later on. (d) A ws::WindowTree instance is also created here. It ensures single WindowTree and WindowTreeClient instances serving various WindowTreeHost instances. The goal of the WindowTreeHostFactoryRegistrar interface is to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be called before WindowTreeHostFactory being properly set up. * services/ui/ws/window_tree_host_factory.cc|h: Adds ::CreatePlatformWindow method so that it triggers the creation of per-outer-window ws::Display instances in external window mode. TODO (in follow up CLs): 1) Factor non-specific WindowManager specific bits out of ws::WindowManagerState, so that event dispatching code can be shared between internal and external window modes. 2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager path. 3) Clean up mus_demo_external (launch instances in parallel?). 4) Clean up WindowTreeClient::OnEmbed flow. BUG=666958 ========== to ========== c++ / mojo changes for 'external window mode' This CL is the first step towards having 'external window mode' support in Mus. The existing MusDemo is adapted to test the functionality, and its respective unittests (mus_demo_unittests) is changed to exercise multiple external windows creation in a follow up CL [2]. Additionally, in [3] tests that assume an 'WindowManager' (and hence are irrelevant to 'external window mode'), are also disabled. [2] https://codereview.chromium.org/2715533005/ [3] https://codereview.chromium.org/2740123002/ New code flow: * MusDemoExternal creates and holds a WindowTreeClient instance. Note: In the case of Chrome, it also happens in c/b/ui/views/chrome_browser_main_extra_parts_views.cc, ::ServiceManagerConnectionStarted, when MusClient is created. * MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory. This "enters" WindowTreeClient in 'external window mode', and creates a WindowTreeHostFactoryRegistrar instance. Through this object, WindowTreeHostFactoryRegistrar::Register is called, creating WindowTree and WindowTreeHostFactory instances on the server side, and acquiring WindowTree and WindowTreeHostFactory mojo handles on the client side. At this point of the process we have: ** WindowTreeClient connected to UI service, in 'external window mode'. ** on the server ws::WindowTree and ws::WindowTreeHostFactory instances, and theirs respective mojo handles available on the client side. Note: In external window mode, the WindowTree and WindowTreeHostFactory are unique instances, serving 0 or 'n' WindowTreeHostMus instances. * Yet from MusDemoExternal, WindowTreeHostMus instance(s) are created, in accordance to the value passed to command line parameter --external-window-count, or 1 by default. Note: In the case of Chrome, this happens from c/b/ui/views/frame/browser_frame_mus.cc, ::GetWidgetParams when DesktopWindowTreeHostMus is created for each outer platform window. WindowTreeHostMus ctor takes a WindowTreeClient instance which works as a "window tree host delegate". * During the creation chain of WindowTreeHostMus, WindowTreeClient::CreateWindowPortForTopLevel is called, and this is what calls the newly added method WindowTreeHostFactory::CreatePlatformWindow, when in 'external window mode'. That is the entry point of of ws::Display instance creation. * In the existing creation flow of ws::Display, the server actually calls back to the client via WindlwtreeClient::OnEmbed when it is done. In 'external window mode' this callback is kept, but it passes a 'null' WindowTree object as parameter. Why? Because in ::ConnectViaWindowTreeHostFactory (above) the unique WindowTree instance was already created. * services/ui/ws/window_tree_host_factory_registrar.cc|h: This class implements WindowTreeHostFactoryRegistrar class. It exposes a ::Register method, that: (a) Binds the mojom::WindowTreeHostFactoryRequest (b) Binds the mojom::WindowTreeRequest (c) Stores the mojom::WindowTreeClientPtr to call out WindowTreeClient later on. (d) A ws::WindowTree instance is also created here. It ensures single WindowTree and WindowTreeClient instances serving various WindowTreeHost instances. The goal of the WindowTreeHostFactoryRegistrar interface is to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be called before WindowTreeHostFactory being properly set up. * services/ui/ws/window_tree_host_factory.cc|h: Adds ::CreatePlatformWindow method so that it triggers the creation of per-outer-window ws::Display instances in external window mode. TODO (in follow up CLs): 1) Factor non-specific WindowManager specific bits out of ws::WindowManagerState, so that event dispatching code can be shared between internal and external window modes. 2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager path. 3) Clean up mus_demo_external (launch instances in parallel?). 4) Clean up WindowTreeClient::OnEmbed flow. BUG=666958 ==========
On 2017/03/09 08:14:08, fwang wrote: > One more split that may be done is to move the introduction of > WindowTreeHostFactoryRegistrar into a preliminary patch (ignoring > WindowTreeHostFactory::CreatePlatformWindow for now). This part should not be > controversial and it would help to see which changes are specific to 'external > mode'. @Antonio: Do you think you can do that? So replying to myself, I see that using WindowTreeHostFactoryRegistrar::Register is to prepare a WindowTree for use in external mode when CreatePlatformWindow is called, so probably it does *not* make sense to introduce it in a preliminary CL. I would instead suggest to get rid of WindowTreeHostFactory::CreateWindowTreeHost after the changes to MusDemo, but I see it is still used by window_tree_client_unittest.cc ; so indeed maybe that should be handled later.
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== c++ / mojo changes for 'external window mode' This CL is the first step towards having 'external window mode' support in Mus. The existing MusDemo is adapted to test the functionality, and its respective unittests (mus_demo_unittests) is changed to exercise multiple external windows creation in a follow up CL [2]. Additionally, in [3] tests that assume an 'WindowManager' (and hence are irrelevant to 'external window mode'), are also disabled. [2] https://codereview.chromium.org/2715533005/ [3] https://codereview.chromium.org/2740123002/ New code flow: * MusDemoExternal creates and holds a WindowTreeClient instance. Note: In the case of Chrome, it also happens in c/b/ui/views/chrome_browser_main_extra_parts_views.cc, ::ServiceManagerConnectionStarted, when MusClient is created. * MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory. This "enters" WindowTreeClient in 'external window mode', and creates a WindowTreeHostFactoryRegistrar instance. Through this object, WindowTreeHostFactoryRegistrar::Register is called, creating WindowTree and WindowTreeHostFactory instances on the server side, and acquiring WindowTree and WindowTreeHostFactory mojo handles on the client side. At this point of the process we have: ** WindowTreeClient connected to UI service, in 'external window mode'. ** on the server ws::WindowTree and ws::WindowTreeHostFactory instances, and theirs respective mojo handles available on the client side. Note: In external window mode, the WindowTree and WindowTreeHostFactory are unique instances, serving 0 or 'n' WindowTreeHostMus instances. * Yet from MusDemoExternal, WindowTreeHostMus instance(s) are created, in accordance to the value passed to command line parameter --external-window-count, or 1 by default. Note: In the case of Chrome, this happens from c/b/ui/views/frame/browser_frame_mus.cc, ::GetWidgetParams when DesktopWindowTreeHostMus is created for each outer platform window. WindowTreeHostMus ctor takes a WindowTreeClient instance which works as a "window tree host delegate". * During the creation chain of WindowTreeHostMus, WindowTreeClient::CreateWindowPortForTopLevel is called, and this is what calls the newly added method WindowTreeHostFactory::CreatePlatformWindow, when in 'external window mode'. That is the entry point of of ws::Display instance creation. * In the existing creation flow of ws::Display, the server actually calls back to the client via WindlwtreeClient::OnEmbed when it is done. In 'external window mode' this callback is kept, but it passes a 'null' WindowTree object as parameter. Why? Because in ::ConnectViaWindowTreeHostFactory (above) the unique WindowTree instance was already created. * services/ui/ws/window_tree_host_factory_registrar.cc|h: This class implements WindowTreeHostFactoryRegistrar class. It exposes a ::Register method, that: (a) Binds the mojom::WindowTreeHostFactoryRequest (b) Binds the mojom::WindowTreeRequest (c) Stores the mojom::WindowTreeClientPtr to call out WindowTreeClient later on. (d) A ws::WindowTree instance is also created here. It ensures single WindowTree and WindowTreeClient instances serving various WindowTreeHost instances. The goal of the WindowTreeHostFactoryRegistrar interface is to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be called before WindowTreeHostFactory being properly set up. * services/ui/ws/window_tree_host_factory.cc|h: Adds ::CreatePlatformWindow method so that it triggers the creation of per-outer-window ws::Display instances in external window mode. TODO (in follow up CLs): 1) Factor non-specific WindowManager specific bits out of ws::WindowManagerState, so that event dispatching code can be shared between internal and external window modes. 2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager path. 3) Clean up mus_demo_external (launch instances in parallel?). 4) Clean up WindowTreeClient::OnEmbed flow. BUG=666958 ========== to ========== c++ / mojo changes for 'external window mode' This CL is the first step towards having 'external window mode' support in Mus. The existing MusDemo is adapted to test the functionality, and its respective unit test (mus_demo_unittests) is changed to exercise multiple external windows creation in a follow up CL [2]. [2] https://codereview.chromium.org/2715533005/ New code flow: * MusDemoExternal creates and holds a WindowTreeClient instance. Note: In Chrome, it also happens in c/b/ui/views/chrome_browser_main_extra_parts_views.cc, ::ServiceManagerConnectionStarted, when MusClient is created. * MusDemoExternal calls WindowTreeClient::ConnectViaWindowTreeHostFactory. This "enters" WindowTreeClient in 'external window mode', and creates a WindowTreeHostFactoryRegistrar instance. Through this object, WindowTreeHostFactoryRegistrar::Register is called, creating WindowTree and WindowTreeHostFactory instances on the server side, and acquiring WindowTree and WindowTreeHostFactory mojo handles on the client side. Note: In external window mode, the WindowTree and WindowTreeHostFactory are unique instances, serving 0 or 'n' WindowTreeHostMus instances. * Yet from MusDemoExternal, WindowTreeHostMus instance(s) are created, in accordance to the value passed to command line parameter --external-window-count, or 1 by default. Note: In Chrome, this happens in c/b/ui/views/frame/browser_frame_mus.cc, ::GetWidgetParams when DesktopWindowTreeHostMus is created for each outer platform window. WindowTreeHostMus ctor takes a WindowTreeClient instance which works as a "window tree host delegate". * During the creation chain of WindowTreeHostMus, WindowTreeClient::CreateWindowPortForTopLevel is called, and this is what calls the newly added method WindowTreeHostFactory::CreatePlatformWindow, when in 'external window mode'. That is the entry point of of ws::Display instance creation. * In the existing creation flow of ws::Display, the server actually calls back to the client via WindowTreeClient::OnEmbed when it is done. In 'external window mode' this callback is kept, but it passes a 'null' WindowTree object as parameter. Why? Because in ::ConnectViaWindowTreeHostFactory (above) the unique WindowTree instance was already created. * services/ui/ws/window_tree_host_factory_registrar.cc|h: This class implements WindowTreeHostFactoryRegistrar class. It exposes a ::Register method, that: (a) Binds the mojom::WindowTreeHostFactoryRequest (b) Binds the mojom::WindowTreeRequest (c) Stores the mojom::WindowTreeClientPtr to call out WindowTreeClient later on. (d) A ws::WindowTree instance is also created here. It ensures single WindowTree and WindowTreeClient instances serving various WindowTreeHost instances. The goal of the WindowTreeHostFactoryRegistrar interface is to ensure a WindowTreeHostFactory::CreatePlatformWindow can not be called before WindowTreeHostFactory being properly set up. * services/ui/ws/window_tree_host_factory.cc|h: Adds ::CreatePlatformWindow method so that it triggers the creation of per-outer-window ws::Display instances in external window mode. TODO (in follow up CLs): 1) Factor non-specific WindowManager specific bits out of ws::WindowManagerState, so that event dispatching code can be shared between internal and external window modes. 2) Rename WindowManagerDisplayRoot since it is used in non-WindowManager path. 3) Clean up mus_demo_external (launch instances in parallel?). 4) Clean up WindowTreeClient::OnEmbed flow. 5) Experiment with launching Chrome in external window mode. BUG=666958 ==========
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
