Videos for expected and actual behaviors in this link:
https://pantheon.corp.google.com/storage/browser/chromiumos-test-logs/bugfiles/cr/872380/

oshima@, anyone, any thoughts here? I am unaware of any changes post 67 that are likely to have affected this.

+wutao

Looks like the issue is caused because we show a shelf icon for "settings" now. The icon is not hidden when switching to a different user. InternalAppWindowShelfController might want to handle ActiveUserChanged, maybe follow CrostiniAppWindowShelfController::ActiveUserChanged [1]

[1] https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/launcher/crostini_app_window_shelf_controller.cc?rcl=4f0ac5703fb84e357fd43736589e84b87385f7ee&l=116

Or it's not registered with MultiUserWindowManager. Such window will appear on all desktops. Is it still v1 app?

Settings is just a Browser window, which is pretty much the same as a "v1 app".

Hi xiyuan@, it seems we cannot simply Add/Remove items from the Shelf, because users can pin the Settings. We need to set the Shelf Item icon to step if it is pinned. I will look more into this how to do this.

s/step/stop

Pinning is per-user. I think ChromeLauncherController should deal with the pings in its ActiveUserChanged.

wutao@, I'm assigning this to you, let me know if you have any questions or need assistance.

It seems difficult to handle the Pinned status.
For the following use case:
Profile 1 with pinned Settings.
Profile 2 with un-pinned Settings.

When switching from Profile 1 to Profile 2, the Setting's shelf item is removed. When switching back, we will lost the info that Profile 1 should has pinned Settings' icon.

How can we persist this info?

UpdateAppLaunchersFromPref() [1] should restored the pinned status. Does that not work?

[1] https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc?rcl=a9f1360c7ee914bb6b234fb988cbec59790c9acc&l=549

Xiyuan, actually for Keyboard Shortcut Viewer, pinned version also work using the logic in Crostini.

However Settings does not work. It will create 2 Settings' icons and the context menu for "Toggle Pin" is messed up.

I will keep investigating why.

Happened to see the following code [1] to handle settings shelf item for user switching. Maybe worth checking out why it breaks and fix it instead.

[1] https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/launcher/multi_profile_browser_status_monitor.cc?rcl=c21e382a9c36f5278c9430f7168944f9ca8f13d4&l=69

Thank you Xiyuan, That is it which creating another copy of Settings!

uploaded a cl to:
https://chromium-review.googlesource.com/c/chromium/src/+/1184037

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

commit 2d8d75d1f8a486418f1b0ea5f3eb1abec2199176
Author: wutao <wutao@chromium.org>
Date: Wed Aug 22 00:55:49 2018

cros: Fix Settings for multiple users

Settings' window is not registered with MultiUserWindowManager, so it is
not handled automatically in multiple users case. This patch registers it
to MultiUserWindowManager and handle the shelf icon change.

Bug:  872380 
Test: manual.
Change-Id: I9d451a27099da77c1c36560c608040684018eeb8
Reviewed-on: https://chromium-review.googlesource.com/1184037
Commit-Queue: Tao Wu <wutao@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584942}
[modify] https://crrev.com/2d8d75d1f8a486418f1b0ea5f3eb1abec2199176/chrome/browser/ui/ash/launcher/internal_app_window_shelf_controller.cc
[modify] https://crrev.com/2d8d75d1f8a486418f1b0ea5f3eb1abec2199176/chrome/browser/ui/ash/launcher/internal_app_window_shelf_controller.h
[modify] https://crrev.com/2d8d75d1f8a486418f1b0ea5f3eb1abec2199176/chrome/browser/ui/ash/launcher/multi_profile_browser_status_monitor.cc

Do we want to merge this fix back to M69?

This bug requires manual review: We are only 12 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

+cindyb@

stevenjb@ and xiyuan@, I am not sure if this bug is a RB. Do we need to merge back to M69? Please advise. Thanks!

I will remove the ReleaseBlock-Stable and remove the merge request since this bug is not a security fix or crash fix.

cindyb@, please let me know if you are ok with it. Thanks!

+cindyb@, please see #22&#23.