New issue
Advanced search Search tips

Issue 826386 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 678705



Sign in to add a comment

mash: Remove ash::Shell access from AppWindowLauncherController

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

Issue description

It 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?
 

Comment 1 by msw@chromium.org, Mar 27 2018

Labels: -Pri-3 Pri-2
Owner: msw@chromium.org
Status: Assigned (was: Untriaged)
Yes! We need this to activate an app's most recently active window when clicking its shelf button.
(currently, Mash activates the first window added for the app, both Mash and classic show a list)

There seem to be a few wm::ActivationChangeObserver subclasses in src/chrome that need addressing.

I'll take this for now, but may not get to it right away.
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();
Also, some use ash::Shell::Get()->activation_client()->AddObserver(); instead. I'm not sure if there is any difference?

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.
Labels: Proj-Mustash
Labels: -Proj-Mustash Proj-Mash-SingleProcess
Owner: sky@chromium.org
Status: Started (was: Assigned)
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*.
Project Member

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

Labels: -Proj-Mash-SingleProcess Proj-Mash-MultiProcess
Owner: ----
Status: Available (was: Started)
This should work now for the single-process mash case, but multi-process needs a lot more work.
Owner: manucornet@chromium.org
Components: -UI>Shell>Shelf
Owner: ----
Owner: mukai@chromium.org
Status: Assigned (was: Available)
I think it can be done along with issue 887156

Sign in to add a comment