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

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

Created:
10 months, 1 week ago by fwang
Modified:
7 months, 3 weeks 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 ...
10 months, 1 week 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/ > > ...
10 months, 1 week 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, ...
10 months, 1 week 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 ...
10 months, 1 week 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 ...
10 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/
9 months, 1 week 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 ...
9 months, 1 week 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 ...
9 months, 1 week 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 > ...
9 months, 1 week 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 ...
9 months, 1 week 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 ...
9 months, 1 week 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 ...
9 months, 1 week 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, ...
9 months, 1 week 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 ...
9 months, 1 week 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 ...
9 months, 1 week ago (2017-02-10 15:52:49 UTC) #24
fwang
9 months, 1 week 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 ...
9 months, 1 week 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 ...
9 months, 1 week 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 ...
9 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 ...
9 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 ...
9 months ago (2017-02-15 13:59:52 UTC) #31
fwang
PTAL
9 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 ...
8 months, 4 weeks 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 ...
8 months, 4 weeks 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 ...
8 months, 4 weeks 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 ...
8 months, 4 weeks 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, ...
8 months, 4 weeks 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 ...
8 months, 4 weeks 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: > } // ...
8 months, 4 weeks 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
8 months, 4 weeks 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 ...
8 months, 4 weeks 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
8 months, 4 weeks ago (2017-02-22 06:20:12 UTC) #55
commit-bot: I haz the power
8 months, 4 weeks 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 efc10ee0f