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

Issue 725376 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug

Blocked on:
issue 759206

Blocking:
issue 675413



Sign in to add a comment

AppShell: Let apps launch multiple windows

Project Member Reported by michae...@chromium.org, May 23 2017

Issue description

We should probably support the chrome.windows API for launching additional windows. Currently app_shell ignores requests to open additional windows.
 
Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5ccf8c4d91374d3fd33ea43e0fc74db1a656e42b

commit 5ccf8c4d91374d3fd33ea43e0fc74db1a656e42b
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Tue Aug 08 23:15:39 2017

Remove DesktopController::GetWindowSize()

This removes the ShellNativeAppWindow classes' dependency
on DesktopController without changing their behavior.

Bug:  725376 
Change-Id: I25e30241003acd97fa96a27623c66174ceadab0b
Reviewed-on: https://chromium-review.googlesource.com/602678
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492770}
[modify] https://crrev.com/5ccf8c4d91374d3fd33ea43e0fc74db1a656e42b/extensions/shell/browser/desktop_controller.h
[modify] https://crrev.com/5ccf8c4d91374d3fd33ea43e0fc74db1a656e42b/extensions/shell/browser/shell_desktop_controller_aura.cc
[modify] https://crrev.com/5ccf8c4d91374d3fd33ea43e0fc74db1a656e42b/extensions/shell/browser/shell_desktop_controller_aura.h
[modify] https://crrev.com/5ccf8c4d91374d3fd33ea43e0fc74db1a656e42b/extensions/shell/browser/shell_desktop_controller_mac.h
[modify] https://crrev.com/5ccf8c4d91374d3fd33ea43e0fc74db1a656e42b/extensions/shell/browser/shell_desktop_controller_mac.mm
[modify] https://crrev.com/5ccf8c4d91374d3fd33ea43e0fc74db1a656e42b/extensions/shell/browser/shell_native_app_window.cc
[modify] https://crrev.com/5ccf8c4d91374d3fd33ea43e0fc74db1a656e42b/extensions/shell/browser/shell_native_app_window.h
[modify] https://crrev.com/5ccf8c4d91374d3fd33ea43e0fc74db1a656e42b/extensions/shell/browser/shell_native_app_window_aura.cc
[modify] https://crrev.com/5ccf8c4d91374d3fd33ea43e0fc74db1a656e42b/extensions/shell/browser/shell_native_app_window_aura.h
[modify] https://crrev.com/5ccf8c4d91374d3fd33ea43e0fc74db1a656e42b/extensions/shell/browser/shell_native_app_window_aura_unittest.cc
[modify] https://crrev.com/5ccf8c4d91374d3fd33ea43e0fc74db1a656e42b/extensions/shell/browser/shell_native_app_window_mac.h
[modify] https://crrev.com/5ccf8c4d91374d3fd33ea43e0fc74db1a656e42b/extensions/shell/browser/shell_native_app_window_mac.mm

Is there a design doc for the overall set of changes you're making in this phase of app_shell? I'm a little fuzzy on what it's intended to support in the end.

Cc: r...@chromium.org jamescook@chromium.org
#3: The AppShell for Linux design doc [1] mentions multi-window support as a future goal.

I have a rough class diagram for window management [2] [3].

In terms of behavior, all we need right now is for AppShell to support *one* fullscreen AppWindow per display. So if you have 2 displays and open 2 app windows, you should expect one full-screen app window on each display. I don't have plans for more complicated window management on Linux although it's architecturally easy to add.

[1] https://docs.google.com/document/d/1l4kK0S0fABDWQfGipYy2D8QL8OnXc2rRHWB5tUq-A5o/
[2] https://drive.google.com/open?id=0B6HSBrih6pNUNHFrLXgtM203eEE
[3] as PDF: attached

Window Management.png
302 KB View Download
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/958fdaa238bee3bdf19cee3e6b3027cfc6b44704

commit 958fdaa238bee3bdf19cee3e6b3027cfc6b44704
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Fri Aug 11 19:42:09 2017

AppShell: Create RootWindowController for per-display windows

Create a class, RootWindowController, for managing individual
WindowTreeHosts. Each root window can have its own set of AppWindows.

The ShellDesktopControllerAura creates a RootWindowController at startup
and removes it at shutdown or when it requests to be closed (e.g. when
the user X's out of the root window). ShellDesktopControllerAura is
still responsible for the global desktop environment, e.g. creating the
extensions system, input methods, capture client, etc.

The next CL will allow ShellDesktopControllerAura to create multiple
RootWindowControllers (one per display). The DesktopController will
marshal requests between app APIs and the AppWindows by mapping them to
the appropriate RootWindowController.

For now, AppShell creates a sole RootWindowController at startup and
quits when the window requests to be closed.

Bug:  725376 
Change-Id: I93735a91442e983b2100c4fe0f62d62d3d68a0ef
Reviewed-on: https://chromium-review.googlesource.com/606967
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493833}
[modify] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/BUILD.gn
[modify] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/browser/default_shell_browser_main_delegate.cc
[modify] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/browser/default_shell_browser_main_delegate.h
[add] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/browser/root_window_controller.cc
[add] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/browser/root_window_controller.h
[add] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/browser/root_window_controller_unittest.cc
[modify] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/browser/shell_browser_main_delegate.h
[modify] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/browser/shell_browser_main_parts.cc
[modify] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/browser/shell_desktop_controller_aura.cc
[modify] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/browser/shell_desktop_controller_aura.h
[modify] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/browser/shell_desktop_controller_aura_unittest.cc
[modify] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/browser/shell_screen.cc
[modify] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/browser/shell_screen.h
[modify] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/browser/shell_screen_unittest.cc
[modify] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/test/shell_test_base_aura.cc
[modify] https://crrev.com/958fdaa238bee3bdf19cee3e6b3027cfc6b44704/extensions/shell/test/shell_test_base_aura.h

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da32b6d9f8d382164fe130b2e264e6dbdc4fd488

commit da32b6d9f8d382164fe130b2e264e6dbdc4fd488
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Fri Aug 18 02:36:59 2017

AppShell: Support multiple displays

In AppShell for Aura, use views::CreateDesktopScreen() to create a
Screen capable of handling multiple displays. ShellDesktopControllerAura
will create at most 1 RootWindowController per display. When an app
requests a new window, an AppWindow will be added to whichever
RootWindowController's display best matches the AppWindow's specified
bounds. For a display with no RootWindowController,
ShellDesktopControllerAura will create one first.

On Chrome OS, the same logic is applied, but we continue using
ShellScreen, which only knows about one display.

Bug:  725376 
Change-Id: I4a56a04d29b8dde6dd88b68edbfd7f6f27f5504b
Reviewed-on: https://chromium-review.googlesource.com/614460
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495433}
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/BUILD.gn
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/browser/DEPS
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/browser/desktop_controller.h
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/browser/root_window_controller.cc
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/browser/root_window_controller.h
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/browser/root_window_controller_unittest.cc
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/browser/shell_app_window_client.cc
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/browser/shell_app_window_client_aura.cc
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/browser/shell_app_window_client_mac.mm
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/browser/shell_desktop_controller_aura.cc
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/browser/shell_desktop_controller_aura.h
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/browser/shell_desktop_controller_aura_unittest.cc
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/browser/shell_desktop_controller_mac.h
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/browser/shell_desktop_controller_mac.mm
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/browser/shell_screen.cc
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/test/shell_test_base_aura.cc
[modify] https://crrev.com/da32b6d9f8d382164fe130b2e264e6dbdc4fd488/extensions/shell/test/shell_test_base_aura.h

Cc: e...@chromium.org dlaz@chromium.org
I'm working on the last major part of this (moving AppWindows between displays). This exposes a bug in WindowTreeHostX11: When the WindowTreeHost's X window is resized, X11 gives us new bounds for the WindowTreeHost. But these bounds are relative to that X window, so the position is basically (0, 0). We need to translate this origin into screen coordinates to tell the WindowTreeHost where it actually is on the screen.

I'm trying a fix at https://chromium-review.googlesource.com/630582.
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/08b334142348134e096b8c1ae71f413eeacdf282

commit 08b334142348134e096b8c1ae71f413eeacdf282
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Thu Aug 24 22:19:13 2017

WindowTreeHostX11: Fix wrong origin on resize

Resizing a WindowTreeHostX11's X window by setting its bounds or using
the mouse resets the host's bounds' origin (x, y). The origin should be
relative to the screen origin, but is set to (A, B) where AxB is the
size of the window frame (e.g. titlebar).

Use XTranslateCoordinates to transform the origin into the root X
window's coordinate space. This is the same fix that other X11-based UI
elements use, e.g. DesktopWindowTreeHostX11 (https://goo.gl/CaQo3u).

BUG= 725376 

Change-Id: I5176eb5a5797972c21c7c663ba4b79d287b93d7b
Reviewed-on: https://chromium-review.googlesource.com/630582
Reviewed-by: Elliot Glaysher <erg@chromium.org>
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497212}
[modify] https://crrev.com/08b334142348134e096b8c1ae71f413eeacdf282/ui/aura/window_tree_host_x11.cc

Comment 9 by sadrul@chromium.org, Aug 25 2017

Cc: rjkroege@chromium.org sadrul@chromium.org
Have you thought about using ozone on linux for AppShell? We have X11 (and wayland) backends for ozone. And looking at issue 675413, using ozone will probably make some things easier (since chromeos always uses ozone).
When I was looking into it, I got the impression Ozone wasn't ready for desktop Linux. Is it viable now?

From https://chromium.googlesource.com/chromium/src/+/master/docs/ozone_overview.md:

"Warning: Experimental support for Linux Desktop is available since m57 but upstream support has always been a bit fragile. For now, development continues on the ozone-wayland-dev branch."

erg@ did say WindowTreeHostX11 was basically unused so I'm not surprised it had this bug, and probably has more. Using a common platform would be great. Just don't want to deal with frequent breakages if the platform is still in development.
Ozone isn't ready for linux-desktop yet, but that's because 'views' doesn't do ozone yet (we don't have a DesktopWindowTreeHostOzone, for example). Since app-shell is not using views, you should be able to switch to ozone. The chromeos developers, for example, are using the ozone-x11 backend on the workstations. So ozone-x11 is being used by more people than WindowTreeHostX11. (if app-shell switches to ozone-x11, then I think we can and should rm the x11-specific code from aura).
AppShell actually does use some views code. It's a dependency of some components we use. I'm not sure to what extent it's actually executed in AppShell but it's definitely compiled and linked in.

We also just started using views::CreateDesktopScreen() to get the Screen functionality of DesktopScreenX11. Does Ozone have something similar, e.g. something that populates a Screen's display_list with attached displays? (I couldn't find an actual desktop::Screen implementation for Ozone.)
Blockedon: 759206
A naive grep on //extensions/shell shows only the dependency on CreateDesktopScreen() (which we can probably remove:  issue 756680 ). Are there other parts that have direct views dependency? (I notice that it pulls in views indirectly because of content_shell, but that's more test-y things that I suspect you can get rid if (content_shell does build and work with just aura without views)).

I don't think there's any Screen implementation for ozone. The user (app-shell, in this instance) would need to provide the appropriate implementation. With X11, it would be the refactored desktop-screen used in views.
Status: Fixed (was: Started)
Moved ozone work to issue 806501, closing this bug.

Sign in to add a comment