New issue
Advanced search Search tips

Issue 722496 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

mash: Fix InternalAppWindowShelfController, use shelf window watcher?

Project Member Reported by msw@chromium.org, May 15 2017

Issue description

mash: Remove temporary shelf window watcher code

chrome --mash is using ash::ShelfWindowWatcher for all windows for now.
It does this by assigning shelf ids and item types to windows with no values.
It also handles window shelf id changes (default value -> real value).
It also handles multiple windows per app via assigning launch ids.

All these behaviors are temporary workarounds and should be removed later.
This bug is just a reminder to cleanup ash/shelf/shelf_window_watcher.cc
 

Comment 1 by vadimt@chromium.org, Nov 28 2017

Labels: Not-Touch-Friendly-Launcher
Labels: -Proj-Mustash Proj-Mash-MultiProcess
Cc: sky@chromium.org
Components: Internals>Services>Ash
Labels: -Pri-3 Proj-Mash-SingleProcess Pri-2
Summary: mash: Fix InternalAppWindowShelfController, use shelf window watcher? (was: mash: Remove temporary shelf window watcher code)
This is still relevant for SingleProcessMash, particularly for the KSV.

Chrome's InternalAppWindowShelfController creates shelf items for the KSV, etc. by observing the aura::Env for new windows.
When Chrome uses the WindowService (in SingleProcessMash or Mash), its Env doesn't see the KSV window init.

I think we should move InternalAppWindowShelfController responsibilities to ShelfWindowWatcher.
I'd probably modify the conditionals mentioned above to handle the internal SheldIDs.
Then, it may or may not make sense to remove the handling of unmarked [dialog] windows.
Cc: wutao@chromium.org
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 21

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

commit f974c6e9fd4c73462c3f072d38d63d2bc2eefc4f
Author: Mike Wasserman <msw@chromium.org>
Date: Tue Aug 21 23:36:35 2018

Update ShelfWindowWatcher Mash conditionals

Bug: 722496
Change-Id: I797582085ebaf0c3f63b0eb4ec60d605342c5129
Reviewed-on: https://chromium-review.googlesource.com/1183584
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584923}
[modify] https://crrev.com/f974c6e9fd4c73462c3f072d38d63d2bc2eefc4f/ash/shelf/shelf_window_watcher.cc

It might make sense to have InternalAppWindowShelfController observe the ash::Shell's aura::Env instance as a temporary workaround for SingleProcessMash.
We'll still need a better long-term fix for multi-process Mash.
There's a similar problem with ArcAppWindowLauncherController using aura::Env::GetInstance() to watch for new windows. This was the cause of extra shelf icons in  issue 887129 .
Labels: -Proj-Mash-SingleProcess
https://chromium-review.googlesource.com/c/chromium/src/+/1236559 fixed this for SingleProcessMash. We still need a fix for multi-process though.

Sign in to add a comment