mash: Shelf support for ARC++ windows |
|||||||
Issue descriptionArcAppWindowLauncherController uses aura::Env to observe for all aura window creation via OnWindowInitialized(). It then checks each window to see if it is associated with an ARC++ app, attaches controllers, and updates shelf icons. This was broken for SingleProcessMash, which resulted in ShelfWindowWatcher creating extra blank icons. See issue 887129 . This won't work in multiprocess mash. Chrome needs another way to learn when ARC++ windows are being created. This relates to issue 722496 for ShelfWindowWatcher and InternalAppWindowShelfController
,
Sep 20
Crostini windows and "internal apps" (Discover, Settings, KSV) have the same problem.
,
Sep 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9f54a2eca04c84c451ea5ae6d4e5f7bbe01dbfb6 commit 9f54a2eca04c84c451ea5ae6d4e5f7bbe01dbfb6 Author: James Cook <jamescook@chromium.org> Date: Fri Sep 21 00:33:40 2018 Fix shelf icons for crostini and internal apps with SingleProcessMash The controllers in chrome were observing the wrong aura::Env instance. Bug: 887156 Test: manual Change-Id: Ib7e978a4ea7014c2fad65ee6afb73e40311117c7 Reviewed-on: https://chromium-review.googlesource.com/1236559 Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: James Cook <jamescook@chromium.org> Cr-Commit-Position: refs/heads/master@{#593020} [modify] https://crrev.com/9f54a2eca04c84c451ea5ae6d4e5f7bbe01dbfb6/chrome/browser/ui/ash/launcher/DEPS [modify] https://crrev.com/9f54a2eca04c84c451ea5ae6d4e5f7bbe01dbfb6/chrome/browser/ui/ash/launcher/crostini_app_window_shelf_controller.cc [modify] https://crrev.com/9f54a2eca04c84c451ea5ae6d4e5f7bbe01dbfb6/chrome/browser/ui/ash/launcher/internal_app_window_shelf_controller.cc
,
Sep 21
@oshima - I'm wondering if there's a place where we can assign that aura::client::kAppType to ash::AppType::ARC_APP somewhere earlier than this: https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc?g=0&l=224 I just don't know where the ARC++ views::Widget or aura::Window is first created.
,
Sep 21
It is in shell_surbase_base, but the logic is shared with other wayland clients. Can you point me by when you need this info?
,
Sep 21
Ideally it would be available when the views::Widget is created so we can set the shelf item type in views::Widget::InitParams::mus_properties. See the link in comment 1. This would mean that when ash sees the Widget's aura Window being created it would know what to do with the shelf icon, rather than waiting for ArcAppWindowLauncherController to fix it later.
,
Sep 21
Does the shelf actually need to know that it's an Arc app vs. Crostini, etc.? Would it be okay to mark all wayland windows with the same shelf item type?
,
Sep 25
,
Sep 25
It'd be helpful to know what exactly the issue is. The truth about if it's indeed ARC (android) window comes from mojo interface. We can treat "client controlled" windows as arc++, but anyone can implement wayland remote-shell protocol, without being arc++ apps.
,
Sep 25
oshima, the issue is that there is shelf code in the browser process that watches aura::Env for all aura::Window creation, detects if the window is an ARC++ app, then "fixes up" the shelf item type so the shelf icon is correct. See code in #5. That won't work in mash because the browser process will not see all aura::Window creation.
,
Sep 26
The icon for shelf can be retrieved from android and set in async fasion (that is, running app can provide different icon), so detecing arc early may or may not help all cases. > That won't work in mash because the browser process will not see all aura::Window creation. Is it just a creation or aura windows at all? I'm asking because you may need to solve similar issue for notification.
,
Sep 28
I chatted with James and we agreed that we need more generic solution for arc bridge in chrome because it currently has assumption that it process can see the arc windows, which won't work in multi process mash. I'll file a separate bug to track it.
,
Sep 28
filed crbug.com/890403
,
Oct 26
,
Dec 12
,
Dec 12
,
Dec 12
mukai@, I believe your design doc will address this issue, correct? https://docs.google.com/document/d/1ejbZ9zzZZOQmsWQbKMpIIfsFpaSMw_WCWThsWwvVdeg/edit?ts=5c1159c1
,
Dec 12
Yes
,
Jan 11
This is also related to Issue 722496 for internal apps. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by jamescook@chromium.org
, Sep 20