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

Issue 2622103004: Mus Demo: Demonstrate external window mode (Closed)

Created:
1 year, 9 months ago by fwang
Modified:
1 year, 6 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.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+153 lines, -30 lines) Patch
M services/ui/demo/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +22 lines, -5 lines 0 comments Download
M services/ui/demo/main.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -12 lines 0 comments Download
A + services/ui/demo/main_external.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -3 lines 0 comments Download
A + services/ui/demo/main_internal.cc View 1 2 3 4 5 6 7 8 9 10 11 0 chunks +-1 lines, --1 lines 0 comments Download
M services/ui/demo/manifest.json View 8 1 chunk +1 line, -1 line 0 comments Download
M services/ui/demo/mus_demo.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -2 lines 0 comments Download
M services/ui/demo/mus_demo.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
A services/ui/demo/mus_demo_external.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +41 lines, -0 lines 0 comments Download
A services/ui/demo/mus_demo_external.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +73 lines, -0 lines 0 comments Download
M services/ui/demo/mus_demo_internal.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +6 lines, -2 lines 0 comments Download
M services/ui/demo/mus_demo_internal.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +5 lines, -5 lines 0 comments Download

Messages

Total messages: 58 (25 generated)
fwang
This is a rebase of Tom's https://codereview.chromium.org/2503923003/ It is still work-in-progress and I'll go back ...
1 year, 9 months ago (2017-01-11 17:36:31 UTC) #1
Tom (Use chromium acct)
On 2017/01/11 17:36:31, fwang wrote: > This is a rebase of Tom's https://codereview.chromium.org/2503923003/ > > ...
1 year, 9 months ago (2017-01-11 22:47:00 UTC) #3
fwang
On 2017/01/11 22:47:00, Tom Anderson wrote: > +sky@ can you comment on the above? Additionally, ...
1 year, 9 months ago (2017-01-12 09:51:08 UTC) #4
sky
On Wed, Jan 11, 2017 at 2:46 PM, <thomasanderson@google.com> wrote: > On 2017/01/11 17:36:31, fwang ...
1 year, 9 months ago (2017-01-12 17:25:16 UTC) #5
fwang
Given discussion with sky, I suspect rewriting the patch to work with aura and in ...
1 year, 9 months ago (2017-01-21 14:50:17 UTC) #6
fwang
OK, I've extracted the (hopefully) non-controversial part of mus_demo refactoring in https://codereview.chromium.org/2679213003/
1 year, 8 months ago (2017-02-08 16:12:04 UTC) #7
fwang
So some quick update on this... First, this CL includes https://codereview.chromium.org/2679213003/ and will be easier ...
1 year, 8 months ago (2017-02-09 16:49:39 UTC) #9
rjkroege
https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_demo.cc File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_demo.cc#newcode142 services/ui/demo/mus_demo.cc:142: window_tree_client_ = base::MakeUnique<aura::WindowTreeClient>( I would like mus demo to ...
1 year, 8 months ago (2017-02-09 18:03:53 UTC) #10
kylechar
On 2017/02/09 18:03:53, rjkroege wrote: > https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_demo.cc > File services/ui/demo/mus_demo.cc (right): > > https://codereview.chromium.org/2622103004/diff/80001/services/ui/demo/mus_demo.cc#newcode142 > ...
1 year, 8 months ago (2017-02-09 18:15:06 UTC) #11
Fady Samuel
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_demo.cc ...
1 year, 8 months ago (2017-02-09 18:17:32 UTC) #12
tonikitoo
On 2017/02/09 18:17:32, Fady Samuel wrote: need to be fixed so the window server will ...
1 year, 8 months ago (2017-02-09 18:30:53 UTC) #13
fwang
Thank you everybody for the feedback. Will read this more carefully tomorrow. Just one comment ...
1 year, 8 months ago (2017-02-09 18:50:38 UTC) #14
rjkroege
On 2017/02/09 16:49:39, fwang wrote: > So some quick update on this... > > First, ...
1 year, 8 months ago (2017-02-09 19:39:16 UTC) #15
rjkroege
On 2017/02/09 18:50:38, fwang wrote: > Thank you everybody for the feedback. Will read this ...
1 year, 8 months ago (2017-02-09 19:43:32 UTC) #16
fwang
On 2017/02/09 19:39:16, rjkroege wrote: > On 2017/02/09 16:49:39, fwang wrote: > OnAcceleratedWidgetAvailable is suppose ...
1 year, 8 months ago (2017-02-10 15:52:49 UTC) #24
fwang
1 year, 8 months ago (2017-02-10 15:53:19 UTC) #25
kylechar
On 2017/02/10 15:52:49, fwang wrote: > On 2017/02/09 19:39:16, rjkroege wrote: > > On 2017/02/09 ...
1 year, 8 months ago (2017-02-10 16:12:59 UTC) #26
fwang
On 2017/02/10 16:12:59, kylechar wrote: > This is expected with the current state of the ...
1 year, 8 months ago (2017-02-10 16:58:18 UTC) #27
tonikitoo
good progresses! https://codereview.chromium.org/2622103004/diff/180001/services/ui/demo/manifest_external.json File services/ui/demo/manifest_external.json (right): https://codereview.chromium.org/2622103004/diff/180001/services/ui/demo/manifest_external.json#newcode7 services/ui/demo/manifest_external.json:7: "ui": [ "window_manager", "window_tree_host_factory" ] I think ...
1 year, 8 months ago (2017-02-14 17:19:22 UTC) #29
fwang
https://codereview.chromium.org/2622103004/diff/180001/services/ui/demo/manifest_external.json File services/ui/demo/manifest_external.json (right): https://codereview.chromium.org/2622103004/diff/180001/services/ui/demo/manifest_external.json#newcode7 services/ui/demo/manifest_external.json:7: "ui": [ "window_manager", "window_tree_host_factory" ] On 2017/02/14 17:19:22, tonikitoo ...
1 year, 8 months ago (2017-02-14 17:31:50 UTC) #30
fwang
https://codereview.chromium.org/2622103004/diff/180001/services/ui/demo/manifest_external.json File services/ui/demo/manifest_external.json (right): https://codereview.chromium.org/2622103004/diff/180001/services/ui/demo/manifest_external.json#newcode7 services/ui/demo/manifest_external.json:7: "ui": [ "window_manager", "window_tree_host_factory" ] On 2017/02/14 17:19:22, tonikitoo ...
1 year, 8 months ago (2017-02-15 13:59:52 UTC) #31
fwang
PTAL
1 year, 8 months ago (2017-02-20 15:33:23 UTC) #32
kylechar
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.cc#newcode7 services/ui/demo/main.cc:7: #if defined(USE_OZONE) && !defined(OS_CHROMEOS) It would probably be cleaner ...
1 year, 7 months ago (2017-02-21 15:15:11 UTC) #40
fwang
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.cc#newcode7 ...
1 year, 7 months ago (2017-02-21 17:30:19 UTC) #41
fwang
https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_demo_external.cc File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_demo_external.cc#newcode29 services/ui/demo/mus_demo_external.cc:29: mojom::WindowTreeHostPtr host_; Oops, missing private here :-( Will take ...
1 year, 7 months ago (2017-02-21 17:44:18 UTC) #44
kylechar
https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_demo_external.cc File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_demo_external.cc#newcode18 services/ui/demo/mus_demo_external.cc:18: class WindowTreeDataExternal : public WindowTreeData { WindowTreeDataExternal + kSquareSize ...
1 year, 7 months ago (2017-02-21 18:51:08 UTC) #45
fwang
https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_demo_external.cc File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2622103004/diff/260001/services/ui/demo/mus_demo_external.cc#newcode18 services/ui/demo/mus_demo_external.cc:18: class WindowTreeDataExternal : public WindowTreeData { On 2017/02/21 18:51:08, ...
1 year, 7 months ago (2017-02-21 19:02:21 UTC) #46
kylechar
lgtm with nits. https://codereview.chromium.org/2622103004/diff/280001/services/ui/demo/mus_demo_external.cc File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2622103004/diff/280001/services/ui/demo/mus_demo_external.cc#newcode37 services/ui/demo/mus_demo_external.cc:37: } } // namespace https://codereview.chromium.org/2622103004/diff/280001/services/ui/demo/mus_demo_external.h File ...
1 year, 7 months ago (2017-02-21 19:15:59 UTC) #47
fwang
https://codereview.chromium.org/2622103004/diff/280001/services/ui/demo/mus_demo_external.cc File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2622103004/diff/280001/services/ui/demo/mus_demo_external.cc#newcode37 services/ui/demo/mus_demo_external.cc:37: } On 2017/02/21 19:15:59, kylechar wrote: > } // ...
1 year, 7 months ago (2017-02-21 19:30:04 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2622103004/300001
1 year, 7 months ago (2017-02-21 20:27:20 UTC) #51
commit-bot: I haz the power
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 ...
1 year, 7 months ago (2017-02-21 22:31:18 UTC) #53
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2622103004/300001
1 year, 7 months ago (2017-02-22 06:20:12 UTC) #55
commit-bot: I haz the power
1 year, 7 months ago (2017-02-22 08:13:22 UTC) #58
Message was sent while issue was closed.
Committed patchset #14 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/12d9a31ee89cb70348fd7c0bdc8d...

Powered by Google App Engine
This is Rietveld 408576698