New issue
Advanced search Search tips

Issue 826569 link

Starred by 2 users

Issue metadata

Status: WontFix
Owner:
Closed: Aug 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Deprecate views::Widget::InitParams::context in favor of target_display_id

Project Member Reported by jamescook@chromium.org, Mar 28 2018

Issue description

There are lots of places in //chrome/browser that call ash::Shell::GetPrimaryRootWindow() or ash::Shell::GetRootWindowForNewWindows() just to set InitParams::context. That won't work in go/mustash (where the Chrome OS "ash" code runs in its own process and the browser doesn't have the entire window tree).

|context| was added for Windows 8 Metro support to make sure new widgets opened on the correct desktop (metro-side ash vs. normal Windows):
https://codereview.chromium.org/11364053/

It got reused for multi-display support in Chrome OS, to ensure windows opened on the right display. However, the Windows code is all gone. Many of the ash call sites use it incorrectly (they just specify primary root window). Ash also has a fallback code path that uses the RootWindowForNewWindows.

I'd like to replace "context" with "target_display_id". I'd also like to make target_display_id optional, and if you don't specify it then the window is placed on display of the RootWindowForNewWindows. If you want the window on the primary display then you can get the primary display id from display::Screen / display::Display.

The migration path would be:
* Make "context" optional (remove the DCHECK that a widget has either a parent or a context) and add target_display_id
* Audit references to "context" and convert to target display
* Remove "context"

 
Cc: est...@chromium.org
estade, any thoughts here based on your recent window positioning work?

We need something like this in several places:
chromeos/certificate_provider/pin_dialog_manager.cc	ash/shell.h	context window
chromeos/power/idle_action_warning_dialog_view.cc	ash/shell.h	context window
ui/views/screen_capture_notification_ui_views.cc	ash/shell.h	context window
ui/views/task_manager_view.cc	ash/wm/window_util.h	context window

Comment 2 by est...@chromium.org, Mar 30 2018

The migration plan sounds good. The general solution seems like the same thing needed for WindowSizer(Ash) where we use GetRootWindowForNewWindows when really what we need is GetDisplayForNewWindows. 

Where do you propose to get the display id from? In Display related code and WindowTreeHostManager I see notions of "active" (which doesn't appear to be unique) and "primary" but not "home for new windows"/"currently being interacted with". Should this concept be added to DisplayManager? Something in Ash (currently it's the shell but it could be some other object) would push changes to DisplayManager when window activation changes.
Status: Started (was: Assigned)
I've been looking into this all morning and it's not clear I'm going to be able to get rid of |context|. It turns out to be used on desktop-aura (Windows and Linux) and is difficult to remove in general.

For display_id I was thinking you could get the primary if you wanted that, or leave it kInvalidDisplay (-1) if you wanted the default display for new windows, at least on ash.

I'm still investigating making |context| optional.

Comment 4 by est...@chromium.org, Mar 30 2018

The window sizer code wants to know the actual display (or at least its work area bounds) so there it's not enough to have a boolean "primary" vs "for new windows". On desktop this is fudged by just looking for the display of the last active browser window; on Chrome OS this would get us pretty far but of course misses out on non-browser windows.
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 2 2018

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

commit 27761e38bec7cf9179ba5a7c78c86ca182fe2729
Author: James Cook <jamescook@chromium.org>
Date: Mon Apr 02 21:01:25 2018

cros: Change default display for new windows without parent or context

Make views::Widget::InitParams::context optional. This cuts some
dependencies between //chrome/browser/ui and //ash, which we need to
do for go/mustash.

Change the default display for windows that supply neither parent nor
context to the "root window for new windows" (i.e. the display where
the user last activated a window). The old behavior was to always use
the primary display. The new behavior is consistent with mash.

Fix a couple cases where windows were defaulting to the primary
display when they should have been using the display for new windows
(e.g. task manager when all browsers are closed).

Bug:  826569 
Test: covered by tests
Change-Id: I4571c3c7066b4247559d43e7b1192ef8411f8919
Reviewed-on: https://chromium-review.googlesource.com/988732
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547515}
[modify] https://crrev.com/27761e38bec7cf9179ba5a7c78c86ca182fe2729/chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc
[modify] https://crrev.com/27761e38bec7cf9179ba5a7c78c86ca182fe2729/chrome/browser/chromeos/power/idle_action_warning_dialog_view.cc
[modify] https://crrev.com/27761e38bec7cf9179ba5a7c78c86ca182fe2729/chrome/browser/ui/views/chrome_views_delegate_chromeos.cc
[modify] https://crrev.com/27761e38bec7cf9179ba5a7c78c86ca182fe2729/chrome/browser/ui/views/frame/browser_frame_ash.cc
[modify] https://crrev.com/27761e38bec7cf9179ba5a7c78c86ca182fe2729/chrome/browser/ui/views/frame/browser_frame_ash.h
[modify] https://crrev.com/27761e38bec7cf9179ba5a7c78c86ca182fe2729/chrome/browser/ui/views/frame/browser_frame_mus.cc
[modify] https://crrev.com/27761e38bec7cf9179ba5a7c78c86ca182fe2729/chrome/browser/ui/views/screen_capture_notification_ui_views.cc
[modify] https://crrev.com/27761e38bec7cf9179ba5a7c78c86ca182fe2729/chrome/browser/ui/views/task_manager_view.cc
[modify] https://crrev.com/27761e38bec7cf9179ba5a7c78c86ca182fe2729/ui/views/widget/desktop_aura/desktop_native_widget_aura.h
[modify] https://crrev.com/27761e38bec7cf9179ba5a7c78c86ca182fe2729/ui/views/widget/native_widget_aura.cc
[modify] https://crrev.com/27761e38bec7cf9179ba5a7c78c86ca182fe2729/ui/views/widget/widget.h

Project Member

Comment 6 by bugdroid1@chromium.org, Apr 3 2018

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

commit 8cce6adb1fe4527bba774e36e2ee57feb255b035
Author: James Cook <jamescook@chromium.org>
Date: Tue Apr 03 23:45:47 2018

cros: Clean up Widget::InitParams::context usage in //ash

views::Widget::InitParams::context is not required on Chrome OS. Remove
places that set it unnecessarily in //ash.

Make ash_unittests provide a default context in ViewsDelegate, just
like Chrome does in ChromeViewsDelegate.

Clean up some CreateTestWindow / CreateTestWidget functions.

Clean up a couple cases where windows were being reparented
unnecessarily in tests.

Bug:  826569 
Test: ash_unittests
Change-Id: I5d3cfa4501a826357615e5a42564ac2b5e9a2a19
Reviewed-on: https://chromium-review.googlesource.com/988939
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547871}
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/accelerators/accelerator_controller_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/autoclick/autoclick_controller.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/display/shared_display_edge_indicator.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/drag_drop/drag_drop_interactive_uitest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/extended_desktop_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/focus_cycler_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/frame/caption_buttons/frame_size_button_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/frame/custom_frame_view_ash_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/login/ui/login_test_base.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/root_window_controller_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/shelf/shelf_layout_manager_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/shelf/shelf_tooltip_manager_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/shelf/shelf_widget_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/shell/window_type_launcher.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/shell_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/test/ash_test_helper_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/test/ash_test_views_delegate.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/tooltips/tooltip_controller_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/wm/immersive_fullscreen_controller_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/wm/overview/cleanup_animation_observer_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/wm/overview/window_selector_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/wm/tablet_mode/tablet_mode_window_manager_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/wm/test_child_modal_parent.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/wm/test_child_modal_parent.h
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/wm/window_modality_controller_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/wm/window_positioner_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/ash/wm/workspace/workspace_layout_manager_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc
[modify] https://crrev.com/8cce6adb1fe4527bba774e36e2ee57feb255b035/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc

Labels: -Proj-Mustash-Mash
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 25 2018

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

commit 36fade6abd290d8dab5952dc996cf466a8d9db6f
Author: James Cook <jamescook@chromium.org>
Date: Wed Apr 25 23:31:19 2018

cros: Always show kiosk usb update notification on primary display

This eliminates a call into ash::Shell::GetRootWindowForNewWindows,
which we need to do for out-of-process ash (go/mustash).

The existing code looks like it was just copied from another place
that shows a similar notification. Regardless, the primary display
should be fine.

Bug:  372857 ,  826569 
Test: browser_tests KioskUpdateTest.*
Change-Id: Iebe8a3a614bdb346fc30f8baab8b77e24ad466c2
Reviewed-on: https://chromium-review.googlesource.com/1029115
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553821}
[modify] https://crrev.com/36fade6abd290d8dab5952dc996cf466a8d9db6f/chrome/browser/chromeos/ui/kiosk_external_update_notification.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 7

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

commit 05b3a3f7cada9f1fe67ef2ff4f73f013525975f3
Author: James Cook <jamescook@chromium.org>
Date: Tue Aug 07 00:38:55 2018

Add Screen::GetDisplayForNewWindows to support remote mojo apps

Chrome OS supports the concept of a "root window for new windows",
which allows new windows to be automatically placed on the correct
display. This knowledge lives in ash, currently inside the browser
process.

Remote processes that use views::Widget (e.g. the shortcut viewer app)
sometimes need to know what window they are going to open on,
for example to do initial bounds computations.

Change Screen to provide set and get for the display for new windows.
Use mojo to send the information to other processes via ScreenProvider
(on the ash side) and ScreenMus (on the remote side). Send the display
id as part of display updates so that ScreenMus gets it on startup,
and also so updates are atomic with other display changes.

Once this lands we'll be able to port browser windows to use this
mechanism and delete the ash::ShellState / chrome ShellStateClient
code I built as a one-off for browser windows under mash.

Bug:  867559 ,  764009 ,  826569 
Test: added unit tests
Change-Id: I6b5beb2610243f30072c3d73180b3ce21a310ca6
Reviewed-on: https://chromium-review.googlesource.com/1162980
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581055}
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/display/screen_ash.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/display/screen_ash.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/display/screen_ash_unittest.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/shell.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/shell_state.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/shell_state.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/ws/window_service_owner.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/public/interfaces/screen_provider_observer.mojom
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/screen_provider.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/screen_provider.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/screen_provider_unittest.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/test_screen_provider_observer.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/test_screen_provider_observer.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/window_service.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/window_service.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/aura/mus/window_tree_client.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/aura/mus/window_tree_client.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/aura/mus/window_tree_client_delegate.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/display/screen.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/display/screen.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/display/screen_unittest.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/views/mus/mus_client.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/views/mus/mus_client.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/views/mus/screen_mus.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/views/mus/screen_mus.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/views/mus/screen_mus_unittest.cc

Status: WontFix (was: Started)
I don't think I can get rid of WidgetParams::context. However, we now have display::Screen::GetDisplayForNewWindows, which can be used to set Widget::mus_properties kDisplayId_InitProperty to put a window on the right display.

Sign in to add a comment