Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 2712203002: c++ / mojo changes for 'external window mode'

Created:
2 years, 8 months ago by tonikitoo
Modified:
2 years, 7 months ago
Reviewers:
rjkroege, kylechar, sky, fwang
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.

Description

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

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+393 lines, -54 lines) Patch
M services/ui/demo/manifest.json View 1 1 chunk +1 line, -1 line 0 comments Download
M services/ui/demo/mus_demo_external.h View 1 2 3 4 7 8 9 1 chunk +0 lines, -2 lines 0 comments Download
M services/ui/demo/mus_demo_external.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +13 lines, -28 lines 0 comments Download
M services/ui/demo/window_tree_data.h View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -1 line 0 comments Download
M services/ui/demo/window_tree_data.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -4 lines 0 comments Download
M services/ui/manifest.json View 1 chunk +3 lines, -0 lines 0 comments Download
M services/ui/public/interfaces/window_tree_host.mojom View 1 2 3 4 5 6 7 8 9 10 1 chunk +22 lines, -0 lines 0 comments Download
M services/ui/service.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +6 lines, -0 lines 0 comments Download
M services/ui/service.cc View 1 2 3 4 5 6 7 8 3 chunks +12 lines, -1 line 0 comments Download
M services/ui/ws/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M services/ui/ws/display.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +7 lines, -1 line 0 comments Download
M services/ui/ws/display.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +36 lines, -4 lines 0 comments Download
M services/ui/ws/window_server.h View 1 2 3 4 5 6 7 8 9 10 5 chunks +16 lines, -0 lines 0 comments Download
M services/ui/ws/window_server.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +10 lines, -4 lines 0 comments Download
M services/ui/ws/window_tree.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +12 lines, -0 lines 0 comments Download
M services/ui/ws/window_tree.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +44 lines, -1 line 0 comments Download
M services/ui/ws/window_tree_host_factory.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -0 lines 0 comments Download
M services/ui/ws/window_tree_host_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +22 lines, -0 lines 0 comments Download
A services/ui/ws/window_tree_host_factory_registrar.h View 1 chunk +39 lines, -0 lines 0 comments Download
A services/ui/ws/window_tree_host_factory_registrar.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +54 lines, -0 lines 0 comments Download
M ui/aura/mus/window_tree_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +12 lines, -0 lines 0 comments Download
M ui/aura/mus/window_tree_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +67 lines, -7 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 72 (52 generated)
tonikitoo
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 ...
2 years, 8 months ago (2017-02-24 21:42:06 UTC) #2
fwang
https://codereview.chromium.org/2712203002/diff/20001/services/ui/demo/mus_demo_external.h File services/ui/demo/mus_demo_external.h (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/demo/mus_demo_external.h#newcode35 services/ui/demo/mus_demo_external.h:35: mojom::WindowTreeHostFactoryPtr window_tree_host_factory_; You can remove window_tree_host_factory_ too, I believe. ...
2 years, 8 months ago (2017-02-27 20:06:50 UTC) #7
fwang
Is this CL still WIP?
2 years, 8 months ago (2017-02-27 20:08:20 UTC) #8
tonikitoo
On 2017/02/27 20:08:20, fwang wrote: Thank you for the review! > Is this CL still ...
2 years, 8 months ago (2017-02-27 20:17:10 UTC) #9
sky
I only looked at the mojom and aura changes. https://codereview.chromium.org/2712203002/diff/20001/services/ui/public/interfaces/window_tree_host.mojom File services/ui/public/interfaces/window_tree_host.mojom (right): https://codereview.chromium.org/2712203002/diff/20001/services/ui/public/interfaces/window_tree_host.mojom#newcode12 services/ui/public/interfaces/window_tree_host.mojom:12: ...
2 years, 8 months ago (2017-02-28 05:02:22 UTC) #12
fwang
I have not tested the patch yet, but here are more thoughts. https://codereview.chromium.org/2712203002/diff/20001/services/ui/public/interfaces/window_tree_host.mojom File services/ui/public/interfaces/window_tree_host.mojom ...
2 years, 8 months ago (2017-02-28 08:23:54 UTC) #13
sky
https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree_client.cc File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree_client.cc#newcode255 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 ...
2 years, 8 months ago (2017-02-28 17:34:10 UTC) #14
kylechar
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.cc#newcode207 services/ui/ws/display.cc:207: void Display::InitWindowManagerDisplayRoots() { In general you probably don't want ...
2 years, 8 months ago (2017-02-28 21:17:17 UTC) #16
tonikitoo
Addressed most of the feedback, however the ::CreateWindowTreeHost method is not changed yet. I left ...
2 years, 8 months ago (2017-03-01 05:05:44 UTC) #17
tonikitoo
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.cc#newcode207 > ...
2 years, 8 months ago (2017-03-01 05:11:29 UTC) #18
fwang
https://codereview.chromium.org/2712203002/diff/40001/services/ui/demo/mus_demo_external.cc File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2712203002/diff/40001/services/ui/demo/mus_demo_external.cc#newcode38 services/ui/demo/mus_demo_external.cc:38: const size_t kNumberOfWindows = 1; Maybe you want to ...
2 years, 8 months ago (2017-03-01 06:38:47 UTC) #23
sky
https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree_client.cc File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree_client.cc#newcode255 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 ...
2 years, 8 months ago (2017-03-01 17:08:38 UTC) #24
fwang
Thank you for updating the patch! IIUC, this CL is mostly for c++ / mojo ...
2 years, 8 months ago (2017-03-07 09:21:25 UTC) #40
tonikitoo
On 2017/03/01 17:08:38, sky wrote: > https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree_client.cc > File ui/aura/mus/window_tree_client.cc (right): > > https://codereview.chromium.org/2712203002/diff/20001/ui/aura/mus/window_tree_client.cc#newcode255 > ...
2 years, 8 months ago (2017-03-07 14:00:50 UTC) #41
kylechar
I've looked over MusDemo and WS code a bit and have some questions. i didn't ...
2 years, 8 months ago (2017-03-07 18:19:44 UTC) #47
tonikitoo
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: > ...
2 years, 8 months ago (2017-03-07 20:09:36 UTC) #48
kylechar
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 > ...
2 years, 8 months ago (2017-03-08 21:08:26 UTC) #55
sky
I'm not going to be able to get to this for a couple of days. ...
2 years, 8 months ago (2017-03-09 03:17:25 UTC) #56
fwang
On 2017/03/08 21:08:26, kylechar wrote: > This CL (for me at least) is pretty difficult ...
2 years, 8 months ago (2017-03-09 08:14:08 UTC) #57
fwang
2 years, 8 months ago (2017-03-13 09:49:35 UTC) #63
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.

Powered by Google App Engine
This is Rietveld 408576698