mash: Shelf selections and context menu commands open windows open on the wrong display. |
|||||||
Issue descriptionmash: Chrome windows open on wrong display from shelf context menu. Running linux desktop ChromeOS on ToT@#503942 (1a) Run "chrome --ash-host-window-bounds=500x500,600+0-600x500 --ash-enable-shelf-model-synchronization" (1b) Run "chrome --ash-host-window-bounds=500x500,600+0-600x500 --mash" (2) Right-click on the chrome icon on the shelf on the second display (on the right) (3) Click "New window" Expected: Chrome window opens on the secondary display. Actual: Chrome window opens on the primary display. This should block enabling shelf model sync by default.
,
Sep 27 2017
Mike, are these user-visible regressions in classic ash with no flags? Like, did these cases work properly in M60/M61 stable?
,
Sep 27 2017
Hmm, after some manual spot-testing, it's broken on M59, M58, M55, and M50... This defect has been around for at least a year... I filed Issue 769389.
,
Sep 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/05655615e0d589c91440c1adc2c3329593263293 commit 05655615e0d589c91440c1adc2c3329593263293 Author: Mike Wasserman <msw@chromium.org> Date: Wed Sep 27 23:30:39 2017 ChromeOS: Open chrome on the shelf context menu's display in cash. Make Chrome shelf item context menus open items on the right display. Use a ScopedRootWindowForNewWindows when executing commands in cash. (we'll need another fix for mash, but my first attempt didn't work) Fixes the defect when using --ash-enable-shelf-model-synchronization. Nix the ShelfView codepath that only matches the local menu lifetime. (ShelfModel Sync handles the menu command execution async/remotely) Bug: 768908 Test: Chrome opens on the same display as the shelf item [context menu]. Change-Id: If8c812c892c4c628e9c012ea9e9cd75461d3b71a Reviewed-on: https://chromium-review.googlesource.com/688108 Reviewed-by: James Cook <jamescook@chromium.org> Commit-Queue: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#504800} [modify] https://crrev.com/05655615e0d589c91440c1adc2c3329593263293/ash/shelf/shelf_view.cc [modify] https://crrev.com/05655615e0d589c91440c1adc2c3329593263293/chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc [modify] https://crrev.com/05655615e0d589c91440c1adc2c3329593263293/chrome/browser/ui/ash/launcher/launcher_context_menu.h
,
Sep 27 2017
My CL fixed the classic-ash regression with --ash-enable-shelf-model-synchronization. This bug still tracks the defect that Browser windows and V1 app windows/tabs launch on the wrong display via left-click selection or context menu "New * " in mash.
,
Nov 27 2017
,
Apr 19 2018
,
May 23 2018
I have a fix out for review for the ash::Shell access in Chrome's WindowSizerAsh. However, that doesn't fix the problems under mash in comment 5. I think that ash/top_level_window_factory.cc GetRootWindowControllerForNewTopLevel() needs to support screen bounds and sometimes return a secondary display. Otherwise we need to audit all the chrome code that computes window bounds and make sure it sets Widget::InitParams::mus_properties display_id appropriately.
,
May 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/40f7d75ba9538c1323b69da9d348c92d5fce4136 commit 40f7d75ba9538c1323b69da9d348c92d5fce4136 Author: James Cook <jamescook@chromium.org> Date: Fri May 25 18:21:58 2018 chromeos: Update browser window placement for out-of-process ash WindowSizerAsh needs synchronous access to the target display for each new browser window. * Create ash::ShellState and move root_window_for_new_windows_ there * Introduce mojom::ShellState interface in ash * Create a ShellStateClient in chrome to cache the target display id for new windows The interface is not implemented by ash::Shell because ash::Shell is already massive (1500 lines). Also, shell.h is included in 900 places in the code base and I don't want to add mojo generated headers to shell.h. Windows still don't appear on the secondary display with --enable-features=Mash, but chrome is getting the correct display id and setting the window bounds properly. I think ash needs to be fixed to support top-level bounds on the secondary display, but that will need to be a follow-up CL. Bug: 764009 , 768908 Test: ash_unittests, unit_tests for WindowSizer Change-Id: Ib77834f8ed5dc4e65f02ff83ab81acbf3331db61 Reviewed-on: https://chromium-review.googlesource.com/1067591 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Commit-Queue: James Cook <jamescook@chromium.org> Cr-Commit-Position: refs/heads/master@{#561950} [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/BUILD.gn [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/manifest.json [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/mojo_interface_factory.cc [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/public/interfaces/BUILD.gn [add] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/public/interfaces/shell_state.mojom [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/root_window_controller.cc [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/scoped_root_window_for_new_windows.cc [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/scoped_root_window_for_new_windows.h [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/shelf/shelf_view.cc [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/shell.cc [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/shell.h [add] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/shell_state.cc [add] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/shell_state.h [add] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/shell_state_unittest.cc [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/wm/window_positioning_utils.cc [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/BUILD.gn [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/DEPS [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.h [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/launcher/DEPS [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc [add] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/shell_state_client.cc [add] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/shell_state_client.h [add] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/shell_state_client_unittest.cc [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/window_sizer/window_sizer.cc [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/window_sizer/window_sizer.h [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/window_sizer/window_sizer_common_unittest.cc [modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/test/BUILD.gn
,
May 30 2018
Hey James, is this still broken?
,
May 30 2018
Problems in comment #5 are fixed, I think this is all working now. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by msw@chromium.org
, Sep 26 2017Cc: varkha@chromium.org osh...@chromium.org
Summary: mash: Shelf selections and context menu commands open windows open on the wrong display. (was: mash: Chrome windows open on wrong display from shelf context menu.)