mash: Remove ash::Shell access from AppWindowLauncherController |
||||||||
Issue descriptionIt uses it to get the window activation client for the primary display to look for app windows becoming active. Do we still need this for mash?
,
Jun 19 2018
Most of these seem to use the pattern: wm::GetActivationClient(ash::Shell::Get()->GetPrimaryRootWindow())->AddObserver(); Its seems that all we really need is an alternative to ash::Shell::Get()->GetPrimaryRootWindow();
,
Jun 19 2018
Also, some use ash::Shell::Get()->activation_client()->AddObserver(); instead. I'm not sure if there is any difference?
,
Jun 19 2018
I believe they are the same activation client for ash, FocusController: https://cs.chromium.org/chromium/src/ash/shell.cc?sq=package:chromium&g=0&l=1393 The trouble is that chrome doesn't know about all windows in the system, so it can't have global knowledge of which window is active.
,
Aug 13
,
Aug 14
,
Oct 29
,
Oct 29
This code is problematic for a number of reasons. The biggest pain point for multi-process-mash is the browser side will not have a handle to any of the windows created by ash for arc and crostini. This will mean the code in browser can't keep the active item up to date. The code responsible for keeping things in sync with the active window really belongs in ash. Thankfully I think most of this code will just work for single-process mash. The one exception is ExtensionAppWindowLauncherController. ExtensionAppWindowLauncherController is currently attaching an ActivationObserver to shell and expecting the windows supplied to OnWindowActivated() to match windows created in the browser. This won't work because OnWindowActivated() is supplied windows created by *ash*.
,
Oct 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f888d68b7cee584b99c126c812a59f4c1612ea0c commit f888d68b7cee584b99c126c812a59f4c1612ea0c Author: Scott Violet <sky@chromium.org> Date: Wed Oct 31 22:04:58 2018 chromeos: shelf activation changes for mash This changes how ExtensionAppWindowLauncherController watches for activation. ExtensionAppWindowLauncherController inherits activation watching from wm::ActivationChangeObserver. The ActivationChangeObserver is added to Shell's root window. This doesn't work for mash. This switches ExtensionAppWindowLauncherController to use a Widget observer. This works in both the mash and non-mash code. I did not promote this logic to AppWindowLauncherController as AppWindowLauncherController is also used for arc/crostini windows, which are created in ash and should continue to use AppWindowLauncherController (at least for the single-process-mash case). For multi-process-mash the arc/crostini launcher code likely does not work at all (because the windows are entirely created by ash) and will need more extensive changes. BUG=826386 TEST=covered by test Change-Id: If5f89d88f782ce452f10889da17210f41189dc77 Reviewed-on: https://chromium-review.googlesource.com/c/1306355 Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#604397} [modify] https://crrev.com/f888d68b7cee584b99c126c812a59f4c1612ea0c/chrome/browser/ui/ash/launcher/app_window_launcher_controller.cc [modify] https://crrev.com/f888d68b7cee584b99c126c812a59f4c1612ea0c/chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h [modify] https://crrev.com/f888d68b7cee584b99c126c812a59f4c1612ea0c/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc [modify] https://crrev.com/f888d68b7cee584b99c126c812a59f4c1612ea0c/chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc [modify] https://crrev.com/f888d68b7cee584b99c126c812a59f4c1612ea0c/chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h [modify] https://crrev.com/f888d68b7cee584b99c126c812a59f4c1612ea0c/ui/views/widget/native_widget_aura.cc
,
Oct 31
This should work now for the single-process mash case, but multi-process needs a lot more work.
,
Nov 14
,
Dec 8
,
Dec 13
I think it can be done along with issue 887156 |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by msw@chromium.org
, Mar 27 2018Owner: msw@chromium.org
Status: Assigned (was: Untriaged)