New issue
Advanced search Search tips

Issue 887156 link

Starred by 1 user

Issue metadata

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


Participants' hotlists:
Launcher-Tech-Debt


Sign in to add a comment

mash: Shelf support for ARC++ windows

Project Member Reported by jamescook@chromium.org, Sep 20

Issue description

ArcAppWindowLauncherController 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

 
Cc: osh...@chromium.org
oshima, do you know if ShellSurfaceWidget (in shell_surface_base.cc) is 1:1 with ARC++ app windows? In particular, could we set a views::Widget::InitParams property so that ash knows that it is an ARC++ app?

This would be similar to this change for chrome app windows:
https://chromium-review.googlesource.com/c/chromium/src/+/644849/7/chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc

Crostini windows and "internal apps" (Discover, Settings, KSV) have the same problem.

Project Member

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

@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.

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

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?
Cc: newcomer@chromium.org
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.


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.

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.
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.
filed crbug.com/890403
Owner: manucornet@chromium.org
Labels: M-73
Status: Assigned (was: Available)
Owner: mukai@chromium.org
mukai@, I believe your design doc will address this issue, correct?

https://docs.google.com/document/d/1ejbZ9zzZZOQmsWQbKMpIIfsFpaSMw_WCWThsWwvVdeg/edit?ts=5c1159c1
Status: Started (was: Assigned)
Yes
This is also related to Issue 722496 for internal apps.

Sign in to add a comment