Issue 1645503004: [exp] ash: sysui for mash

Issue 1645503004: [exp] ash: sysui for mash (Closed)

Created:
4 years, 10 months ago by sadrul

Modified:
4 years, 10 months ago

Reviewers:
sky

CC:
chromium-reviews, kalyank, sadrul

Base URL:
https://chromium.googlesource.com/chromium/src.git@ash-in-mus

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Patch Set 10 : . #

Created: 4 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+636 lines, -97 lines)			Patch
M	ash/host/ash_window_tree_host.h	View	1 2 3 4 5 6 7 8	2 chunks	+4 lines, -0 lines	0 comments	Download
M	ash/host/ash_window_tree_host.cc	View	1 2 3 4 5 6 7 8	3 chunks	+15 lines, -0 lines	0 comments	Download
M	ash/host/ash_window_tree_host_platform.h	View	1 2 3 4 5 6 7 8	1 chunk	+4 lines, -2 lines	0 comments	Download
A	ash/mus/sysui/BUILD.gn	View	1 2 3 4 5 6 7 8 9	1 chunk	+53 lines, -0 lines	0 comments	Download
A	ash/mus/sysui/DEPS	View	1 2 3 4 5 6 7 8 9	1 chunk	+13 lines, -0 lines	0 comments	Download
A +	ash/mus/sysui/main.cc	View		1 chunk	+2 lines, -2 lines	0 comments	Download
A +	ash/mus/sysui/shell_delegate_mus.h	View	1 2 3 4 5 6 7 8 9	3 chunks	+10 lines, -15 lines	0 comments	Download
A	ash/mus/sysui/shell_delegate_mus.cc	View	1 2 3 4 5 6 7 8 9	1 chunk	+173 lines, -0 lines	0 comments	Download
A +	ash/mus/sysui/stub_context_factory.h	View	1 2 3 4 5 6 7 8 9	2 chunks	+18 lines, -16 lines	0 comments	Download
A	ash/mus/sysui/stub_context_factory.cc	View	1 2 3 4 5 6 7 8 9	1 chunk	+83 lines, -0 lines	0 comments	Download
A +	ash/mus/sysui/sysui_application.h	View		2 chunks	+14 lines, -17 lines	0 comments	Download
A	ash/mus/sysui/sysui_application.cc	View	1 2 3 4 5 6 7 8 9	1 chunk	+185 lines, -0 lines	0 comments	Download
M	ash/root_window_settings.h	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	ash/snap_to_pixel_layout_manager.cc	View	1 2 3 4 5 6	1 chunk	+0 lines, -1 line	0 comments	Download
M	ash/system/web_notification/ash_popup_alignment_delegate.h	View	1 2 3 4 5 6 7	1 chunk	+0 lines, -1 line	0 comments	Download
M	components/mus/ws/window_tree_host_impl.cc	View	1 2 3 4 5 6 7 8	1 chunk	+5 lines, -0 lines	0 comments	Download
M	mash/browser_driver/browser_driver_application_delegate.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	mash/shell/BUILD.gn	View	1 2 3 4 5 6 7 8 9	1 chunk	+1 line, -0 lines	0 comments	Download
M	mash/shell/shell_application_delegate.h	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -0 lines	0 comments	Download
M	mash/shell/shell_application_delegate.cc	View	1 2 3 4 5 6 7 8	2 chunks	+10 lines, -0 lines	0 comments	Download
M	ui/base/user_activity/user_activity_detector.cc	View	1 2 3 4 5 6 7	2 chunks	+1 line, -6 lines	0 comments	Download
M	ui/views/mus/native_widget_mus.h	View	1 2 3 4 5 6 7 8	4 chunks	+4 lines, -2 lines	0 comments	Download
M	ui/views/mus/native_widget_mus.cc	View	1 2 3 4 5 6 7 8	6 chunks	+24 lines, -18 lines	0 comments	Download
M	ui/views/mus/window_manager_connection.h	View	1 2 3 4 5 6 7 8	2 chunks	+3 lines, -3 lines	0 comments	Download
M	ui/views/mus/window_manager_connection.cc	View	1 2 3 4 5 6 7 8	3 chunks	+11 lines, -12 lines	0 comments	Download

Messages

Total messages: 4 (1 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

sky

https://codereview.chromium.org/1645503004/diff/140001/ash/display/screen_ash.cc File ash/display/screen_ash.cc (right): https://codereview.chromium.org/1645503004/diff/140001/ash/display/screen_ash.cc#newcode134 ash/display/screen_ash.cc:134: return GetPrimaryDisplay(); Code like this shouldn't be hit. I ...

4 years, 10 months ago (2016-02-03 22:43:49 UTC) #2

sadrul

4 years, 10 months ago (2016-02-04 00:03:42 UTC) #3

https://codereview.chromium.org/1645503004/diff/140001/ash/display/screen_ash.cc
File ash/display/screen_ash.cc (right):

https://codereview.chromium.org/1645503004/diff/140001/ash/display/screen_ash...
ash/display/screen_ash.cc:134: return GetPrimaryDisplay();
On 2016/02/03 22:43:48, sky wrote:
> Code like this shouldn't be hit. I think we should add logging to make us
> remember to clean up call sites. I would also a DCHECK(in-mus). These comments
> go for all places where you are adding early outs like this.

Acknowledged.

https://codereview.chromium.org/1645503004/diff/140001/ash/host/ash_window_tr...
File ash/host/ash_window_tree_host.cc (right):

https://codereview.chromium.org/1645503004/diff/140001/ash/host/ash_window_tr...
ash/host/ash_window_tree_host.cc:28: AshWindowTreeHost::Factory g_factory;
On 2016/02/03 22:43:48, sky wrote:
> nit: g_factory is file local, so no 'g_'

Acknowledged.

https://codereview.chromium.org/1645503004/diff/140001/ash/host/ash_window_tr...
File ash/host/ash_window_tree_host.h (right):

https://codereview.chromium.org/1645503004/diff/140001/ash/host/ash_window_tr...
ash/host/ash_window_tree_host.h:44: static void SetFactory(const Factory&
factory);
On 2016/02/03 22:43:48, sky wrote:
> This seems ick. Can't we create the create the right one directly when in mus?

We cannot create the right one directly from the mus-app; the AshWindowTreeHost
gets created a few steps deep inside of the call to Shell::CreateInstance():

  ash::AshWindowTreeHost::Create()
  ash::WindowTreeHostManager::AddWindowTreeHostForDisplay()
  ash::WindowTreeHostManager::CreatePrimaryHost()                               

  ash::Shell::Init()
  ash::Shell::CreateInstance()

To create the right one directly from AshWindowTreeHost::Create() would require
the mus-bits to be built into the ash component itself, and I want to avoid
having to do that. Another option would be to introduce
AshWindowTreeHostInitParams::Factory. (although I think it's simpler as it is
now, and it's somewhat similar to
Env::set_context_factory/ViewsDelegate::set_native_widget_factory etc.)

https://codereview.chromium.org/1645503004/diff/140001/ash/mus/sysui/sysui_ap...
File ash/mus/sysui/sysui_application.cc (right):

https://codereview.chromium.org/1645503004/diff/140001/ash/mus/sysui/sysui_ap...
ash/mus/sysui/sysui_application.cc:35: class AshWindowTreeHostMus : public
AshWindowTreeHostOzone {
On 2016/02/03 22:43:48, sky wrote:
> Why do we need this? I thought the model we were going for was only to create
> top level widgets? What is rendered onto this? I was thinking we would have a
> WindowTreeHost implementation that basically does nothing, including no
> rendering.

Yeah. We don't actually need to create a mus::Window for the ash host. I have
removed this, and changed this to use a StubWindow instead. So the ash host
essentially would be completely noop

https://codereview.chromium.org/1645503004/diff/140001/ash/mus/sysui/sysui_ap...
ash/mus/sysui/sysui_application.cc:55: class ScopedObserver {
On 2016/02/03 22:43:48, sky wrote:
> Nit: name this better, maybe ScopedAshEnvObserver

Acknowledged.

https://codereview.chromium.org/1645503004/diff/140001/ash/mus/sysui/sysui_ap...
ash/mus/sysui/sysui_application.cc:86: ScopedObserver obs(this);
On 2016/02/03 22:43:48, sky wrote:
> Instead of digging out the factory and all that, could you call to
> WindowManagerConnection directly to create the widget and get the root_window
> from it?

That would be simpler, yeah. We can avoid this ScopedEnvObserver thingy. Will
do.

> Also, you'll need to deal with child widgets differently.

https://codereview.chromium.org/1645503004/diff/140001/components/mus/public/...
File components/mus/public/cpp/lib/window_tree_client_impl.cc (right):

https://codereview.chromium.org/1645503004/diff/140001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:564: if
(!in_flight_map_.count(change_id))  // XXX(sad): ??
On 2016/02/03 22:43:48, sky wrote:
> This early out indicates something is wrong. How are you hitting this?

I have no idea why I am hitting this, but I hit this when I start up with the
rest of the patch. I haven't dug into it yet.

The various pieces have landed. Closing this one.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages