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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 2 weeks ago by fwang
Modified:
1 month, 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
Commit queue not available (can’t edit this change).

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 ...
4 months, 2 weeks 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/ > > ...
4 months, 2 weeks 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, ...
4 months, 2 weeks 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 ...
4 months, 2 weeks 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 ...
4 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/
3 months, 2 weeks 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 ...
3 months, 2 weeks 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 ...
3 months, 2 weeks ago (2017-02-09 18:03:53 UTC) #10
kylechar (OOO until 5-29)
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 > ...
3 months, 2 weeks 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 ...
3 months, 2 weeks 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 ...
3 months, 2 weeks 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 ...
3 months, 2 weeks 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, ...
3 months, 2 weeks 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 ...
3 months, 2 weeks 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 ...
3 months, 2 weeks ago (2017-02-10 15:52:49 UTC) #24
fwang
3 months, 2 weeks ago (2017-02-10 15:53:19 UTC) #25
kylechar (OOO until 5-29)
On 2017/02/10 15:52:49, fwang wrote: > On 2017/02/09 19:39:16, rjkroege wrote: > > On 2017/02/09 ...
3 months, 2 weeks 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 ...
3 months, 2 weeks 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 ...
3 months, 1 week 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 ...
3 months, 1 week 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 ...
3 months, 1 week ago (2017-02-15 13:59:52 UTC) #31
fwang
PTAL
3 months ago (2017-02-20 15:33:23 UTC) #32
kylechar (OOO until 5-29)
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 ...
3 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 ...
3 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 ...
3 months ago (2017-02-21 17:44:18 UTC) #44
kylechar (OOO until 5-29)
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 ...
3 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, ...
3 months ago (2017-02-21 19:02:21 UTC) #46
kylechar (OOO until 5-29)
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 ...
3 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: > } // ...
3 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
3 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 ...
3 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
3 months ago (2017-02-22 06:20:12 UTC) #55
commit-bot: I haz the power
3 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06