New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 759232 link

Starred by 2 users

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome
Pri: 3
Type: Bug


Show other hotlists

Hotlists containing this issue:
Fixing-touch


Sign in to add a comment

aura: Consolidate DesktopNativeWidgetAura windows with their content.

Project Member Reported by msw@chromium.org, Aug 25 2017

Issue description

aura: Consolidate DesktopNativeWidgetAura windows with their content.

(please forgive inaccuracies or naive/incorrect assumptions here, my understanding is limited)

On desktop platforms (Windows and Linux) we embed "top-level" aura::Windows within other aura::Windows for hosting the window in a desktop environment. We do something similar with Mus/Mash for Chrome OS.

Nesting these windows adds non-obvious complexity in various places, eg.:

1) Transient "top-level" windows (ie. context menus and certain dialogs) use the *parent of the parent window* as the transient parent *for the parent of* the context menu window. ie:
  <A> = <extra aura::window for DesktopNativeWidgetAura/DesktopWindowTreeHost>
    <B> = <chrome/app aura::window with meaningful content "DesktopNativeWidgetAura - content window">
  <C> = <extra aura::window for DesktopNativeWidgetAura/DesktopWindowTreeHost>
    <D> = <menu/dialog aura::window with meaningful content "DesktopNativeWidgetAura - content window">
(in the context of the chrome/app and menu/dialog, there are extra hops to find the transient parent)

2) MusPropertyMirror exists for the sole purpose of copying properties from app-content aura::windows to mash-frame aura::windows and vice versa, so the content window properties are accessible to the app, and the frame window properties are transported/synchronized between clients and the window manager (by MusPropertyMirror).

If it's feasible, consolidating the window hierarchy to avoid these extra windows may simplify development.
Please feel free to make corrections, add context, describe blockers, or express opinions here, thanks.
 

Comment 1 by sky@chromium.org, Aug 25 2017

To be specific, Mike is asking why don't we get rid of DesktopNativeWidgetAura::content_window_ and only use the window from DesktopWindowTreeHost? AFAICT the initial commit of DesktopNativeWidgetAura had content_window_. I can only think of potential problems with getting rid of content_window_, but it's worth seeing if we can in fact consolidate.

Comment 2 by sadrul@chromium.org, Aug 26 2017

Getting rid of |content_window_| would be good, yes! We used to create one more window in DesktopNativeWidgetAura, actually (|content_window_container_|), which was removed a few months back. I wanted to look into removing |content_window_| too, but didn't manage to get back to it. So if we can do that now, that would be pretty neat!

Some possible issues that I can think of:
. Window::GetToplevelWindow() looks at windows with delegates [2]. The root-window created by WindowTreeHost currently never has a delegate [3]. So either GetToplevelWindow() will need to be updated, or the root-window for a DesktopWindowTreeHost will need to use DesktopNativeWidgetAura as the delegate. This may have its own set of issues.

. Tab-casting of a backgrounded tab may need some special attention. The WebContentsViewAura's window is hidden and attached to the root-window for a background tab [4]. If this new hidden window gets added to the actual content window, it should in theory be fine, but something to watch out for.

[1] https://codereview.chromium.org/2669303005
[2] https://cs.chromium.org/chromium/src/ui/aura/window.cc?l=516
[3] https://cs.chromium.org/chromium/src/ui/aura/window_tree_host.cc?type=cs&sq=package:chromium&l=247
[4] https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc?l=40

Comment 3 by ovanieva@google.com, Aug 29 2017

Components: -UI>Shell>WindowManager

Comment 4 by sky@chromium.org, Aug 29 2017

Components: -Internals>WindowsServer Internals>Views

Comment 5 by sadrul@chromium.org, Aug 30 2017

Cc: varkha@chromium.org riajiang@chromium.org

Comment 6 by ovanieva@google.com, Oct 10 2017

sky@ and sadrul@ who can this be assigned to?

Comment 7 by sky@chromium.org, Oct 10 2017

No one is currently working on this, so it should remain unassigned.

Comment 8 by m...@chromium.org, Jan 3 2018

Cc: m...@chromium.org
Components: Internals>Services>Ash
Labels: -Proj-Mustash-Mash
Components: -UI>Aura -Internals>Aura -Internals>Views
Components: -Internals>Services>Ash Internals>Views
This isn't directly related to services>ash. The code Mike filed this against is used on desktops too, and is in views. Views is a part component to tag this against.

Sign in to add a comment