New issue
Advanced search Search tips

Issue 679087 link

Starred by 2 users

Issue metadata

Status: Archived
Owner:
Closed: Jun 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 681072



Sign in to add a comment

mash: Applist gets a separate shelf item when opened

Project Member Reported by msw@chromium.org, Jan 6 2017

Issue description

mash: Applist gets a separate shelf item when opened
On ToT @ #441615, having built 'chrome' and 'mash:all':
(1) Run chrome --mash
(2) Click the Applist (App Launcher) shelf item.
(3) Observe the shelf items.
Expected: No new shelf item is added for the app list.
Actual: A new shelf item is added for the app list.
 

Comment 1 by msw@chromium.org, Jan 13 2017

Blocking: 681072

Comment 2 by msw@chromium.org, Jan 13 2017

I took a quick look; this isn't straightforward to fix at the moment.

WmWindow::GetIntProperty returns TYPE_APP for SHELF_ITEM_TYPE with most windows (temp workaround):
https://cs.chromium.org/chromium/src/ash/common/wm_window.cc?rcl=1484324523&l=358
AppListView/AppListPresenterImpl (in ui/app_list) can't set it's ash-specific SHELF_ITEM_TYPE on creation.
So ShelfWindowWatcher sees a new TYPE_APP window, and makes an icon.

I tried making chrome's app list users set TYPE_APP_LIST, but for some reason this property change (on the chrome side) isn't propagated to ash... There may be a bug in PropertyConverter or its usage.

Even if the window property change took effect, ShelfWindowWatcher would need to delete the existing item (to cleanup its temp workaround mess), before potentially adjusting the app list item. I'm not sure we should bother dealing with the cleanup for the temp workaround, hopefully we can set the property's initial value on window creation (by moving app list to ash,  issue 673629 ), or by removing mash's TYPE_APP hack (so the first time ShelfWindowWatcher gets an item type, it's the correct one...)

Comment 3 Deleted

Comment 4 by msw@chromium.org, May 19 2017

Status: Assigned (was: Fixed)
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 8 2017

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

commit feae432eb3cfeafbd4c30795171ac46462a23c80
Author: msw <msw@chromium.org>
Date: Thu Jun 08 04:24:22 2017

mash: Limit ShelfWindowWatcher to panels and dialogs.

Chrome now syncs its own ShelfModel with Ash in Mash.
Avoid ShelfWindowWatcher and ChromeLauncherController clashing.
(ie. don't create SWW items for windows that CLC will handle)

Set TYPE_APP for extension and arc app windows to avoid SWW items.
Set TYPE_APP_PANEL in extension code; simplify ChromeNativeAppWindowViewsAuraAsh.

Make SWW items for transient windows (per discussion w/oshima).
Only make SWW items for WINDOW_TYPE_NORMAL windows w/o a ShelfItemType.
(avoid making items for [transient] controls, popups, etc.)

Only make SWW items for visible windows (or minimized panels).
(clients set a ShelfItemType before Show() to avoid SWW items)
(fixes StatusAreaBubble, prior to WindowState::ignored_by_shelf)

Expand and refine ShelfWindowWatcher unit tests.
Fix simulated panel window creation in other unit tests.
Restore ExperimentalAppWindowApiTest.SetIcon to mash white lists.
Disable WindowSelectorTest.MultipleDisplays in mash for now.

TODO: Determine why OnAppWindowAdded is only called in Mash...

BUG= 557406 , 679087 , 695562 , 729425 , 730759 
TEST=Automated; chrome --mash doesn't crash opening the wallpaper picker.
R=sky@chromium.org
TBR=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2927693002
Cr-Commit-Position: refs/heads/master@{#477884}

[modify] https://crrev.com/feae432eb3cfeafbd4c30795171ac46462a23c80/ash/shelf/shelf_window_watcher.cc
[modify] https://crrev.com/feae432eb3cfeafbd4c30795171ac46462a23c80/ash/shelf/shelf_window_watcher_unittest.cc
[modify] https://crrev.com/feae432eb3cfeafbd4c30795171ac46462a23c80/ash/wm/overview/window_selector_unittest.cc
[modify] https://crrev.com/feae432eb3cfeafbd4c30795171ac46462a23c80/ash/wm/panels/panel_layout_manager_unittest.cc
[modify] https://crrev.com/feae432eb3cfeafbd4c30795171ac46462a23c80/ash/wm/panels/panel_window_resizer_unittest.cc
[modify] https://crrev.com/feae432eb3cfeafbd4c30795171ac46462a23c80/chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
[modify] https://crrev.com/feae432eb3cfeafbd4c30795171ac46462a23c80/chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
[modify] https://crrev.com/feae432eb3cfeafbd4c30795171ac46462a23c80/chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h
[modify] https://crrev.com/feae432eb3cfeafbd4c30795171ac46462a23c80/chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc
[modify] https://crrev.com/feae432eb3cfeafbd4c30795171ac46462a23c80/testing/buildbot/filters/mash.browser_tests.filter
[modify] https://crrev.com/feae432eb3cfeafbd4c30795171ac46462a23c80/testing/buildbot/filters/mojo.fyi.browser_tests.filter

Comment 6 by msw@chromium.org, Jun 8 2017

Status: Fixed (was: Assigned)
This should be fixed; (at least for now, the shelf code is still in flux)

Comment 7 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment