New issue
Advanced search Search tips

Issue 665997 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocked on:
issue 648964



Sign in to add a comment

mash: Refactor SystemTrayDelegate::ShouldShowSettings to mojo

Project Member Reported by jamescook@chromium.org, Nov 16 2016

Issue description

This function is called synchronously when the system tray menu is being opened to determine if the settings icon should be shown (in non-MD) or disabled (in MD). It also disabled some other system tray widgets.

We don't want to block showing UI widgets on a round-trip IPC to chrome, and we need to support opening the tray menu when chrome isn't running. So we should cache this value in ash.

ShouldShowSettings is always true except:
1. When adding a supervised user (via  SupervisedUserCreationFlow::ShouldShowSettings() returning false)
2. When in the secondary login screen (via SessionStateDelegate returning that the SessionState is LOGIN_SECONDARY)

Case 1 could be solved by sending data to ash over mojo when that UserFlow is entered.

Case 2 is blocked on a replacement for SessionStateDelegate,  issue 648964 

 
Components: -MUS Internals>MUS
Status: Started (was: Assigned)
Status: Assigned (was: Started)
Cc: -xiy...@chromium.org jamescook@chromium.org
Owner: xiy...@chromium.org
xiyuan, do you want to take this bug? It sounds like you have a CL for it.

Comment 4 Deleted

Cc: -jamescook@chromium.org xiy...@chromium.org
Owner: jamescook@chromium.org
I'll patch in your CL and take care of it.

Project Member

Comment 6 by bugdroid1@chromium.org, Apr 18 2017

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

commit 341c0454a9e0c39895d2228e25a9fd57f919e16e
Author: jamescook <jamescook@chromium.org>
Date: Tue Apr 18 18:04:18 2017

cros: Use SessionController to enable system tray settings / notifications tray

This eliminates 2 methods from SystemTrayDelegate and makes
the system tray work better in mustash.

* Rename ShouldShowSettings to ShouldEnableSettings, because
the settings gear icon is always visible.
* Cache the booleans from the user flow in ash::mojom::UserSession,
because these are set once per session
* Consolidate the session active/locked/etc. logic in SessionController
* Clean up WebNotificationTray, which uses logged in state and
should-show state inconsistently.

BUG= 648964 , 665997 
TEST=ash_unittests TrayItemTest.*, manual testing of supervised user flow and adding secondary user flow

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

[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/ash/public/interfaces/session_controller.mojom
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/ash/session/session_controller.cc
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/ash/session/session_controller.h
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/ash/system/network/network_state_list_detailed_view.cc
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/ash/system/screen_layout_observer.cc
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/ash/system/tiles/tiles_default_view.cc
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/ash/system/tray/default_system_tray_delegate.cc
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/ash/system/tray/default_system_tray_delegate.h
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/ash/system/tray/system_tray_delegate.cc
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/ash/system/tray/system_tray_delegate.h
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/ash/system/web_notification/web_notification_tray.cc
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/ash/system/web_notification/web_notification_tray.h
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/ash/test/test_session_controller_client.cc
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/chrome/browser/chromeos/login/supervised/supervised_user_creation_flow.cc
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/chrome/browser/chromeos/login/supervised/supervised_user_creation_flow.h
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/chrome/browser/chromeos/login/user_flow.cc
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/chrome/browser/chromeos/login/user_flow.h
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/chrome/browser/ui/ash/session_controller_client.cc
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc
[modify] https://crrev.com/341c0454a9e0c39895d2228e25a9fd57f919e16e/chrome/browser/ui/ash/system_tray_delegate_chromeos.h

Status: Fixed (was: Assigned)

Comment 8 by dchan@google.com, May 30 2017

Labels: VerifyIn-60

Comment 9 by dchan@chromium.org, Aug 1 2017

Labels: VerifyIn-61

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

Status: Archived (was: Fixed)
Components: -Internals>MUS Internals>Services>WindowService

Sign in to add a comment