New issue
Advanced search Search tips

Issue 768908 link

Starred by 0 users

Issue metadata

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

Blocked on:
issue 764009

Blocking:
issue 557406



Sign in to add a comment

mash: Shelf selections and context menu commands open windows open on the wrong display.

Project Member Reported by msw@chromium.org, Sep 26 2017

Issue description

mash: 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.
 

Comment 1 by msw@chromium.org, Sep 26 2017

Blockedon: 764009
Cc: 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.)
Yikes, this area seems pretty broken. Some mash defectiveness stems from  Issue 764009 , but beyond mash browser initial bounds:
1) On ToT without any flags, V2 (packaged) app (ie. Files) windows launch on the wrong display via left-click selection or context menu.
2) V1 app tabs/windows launch on the wrong display via context menus with either flag: --ash-enable-shelf-model-synchronization / --mash.
3) Browser windows launch on the wrong display via left-click selection or context menu with either flag.

The context menu defects occur because Ash's context menu is destroyed (and ShelfView::OnMenuClosed resets |scoped_root_window_for_new_windows_|) before Chrome's ExtensionLauncherContextMenu::ExecuteCommand receives the mojo IPC to execute the context menu command to open a new window. So at the time of the browser window creation, Ash doesn't have a ScopedRootWindowForNewWindows set for the window hosting the context menu, and the new browser window appears on the default/primary display.

Some possible solutions:
1) Pass |display_id| or |bounds| to chrome::NewEmptyWindow (or similar). It's unclear how these should interact with the existing new browser window bounds determination in WindowSizer and ash::WindowPositioner (see related  Issue 764009 ).
2) Make ExtensionLauncherContextMenu::ExecuteCommand create its own ScopedRootWindowForNewWindows to solve the classic ash case. We'll need to fix this somehow for mash later on... maybe by:
3) Plumb a scoped display_id value to WindowSizer/ash::WindowPositioner and/or ChromeViewsDelegate::OnBeforeWidgetInit for kDisplayId_InitProperty, etc. (but that'll potentially conflict with WindowSizer...)
4) I wrote up a patch that constructs a ScopedRootWindowForNewWindows instance during the ShelfContextMenu's call to ExecuteCommand, and that works in Classic Ash, but doesn't seem sufficient in Mash (likely conflicts with WindowSizer-related code in mash)
5) Something with NewWindowClient?

I may go with something like (2) to fix the classic ash defects with --ash-enable-shelf-model-synchronization, to unblock that sooner.
(my local work fixes this for Chrome browser window launching via context menus, but doesn't fix the V1 or V2 app defects...)
Mike, are these user-visible regressions in classic ash with no flags? Like, did these cases work properly in M60/M61 stable?

Comment 3 by msw@chromium.org, 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.
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Comment 5 by msw@chromium.org, Sep 27 2017

Cc: msw@chromium.org
Owner: ----
Status: Available (was: Started)
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.

Comment 6 by vadimt@chromium.org, Nov 27 2017

Labels: Touch-Friendly-Launcher Touch-Friendly-Launcher-Triaged
Components: Internals>Services>Ash
Labels: -Proj-Mustash-Mash
Cc: -varkha@chromium.org est...@chromium.org
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.

Project Member

Comment 9 by bugdroid1@chromium.org, 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

Comment 10 by msw@chromium.org, May 30 2018

Owner: jamescook@chromium.org
Hey James, is this still broken?
Status: Fixed (was: Available)
Problems in comment #5 are fixed, I think this is all working now.

Sign in to add a comment