Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2622103004:
Mus Demo: Demonstrate external window mode (Closed)
|
Created:
3 years, 11 months ago by fwang Modified:
3 years, 8 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), sky, Tom (Use chromium acct), tonikitoo Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMus Demo: Demonstrate external window mode
Previously the only user of mus was ChromeOS, and so Chrome currently
creates all toplevel windows as children of an ash-managed root window.
In this sense, all created windows are 'internal' to this virtual root.
However, as desktop Linux also transitions to using Mus, it is necessary
to support 'external' window mode where toplevel windows are created as
native (x11, wayland, ...) windows.
This CL adjusts the Mus Demo so that non-ChromeOS Ozone platforms work
in external mode. For now, only a single external window is created by
Mus Demo. Supporting multiple external windows will require some code
refactoring in WindowTreeHostFactory that will be handled in a
follow-up CL [1].
The CL also disables an assertion for these non-ChromeOS Ozone platforms
until display & screen managers are refactored to remove assumptions
specific to internal mode [2].
[1] https://codereview.chromium.org/2700493005
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=693081
BUG=666958
TEST=./out/LinuxOzoneX11/mash --service=mus_demo --ozone-platform=x11 or ./out/LinuxOzoneX11/mus_demo_unittests --ozone-platform=x11
Review-Url: https://codereview.chromium.org/2622103004
Cr-Commit-Position: refs/heads/master@{#451922}
Committed: https://chromium.googlesource.com/chromium/src/+/12d9a31ee89cb70348fd7c0bdc8d26f6ddebf597
Patch Set 1 #Patch Set 2 : Rename Register to CreateWindowTree and add a WindowTree& parameter. #Patch Set 3 : Rebase on top of 2679213003 and excluding 2645093003 #
Total comments: 3
Patch Set 4 : Rebasing on top of 2688013003 #Patch Set 5 : External mode: Do not make MusDemo a WindowManager delegate, prevent ScreenManager from launching a… #Patch Set 6 : Comment out DCHECK and adjust TODO's. #Patch Set 7 : Rebase #Patch Set 8 : Add MusDemoExternal #
Total comments: 5
Patch Set 9 : Rebase ; Make external/internal conditional. #Patch Set 10 : Fix comments and rebase. #Patch Set 11 : Only disable the assert in external mode for now. #
Total comments: 13
Patch Set 12 : Address kylechar's feedback #
Total comments: 10
Patch Set 13 : Address more review comments. #
Total comments: 4
Patch Set 14 : Add description of internal VS external modes ; fix namespace comment #
Messages
Total messages: 58 (25 generated)
This is a rebase of Tom's https://codereview.chromium.org/2503923003/ It is still work-in-progress and I'll go back to this tomorrow. I'm wondering about two things, so ideas or suggestions are welcome for them: 1) How to implement MusDemo::WindowTreeDataFromHost. In Tom's commit, each WindowTreeData had a single client which itself was associated to a single host which itself had a single window tree. Hence it was easy to get the data from the root window. Here things are somewhat different. 2) The Mojo and public C++ API for WindowTreeHostFactory to do in order to have multiple host for a single client. Especially, Tom's suggested to add an OUT WindowTree parameter to CreateWindowTreeHost in https://codereview.chromium.org/2503923003/#msg27 although I'm not exactly sure what was the goal (maybe it's related to 1).
thomasanderson@google.com changed reviewers: + sky@chromium.org, thomasanderson@google.com
On 2017/01/11 17:36:31, fwang wrote: > This is a rebase of Tom's https://codereview.chromium.org/2503923003/ > > It is still work-in-progress and I'll go back to this tomorrow. I'm wondering > about two things, so ideas or suggestions are welcome for them: > > 1) How to implement MusDemo::WindowTreeDataFromHost. In Tom's commit, each > WindowTreeData had a single client which itself was associated to a single host > which itself had a single window tree. Hence it was easy to get the data from > the root window. Here things are somewhat different. > > 2) The Mojo and public C++ API for WindowTreeHostFactory to do in order to have > multiple host for a single client. Especially, Tom's suggested to add an OUT > WindowTree parameter to CreateWindowTreeHost in > https://codereview.chromium.org/2503923003/#msg27 although I'm not exactly sure > what was the goal (maybe it's related to 1). +sky@ can you comment on the above?
On 2017/01/11 22:47:00, Tom Anderson wrote: > +sky@ can you comment on the above? Additionally, I also have a TODO in MusDemo::AddWindowTreeHost. Tom's code relied on window-related classes ui namespace (so it was possible to use functions from services/ui/public/cpp/window_tree_host_factory.h) but now we use window-related classes in the aura namespace so I guess the creation is somewhat different.
On Wed, Jan 11, 2017 at 2:46 PM, <thomasanderson@google.com> wrote: > On 2017/01/11 17:36:31, fwang wrote: >> This is a rebase of Tom's https://codereview.chromium.org/2503923003/ >> >> It is still work-in-progress and I'll go back to this tomorrow. I'm >> wondering >> about two things, so ideas or suggestions are welcome for them: >> >> 1) How to implement MusDemo::WindowTreeDataFromHost. In Tom's commit, each >> WindowTreeData had a single client which itself was associated to a single > host >> which itself had a single window tree. Hence it was easy to get the data >> from >> the root window. Here things are somewhat different. I think you'll have to assume OnEmbed() goes with the oldest request to CreateWindowTreeHost. That is, any time you call CreateWindowTreeHost you'll have a WindowTreeHost. The root of that WindowTreeHost is ready in the next call to OnEmbed(). >> 2) The Mojo and public C++ API for WindowTreeHostFactory to do in order to > have >> multiple host for a single client. Especially, Tom's suggested to add an >> OUT >> WindowTree parameter to CreateWindowTreeHost in >> https://codereview.chromium.org/2503923003/#msg27 although I'm not exactly > sure >> what was the goal (maybe it's related to 1). There needs to be a single WindowTree connection for the client. What you have as Register should likely take a WindowTree&, which will be the single WindowTree connection. OnEmbed() will pass in a null WindowTree. -Scott > > +sky@ can you comment on the above? > > https://codereview.chromium.org/2622103004/ -- 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.
Given discussion with sky, I suspect rewriting the patch to work with aura and in particular having a single WindowTree/WindowTreeClient connection would require a bit of refactoring so I'm extracting the parts that is ready in https://codereview.chromium.org/2645093003/ in order to facilitate review.
OK, I've extracted the (hopefully) non-controversial part of mus_demo refactoring in https://codereview.chromium.org/2679213003/
fwang@igalia.com changed reviewers: + kylechar@chromium.org, rjkroege@chromium.org - thomasanderson@google.com
So some quick update on this... First, this CL includes https://codereview.chromium.org/2679213003/ and will be easier to review when 2679213003 is merged. The latest version of the patch adds a --external-window-mode switch to create a ws::Display / ui::aura::WindowTreeHost using the window tree host factory API (instead of using the one created when the window server is started). This is what was discussed here and at BlinkOn. For now, I'm only considering the creation of a single ui::aura::WindowTreeHost, so I'm ignoring potential changes in WindowTreeHostFactory mentioned by sky above. Currently, OnEmbed calls fails because no primary display is available: [FATAL:window_tree_host.cc(40)] Check failed: display.is_valid(). aura::GetDeviceScaleFactorFromDisplay() aura::WindowTreeHost::InitCompositor() aura::WindowTreeHost::InitHost() aura::WindowTreeClient::CreateWindowTreeHost() aura::WindowTreeClient::OnEmbedImpl() aura::WindowTreeClient::OnEmbed() (Note: in internal mode, the primary display is added by MusDemo::OnWmWillCreateDisplay as a result of aura::WindowTreeClient::WmNewDisplayAdded being called) On the window server, the OnEmbed call is performed with the following stack trace: ui::ws::WindowTree::Init() ui::ws::WindowServer::AddTree() ui::ws::WindowServer::EmbedAtWindow() ui::ws::DisplayBindingImpl::CreateWindowTree() ui::ws::Display::InitWindowManagerDisplayRoots() ui::ws::Display::OnAcceleratedWidgetAvailable() ui::ws::PlatformDisplayDefault::OnAcceleratedWidgetAvailable() ui::X11WindowBase::Create() ui::X11WindowBase::Show() ui::ws::PlatformDisplayDefault::Init() ui::ws::Display::Init() ui::ws::WindowTreeHostFactory::CreateWindowTreeHost() In ui::ws::DisplayBindingImpl::CreateWindowTree, ConfigureWindowManager (which needed to call aura::WindowTreeClient::WmNewDisplayAdded) is even executed *after* the tree is embedded. (Note: in internal mode, the display is added following another branch: InitWindowManagerDisplayRoots -> CreateWindowManagerDisplayRootFromFactory -> AddRootForWindowManager -> WmNewDisplayAdded) @sky @rjkroege @kylechar: I'm wondering if registering the primary display before embedding the tree is what we want here? Any suggestion on how to do that? Thanks.
https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_de... services/ui/demo/mus_demo.cc:142: window_tree_client_ = base::MakeUnique<aura::WindowTreeClient>( I would like mus demo to not depend on aura in either external / internal window mode. https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_de... services/ui/demo/mus_demo.cc:146: // TODO: Management of display (CL 2645093003 and 2684623002)? Need a ScreenManagerOzoneExternal I would think. Per discussion in other CL.
On 2017/02/09 18:03:53, rjkroege wrote: > https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_de... > File services/ui/demo/mus_demo.cc (right): > > https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_de... > services/ui/demo/mus_demo.cc:142: window_tree_client_ = > base::MakeUnique<aura::WindowTreeClient>( > I would like mus demo to not depend on aura in either external / internal window > mode. > > https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_de... > services/ui/demo/mus_demo.cc:146: // TODO: Management of display (CL 2645093003 > and 2684623002)? > Need a ScreenManagerOzoneExternal I would think. Per discussion in other CL. The conversation for how external window mode could work with MusDemo is kind of spread across multiple CLs, but here is an example for how I think it could work. 1. MusDemoExternal connects to service:ui via mojom::WindowTreeHostFactory. MusDemoExternal is not a WM and never connects via the mojom::WindowManager. 2. service:ui starts in external window mode. It got a connection from mojom::WindowTreeHostFactory so it knows it’s in external window mode. 3. Service:ui creates a ScreenManagerOzoneExternal. This never calls anything on the ScreenManagerDelegate because that will create a ws::Display. ScreenManager can’t create ws::Displays in external mode. 4. Service:ui creates a ws::Display for the top level Chrome window. This takes a bit of a different code path that is definitely all broken. 5. MusDemoExternal finishes the connection to the WindowTree for the top level Window. The biggest challenge is that there are many assumptions in services/ui/ws/ that a mojom::WindowManager connection exists. There isn’t a WindowManager in external mode because the host operating system plays that role. Those assumptions need to be fixed so the window server will start and run.
On 2017/02/09 18:15:06, kylechar wrote: > On 2017/02/09 18:03:53, rjkroege wrote: > > > https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_de... > > File services/ui/demo/mus_demo.cc (right): > > > > > https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_de... > > services/ui/demo/mus_demo.cc:142: window_tree_client_ = > > base::MakeUnique<aura::WindowTreeClient>( > > I would like mus demo to not depend on aura in either external / internal > window > > mode. > > > > > https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_de... > > services/ui/demo/mus_demo.cc:146: // TODO: Management of display (CL > 2645093003 > > and 2684623002)? > > Need a ScreenManagerOzoneExternal I would think. Per discussion in other CL. > > The conversation for how external window mode could work with MusDemo is kind of > spread across multiple CLs, but here is an example for how I think it could > work. > 1. MusDemoExternal connects to service:ui via mojom::WindowTreeHostFactory. > MusDemoExternal is not a WM and never connects via the mojom::WindowManager. > 2. service:ui starts in external window mode. It got a connection from > mojom::WindowTreeHostFactory so it knows it’s in external window mode. > 3. Service:ui creates a ScreenManagerOzoneExternal. This never calls anything on > the ScreenManagerDelegate because that will create a ws::Display. ScreenManager > can’t create ws::Displays in external mode. > 4. Service:ui creates a ws::Display for the top level Chrome window. This takes > a bit of a different code path that is definitely all broken. > 5. MusDemoExternal finishes the connection to the WindowTree for the top level > Window. > > The biggest challenge is that there are many assumptions in services/ui/ws/ that > a mojom::WindowManager connection exists. There isn’t a WindowManager in > external mode because the host operating system plays that role. Those > assumptions need to be fixed so the window server will start and run. Do we have a crbug for this? Could we please keep the conversations in the crbug so it's easy to track this discussion?
On 2017/02/09 18:17:32, Fady Samuel wrote: need to be fixed so the window server will start and run. > > Do we have a crbug for this? Could we please keep the conversations in the crbug > so it's easy to track this discussion? Lets use https://crbug.com/666958
Thank you everybody for the feedback. Will read this more carefully tomorrow. Just one comment inline regarding one comment by Robert... https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_de... services/ui/demo/mus_demo.cc:142: window_tree_client_ = base::MakeUnique<aura::WindowTreeClient>( On 2017/02/09 18:03:53, rjkroege wrote: > I would like mus demo to not depend on aura in either external / internal window > mode. I'm not sure I understand this comment. Mus demo has already been changed to use aura::WindowTreeClient (in December) and the ui::ws::WindowTreeClient related classes have been removed in https://codereview.chromium.org/2651593002...
On 2017/02/09 16:49:39, fwang wrote: > So some quick update on this... > > First, this CL includes https://codereview.chromium.org/2679213003/ and will be > easier to review when 2679213003 is merged. > > The latest version of the patch adds a --external-window-mode switch to create a > ws::Display / ui::aura::WindowTreeHost using the window tree host factory API > (instead of using the one created when the window server is started). This is > what was discussed here and at BlinkOn. > > For now, I'm only considering the creation of a single ui::aura::WindowTreeHost, > so I'm ignoring potential changes in WindowTreeHostFactory mentioned by sky > above. > > Currently, OnEmbed calls fails because no primary display is available: > > [FATAL:window_tree_host.cc(40)] Check failed: display.is_valid(). > aura::GetDeviceScaleFactorFromDisplay() > aura::WindowTreeHost::InitCompositor() > aura::WindowTreeHost::InitHost() > aura::WindowTreeClient::CreateWindowTreeHost() > aura::WindowTreeClient::OnEmbedImpl() > aura::WindowTreeClient::OnEmbed() > > (Note: in internal mode, the primary display is added by > MusDemo::OnWmWillCreateDisplay as a result of > aura::WindowTreeClient::WmNewDisplayAdded being called) > > On the window server, the OnEmbed call is performed with the following stack > trace: > > ui::ws::WindowTree::Init() > ui::ws::WindowServer::AddTree() > ui::ws::WindowServer::EmbedAtWindow() > ui::ws::DisplayBindingImpl::CreateWindowTree() > ui::ws::Display::InitWindowManagerDisplayRoots() > ui::ws::Display::OnAcceleratedWidgetAvailable() > ui::ws::PlatformDisplayDefault::OnAcceleratedWidgetAvailable() > ui::X11WindowBase::Create() > ui::X11WindowBase::Show() > ui::ws::PlatformDisplayDefault::Init() > ui::ws::Display::Init() > ui::ws::WindowTreeHostFactory::CreateWindowTreeHost() > > In ui::ws::DisplayBindingImpl::CreateWindowTree, ConfigureWindowManager (which > needed to call aura::WindowTreeClient::WmNewDisplayAdded) is even executed > *after* the tree is embedded. > > (Note: in internal mode, the display is added following another branch: > InitWindowManagerDisplayRoots -> CreateWindowManagerDisplayRootFromFactory -> > AddRootForWindowManager -> WmNewDisplayAdded) > > @sky @rjkroege @kylechar: I'm wondering if registering the primary display > before embedding the tree is what we want here? Any suggestion on how to do > that? Thanks. OnAcceleratedWidgetAvailable is suppose to mean that you have an X window available. So you must have a valid display by then for things to be correct. I'd look into why the display is invalid. Sounds like we're not configure it right in mus. I believe that this is because of the asynchrony that internal mode forces on display::Display creation.
On 2017/02/09 18:50:38, fwang wrote: > Thank you everybody for the feedback. Will read this more carefully tomorrow. > Just one comment inline regarding one comment by Robert... > > https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_de... > File services/ui/demo/mus_demo.cc (right): > > https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_de... > services/ui/demo/mus_demo.cc:142: window_tree_client_ = > base::MakeUnique<aura::WindowTreeClient>( > On 2017/02/09 18:03:53, rjkroege wrote: > > I would like mus demo to not depend on aura in either external / internal > window > > mode. > > I'm not sure I understand this comment. Mus demo has already been changed to use > aura::WindowTreeClient (in December) and the ui::ws::WindowTreeClient related > classes have been removed in https://codereview.chromium.org/2651593002... Sorry. I misspoke. You're right. I was wishfully-thinking. Someday, I'd like a program that uses mus based only the mojom API to prove that it's possible.
Description was changed from ========== Demonstrate external-window-mode in mus-demo This CL * Adds a new command line switch to enable external window mode * Modifies mus-demo to use the new flag to create 2 toplevel windows * Modifies WindowTreeHostFactory to allow creating multiples WindowTreeHosts for a single WindowTreeClient. BUG=666958 ========== to ========== Demonstrate external-window-mode in mus-demo BUG=666958 ==========
The CQ bit was checked by fwang@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...
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/09 19:39:16, rjkroege wrote: > On 2017/02/09 16:49:39, fwang wrote: > OnAcceleratedWidgetAvailable is suppose to mean that you have an X window > available. So you must have a valid display by then for things to be correct. > I'd look into why the display is invalid. Sounds like we're not configure it > right in mus. I believe that this is because of the asynchrony that internal > mode forces on display::Display creation. So actually Antonio pointed out yesterday that the code somewhat works with a release build except that we have an extra empty window. I checked today and this extra window is just because we currently use the ScreenManager*Internal that creates an empty display (I updated the patch with a quick hack for now). And indeed the window created by the window tree host factory has the expected behavior (with its spinning square etc). So the next step will be to understand why this debug assert fails...
On 2017/02/10 15:52:49, fwang wrote: > On 2017/02/09 19:39:16, rjkroege wrote: > > On 2017/02/09 16:49:39, fwang wrote: > > OnAcceleratedWidgetAvailable is suppose to mean that you have an X window > > available. So you must have a valid display by then for things to be correct. > > I'd look into why the display is invalid. Sounds like we're not configure it > > right in mus. I believe that this is because of the asynchrony that internal > > mode forces on display::Display creation. > > So actually Antonio pointed out yesterday that the code somewhat works with a > release build except that we have an extra empty window. I checked today and > this extra window is just because we currently use the ScreenManager*Internal > that creates an empty display (I updated the patch with a quick hack for now). > And indeed the window created by the window tree host factory has the expected > behavior (with its spinning square etc). So the next step will be to understand > why this debug assert fails... This is expected with the current state of the code. The assumptions is we are always in internal window mode, so a PlatformWindow (ws::Display->ws::PlatformDisplay->ws::PlatformWindow) is created for each physical display attached. ScreenManagerStubInternal pretends there is one attached physical display, so the WS creates a PlatformWindow for it. The WS needs to not do this. We also assume a ws::Display is the same as a display::Display. This is not the case in external window mode, so that will require more changes.
On 2017/02/10 16:12:59, kylechar wrote: > This is expected with the current state of the code. The assumptions is we are > always in internal window mode, so a PlatformWindow > (ws::Display->ws::PlatformDisplay->ws::PlatformWindow) is created for each > physical display attached. ScreenManagerStubInternal pretends there is one > attached physical display, so the WS creates a PlatformWindow for it. The WS > needs to not do this. > > We also assume a ws::Display is the same as a display::Display. This is not the > case in external window mode, so that will require more changes. Yes, we'll definitely have some refactoring to do to remove assumptions on window mode. Anyway, I checked again the assert and as I said the problem is really that when GetDeviceScaleFactorFromDisplay is called the screen's display list is empty (and the ws did not inform about the existence of any display). However removing the assert makes the code work in debug build too, so it's not so bad :-) BTW, I also experimented with the factory creating two window tree hosts and indeed I get errors as expected by https://codereview.chromium.org/2622103004/#msg5
tonikitoo@igalia.com changed reviewers: + tonikitoo@igalia.com
good progresses! https://codereview.chromium.org/2622103004/diff/180001/services/ui/demo/manif... File services/ui/demo/manifest_external.json (right): https://codereview.chromium.org/2622103004/diff/180001/services/ui/demo/manif... services/ui/demo/manifest_external.json:7: "ui": [ "window_manager", "window_tree_host_factory" ] I think we can remove "window_manager" requirement. https://codereview.chromium.org/2622103004/diff/180001/ui/aura/window_tree_ho... File ui/aura/window_tree_host.cc (right): https://codereview.chromium.org/2622103004/diff/180001/ui/aura/window_tree_ho... ui/aura/window_tree_host.cc:40: // TODO: MusDemo fails in external mode because no display is available. fixing this should be our goal now, so that we unblock this CL.
https://codereview.chromium.org/2622103004/diff/180001/services/ui/demo/manif... File services/ui/demo/manifest_external.json (right): https://codereview.chromium.org/2622103004/diff/180001/services/ui/demo/manif... services/ui/demo/manifest_external.json:7: "ui": [ "window_manager", "window_tree_host_factory" ] On 2017/02/14 17:19:22, tonikitoo wrote: > I think we can remove "window_manager" requirement. Without that I got a Mojo permission error, but I have not checked the details.
https://codereview.chromium.org/2622103004/diff/180001/services/ui/demo/manif... File services/ui/demo/manifest_external.json (right): https://codereview.chromium.org/2622103004/diff/180001/services/ui/demo/manif... services/ui/demo/manifest_external.json:7: "ui": [ "window_manager", "window_tree_host_factory" ] On 2017/02/14 17:19:22, tonikitoo wrote: > I think we can remove "window_manager" requirement. This is obsolete now that we have a single service. https://codereview.chromium.org/2622103004/diff/180001/ui/aura/window_tree_ho... File ui/aura/window_tree_host.cc (right): https://codereview.chromium.org/2622103004/diff/180001/ui/aura/window_tree_ho... ui/aura/window_tree_host.cc:40: // TODO: MusDemo fails in external mode because no display is available. On 2017/02/14 17:19:22, tonikitoo wrote: > fixing this should be our goal now, so that we unblock this CL. Acknowledged.
PTAL
Description was changed from ========== Demonstrate external-window-mode in mus-demo BUG=666958 ========== to ========== Previously the only user of mus was ChromeOS, and so Chrome currently creates all toplevel windows as children of an ash-managed root window. In this sense, all created windows are 'internal' to this virtual root. However, as desktop Linux also transitions to using Mus, it is necessary to support 'external' window mode where toplevel windows are created as native (x11, wayland, ...) windows. This CL adjusts the Mus Demo so that non-ChromeOS Ozone platforms work in external mode. For now, only a single external window is created by Mus Demo. Supporting multiple external windows will require some code refactoring in WindowTreeHostFactory that will be handled in a follow-up CL [1]. The CL also disables an assertion for these non-ChromeOS Ozone platforms until display & screen managers are refactored to remove assumption specific to internal mode [2]. [1] https://codereview.chromium.org/2700493005 [2] https://bugs.chromium.org/p/chromium/issues/detail?id=693081 BUG=666958 TEST=./out/LinuxOzoneX11/mash --service=mus_demo --ozone-platform=x11 or ./out/LinuxOzoneX11/mus_demo_unittests --ozone-platform=x11 ==========
Description was changed from ========== Previously the only user of mus was ChromeOS, and so Chrome currently creates all toplevel windows as children of an ash-managed root window. In this sense, all created windows are 'internal' to this virtual root. However, as desktop Linux also transitions to using Mus, it is necessary to support 'external' window mode where toplevel windows are created as native (x11, wayland, ...) windows. This CL adjusts the Mus Demo so that non-ChromeOS Ozone platforms work in external mode. For now, only a single external window is created by Mus Demo. Supporting multiple external windows will require some code refactoring in WindowTreeHostFactory that will be handled in a follow-up CL [1]. The CL also disables an assertion for these non-ChromeOS Ozone platforms until display & screen managers are refactored to remove assumption specific to internal mode [2]. [1] https://codereview.chromium.org/2700493005 [2] https://bugs.chromium.org/p/chromium/issues/detail?id=693081 BUG=666958 TEST=./out/LinuxOzoneX11/mash --service=mus_demo --ozone-platform=x11 or ./out/LinuxOzoneX11/mus_demo_unittests --ozone-platform=x11 ========== to ========== Previously the only user of mus was ChromeOS, and so Chrome currently creates all toplevel windows as children of an ash-managed root window. In this sense, all created windows are 'internal' to this virtual root. However, as desktop Linux also transitions to using Mus, it is necessary to support 'external' window mode where toplevel windows are created as native (x11, wayland, ...) windows. This CL adjusts the Mus Demo so that non-ChromeOS Ozone platforms work in external mode. For now, only a single external window is created by Mus Demo. Supporting multiple external windows will require some code refactoring in WindowTreeHostFactory that will be handled in a follow-up CL [1]. The CL also disables an assertion for these non-ChromeOS Ozone platforms until display & screen managers are refactored to remove assumptions specific to internal mode [2]. [1] https://codereview.chromium.org/2700493005 [2] https://bugs.chromium.org/p/chromium/issues/detail?id=693081 BUG=666958 TEST=./out/LinuxOzoneX11/mash --service=mus_demo --ozone-platform=x11 or ./out/LinuxOzoneX11/mus_demo_unittests --ozone-platform=x11 ==========
Description was changed from ========== Previously the only user of mus was ChromeOS, and so Chrome currently creates all toplevel windows as children of an ash-managed root window. In this sense, all created windows are 'internal' to this virtual root. However, as desktop Linux also transitions to using Mus, it is necessary to support 'external' window mode where toplevel windows are created as native (x11, wayland, ...) windows. This CL adjusts the Mus Demo so that non-ChromeOS Ozone platforms work in external mode. For now, only a single external window is created by Mus Demo. Supporting multiple external windows will require some code refactoring in WindowTreeHostFactory that will be handled in a follow-up CL [1]. The CL also disables an assertion for these non-ChromeOS Ozone platforms until display & screen managers are refactored to remove assumptions specific to internal mode [2]. [1] https://codereview.chromium.org/2700493005 [2] https://bugs.chromium.org/p/chromium/issues/detail?id=693081 BUG=666958 TEST=./out/LinuxOzoneX11/mash --service=mus_demo --ozone-platform=x11 or ./out/LinuxOzoneX11/mus_demo_unittests --ozone-platform=x11 ========== to ========== Mus Demo: Demonstrate external window mode Previously the only user of mus was ChromeOS, and so Chrome currently creates all toplevel windows as children of an ash-managed root window. In this sense, all created windows are 'internal' to this virtual root. However, as desktop Linux also transitions to using Mus, it is necessary to support 'external' window mode where toplevel windows are created as native (x11, wayland, ...) windows. This CL adjusts the Mus Demo so that non-ChromeOS Ozone platforms work in external mode. For now, only a single external window is created by Mus Demo. Supporting multiple external windows will require some code refactoring in WindowTreeHostFactory that will be handled in a follow-up CL [1]. The CL also disables an assertion for these non-ChromeOS Ozone platforms until display & screen managers are refactored to remove assumptions specific to internal mode [2]. [1] https://codereview.chromium.org/2700493005 [2] https://bugs.chromium.org/p/chromium/issues/detail?id=693081 BUG=666958 TEST=./out/LinuxOzoneX11/mash --service=mus_demo --ozone-platform=x11 or ./out/LinuxOzoneX11/mus_demo_unittests --ozone-platform=x11 ==========
The CQ bit was checked by fwang@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.
https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/main.cc File services/ui/demo/main.cc (right): https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/main.... services/ui/demo/main.cc:7: #if defined(USE_OZONE) && !defined(OS_CHROMEOS) It would probably be cleaner to have a main_internal.cc and main_external.cc after looking at this. https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/mus_d... File services/ui/demo/mus_demo_external.h (right): https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.h:23: void OnStartImpl(std::unique_ptr<aura::WindowTreeClient>& window_tree_client, // MusDemo: https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.h:23: void OnStartImpl(std::unique_ptr<aura::WindowTreeClient>& window_tree_client, I didn't notice this wasn't const in a previous CL. If you're going to be modifying an argument it should be a pointer instead of a reference. Please change them to be pointers or const refs. https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/windo... File services/ui/demo/window_tree_data.h (right): https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/windo... services/ui/demo/window_tree_data.h:26: explicit WindowTreeData(int square_size); What is the difference between the two constructors? Can you add comments. https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/windo... services/ui/demo/window_tree_data.h:27: explicit WindowTreeData(mojom::WindowTreeHostFactory* factory, Only constructor with a single parameter should be explicit. https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/windo... services/ui/demo/window_tree_data.h:48: mojom::WindowTreeHostPtr host_; Is this only needed for external mode? Can we somehow have only shared code in WindowTreeData? https://codereview.chromium.org/2622103004/diff/240001/ui/aura/window_tree_ho... File ui/aura/window_tree_host.cc (right): https://codereview.chromium.org/2622103004/diff/240001/ui/aura/window_tree_ho... ui/aura/window_tree_host.cc:40: // TODO: This assertion fails in external mode because no display is available Removing this DCHECK shouldn't be necessary. You'll need to fix the underlying problem that display::Display objects are not sent to clients in external mode yet. That will be easier to fix after https://codereview.chromium.org/2692863009/ lands.
Thanks for the review. WIll upload a new version soon. https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/main.cc File services/ui/demo/main.cc (right): https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/main.... services/ui/demo/main.cc:7: #if defined(USE_OZONE) && !defined(OS_CHROMEOS) On 2017/02/21 15:15:10, kylechar wrote: > It would probably be cleaner to have a main_internal.cc and main_external.cc > after looking at this. Done. https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/mus_d... File services/ui/demo/mus_demo_external.h (right): https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.h:23: void OnStartImpl(std::unique_ptr<aura::WindowTreeClient>& window_tree_client, On 2017/02/21 15:15:11, kylechar wrote: > I didn't notice this wasn't const in a previous CL. If you're going to be > modifying an argument it should be a pointer instead of a reference. Please > change them to be pointers or const refs. Done. I don't really like the OnStartImpl. Note after https://codereview.chromium.org/2700493005 we could avoid the WindowTreeData param and just return a std::unique_ptr<aura::WindowTreeClient>, so no need to pass ref or pointer as arguments. https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/windo... File services/ui/demo/window_tree_data.h (right): https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/windo... services/ui/demo/window_tree_data.h:26: explicit WindowTreeData(int square_size); On 2017/02/21 15:15:11, kylechar wrote: > What is the difference between the two constructors? Can you add comments. The new one is only used in external mode. I've moved it to WindowTreeDataExternal so that it is clearer. https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/windo... services/ui/demo/window_tree_data.h:27: explicit WindowTreeData(mojom::WindowTreeHostFactory* factory, On 2017/02/21 15:15:11, kylechar wrote: > Only constructor with a single parameter should be explicit. Done. https://codereview.chromium.org/2622103004/diff/240001/services/ui/demo/windo... services/ui/demo/window_tree_data.h:48: mojom::WindowTreeHostPtr host_; On 2017/02/21 15:15:11, kylechar wrote: > Is this only needed for external mode? Can we somehow have only shared code in > WindowTreeData? Yes. All these data will be moved to a WindowTreeDataExternal. https://codereview.chromium.org/2622103004/diff/240001/ui/aura/window_tree_ho... File ui/aura/window_tree_host.cc (right): https://codereview.chromium.org/2622103004/diff/240001/ui/aura/window_tree_ho... ui/aura/window_tree_host.cc:40: // TODO: This assertion fails in external mode because no display is available On 2017/02/21 15:15:11, kylechar wrote: > Removing this DCHECK shouldn't be necessary. You'll need to fix the underlying > problem that display::Display objects are not sent to clients in external mode > yet. That will be easier to fix after > https://codereview.chromium.org/2692863009/ lands. OK, as discussed on IRC, the next patch will make MusExternal create a fake display instead, so that this hack won't be necessary.
The CQ bit was checked by fwang@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...
https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_d... File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:29: mojom::WindowTreeHostPtr host_; Oops, missing private here :-( Will take care of it in the next iteration...
https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_d... File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:18: class WindowTreeDataExternal : public WindowTreeData { WindowTreeDataExternal + kSquareSize should be in an anonymous namespace. https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:45: window_tree_client->reset(new aura::WindowTreeClient( Do something like the following for these? *window_tree_client = base::MakeUnique<aura::WindowTreeClient>(...); https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:55: fake_display_ = base::MakeUnique<display::Display>(0); |fake_display_| doesn't need to be a member variable or unique_ptr. You can just make display::Display here and pass it AddPrimaryDisplay(). It gets copied there and won't be needed after. https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_d... File services/ui/demo/mus_demo_internal.cc (right): https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_d... services/ui/demo/mus_demo_internal.cc:28: window_tree_client->reset( *window_tree_client = base::MakeUnique
https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_d... File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:18: class WindowTreeDataExternal : public WindowTreeData { On 2017/02/21 18:51:08, kylechar wrote: > WindowTreeDataExternal + kSquareSize should be in an anonymous namespace. Done. https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:29: mojom::WindowTreeHostPtr host_; On 2017/02/21 17:44:17, fwang wrote: > Oops, missing private here :-( Will take care of it in the next iteration... Done. https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:45: window_tree_client->reset(new aura::WindowTreeClient( On 2017/02/21 18:51:08, kylechar wrote: > Do something like the following for these? > > *window_tree_client = base::MakeUnique<aura::WindowTreeClient>(...); Mmh, not sure what it did not work when I initially tried. Done now. https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:55: fake_display_ = base::MakeUnique<display::Display>(0); On 2017/02/21 18:51:08, kylechar wrote: > |fake_display_| doesn't need to be a member variable or unique_ptr. You can just > make display::Display here and pass it AddPrimaryDisplay(). It gets copied there > and won't be needed after. Done. https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_d... File services/ui/demo/mus_demo_internal.cc (right): https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_d... services/ui/demo/mus_demo_internal.cc:28: window_tree_client->reset( On 2017/02/21 18:51:08, kylechar wrote: > *window_tree_client = base::MakeUnique Done.
lgtm with nits. https://codereview.chromium.org/2622103004/diff/280001/services/ui/demo/mus_d... File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2622103004/diff/280001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:37: } } // namespace https://codereview.chromium.org/2622103004/diff/280001/services/ui/demo/mus_d... File services/ui/demo/mus_demo_external.h (right): https://codereview.chromium.org/2622103004/diff/280001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.h:17: class MusDemoExternal : public MusDemo { Can you add a class level comment for what external mode is?
https://codereview.chromium.org/2622103004/diff/280001/services/ui/demo/mus_d... File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2622103004/diff/280001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.cc:37: } On 2017/02/21 19:15:59, kylechar wrote: > } // namespace Done. https://codereview.chromium.org/2622103004/diff/280001/services/ui/demo/mus_d... File services/ui/demo/mus_demo_external.h (right): https://codereview.chromium.org/2622103004/diff/280001/services/ui/demo/mus_d... services/ui/demo/mus_demo_external.h:17: class MusDemoExternal : public MusDemo { On 2017/02/21 19:15:59, kylechar wrote: > Can you add a class level comment for what external mode is? Done.
The CQ bit was checked by fwang@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from kylechar@chromium.org Link to the patchset: https://codereview.chromium.org/2622103004/#ps300001 (title: "Add description of internal VS external modes ; fix namespace comment")
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by fwang@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1487744361307060,
"parent_rev": "453d5882079b36fa2ab5b2240b9bcad81428eaa8", "commit_rev":
"12d9a31ee89cb70348fd7c0bdc8d26f6ddebf597"}
Message was sent while issue was closed.
Description was changed from ========== Mus Demo: Demonstrate external window mode Previously the only user of mus was ChromeOS, and so Chrome currently creates all toplevel windows as children of an ash-managed root window. In this sense, all created windows are 'internal' to this virtual root. However, as desktop Linux also transitions to using Mus, it is necessary to support 'external' window mode where toplevel windows are created as native (x11, wayland, ...) windows. This CL adjusts the Mus Demo so that non-ChromeOS Ozone platforms work in external mode. For now, only a single external window is created by Mus Demo. Supporting multiple external windows will require some code refactoring in WindowTreeHostFactory that will be handled in a follow-up CL [1]. The CL also disables an assertion for these non-ChromeOS Ozone platforms until display & screen managers are refactored to remove assumptions specific to internal mode [2]. [1] https://codereview.chromium.org/2700493005 [2] https://bugs.chromium.org/p/chromium/issues/detail?id=693081 BUG=666958 TEST=./out/LinuxOzoneX11/mash --service=mus_demo --ozone-platform=x11 or ./out/LinuxOzoneX11/mus_demo_unittests --ozone-platform=x11 ========== to ========== Mus Demo: Demonstrate external window mode Previously the only user of mus was ChromeOS, and so Chrome currently creates all toplevel windows as children of an ash-managed root window. In this sense, all created windows are 'internal' to this virtual root. However, as desktop Linux also transitions to using Mus, it is necessary to support 'external' window mode where toplevel windows are created as native (x11, wayland, ...) windows. This CL adjusts the Mus Demo so that non-ChromeOS Ozone platforms work in external mode. For now, only a single external window is created by Mus Demo. Supporting multiple external windows will require some code refactoring in WindowTreeHostFactory that will be handled in a follow-up CL [1]. The CL also disables an assertion for these non-ChromeOS Ozone platforms until display & screen managers are refactored to remove assumptions specific to internal mode [2]. [1] https://codereview.chromium.org/2700493005 [2] https://bugs.chromium.org/p/chromium/issues/detail?id=693081 BUG=666958 TEST=./out/LinuxOzoneX11/mash --service=mus_demo --ozone-platform=x11 or ./out/LinuxOzoneX11/mus_demo_unittests --ozone-platform=x11 Review-Url: https://codereview.chromium.org/2622103004 Cr-Commit-Position: refs/heads/master@{#451922} Committed: https://chromium.googlesource.com/chromium/src/+/12d9a31ee89cb70348fd7c0bdc8d... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:300001) as https://chromium.googlesource.com/chromium/src/+/12d9a31ee89cb70348fd7c0bdc8d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
