New issue
Advanced search Search tips

Issue 712799 link

Starred by 1 user
Status: Fixed
Owner:
jamescook@chromium.org
Closed: Apr 2017
Cc:
xiy...@chromium.org
mustash-bugs@google.com
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 647412
issue 713317



Sign in to add a comment

mash: Refactor SystemTrayDelegate supervised user methods

Project Member Reported by jamescook@chromium.org, Apr 18 2017 
These methods need to be eliminated or converted to mojo:

  // Returns the display email of the user that manages the current supervised
  // user.
  virtual std::string GetSupervisedUserManager() const;

  // Returns the name of the user that manages the current supervised user.
  virtual base::string16 GetSupervisedUserManagerName() const;

  // Returns the notification for supervised users.
  virtual base::string16 GetSupervisedUserMessage() const;

  // Returns true if the current user is supervised: has legacy supervised
  // account or kid account.
  virtual bool IsUserSupervised() const;

  // Returns true if the current user is child.
  // TODO(merkulova): remove on FakeUserManager componentization.
  //  crbug.com/443119 
  virtual bool IsUserChild() const;

Idea:
* Add user manager email and name to mojom::UserSession and send from Chrome's SessionControllerClient
* Use the existing mojom::UserType for the last two methods

Comment 1 by xiy...@chromium.org, Apr 18 2017 

SGTM.

The tricky part is GetSupervisedUserManager etc that extracts the info from a PKS. Currently SessionControllerClient observes only UserManager, which does not know or handle user profiles. We need to find a place/timing to make SessionControllerClient aware of user profiles. 

For short term solution, let's make SessionControllerClient observe NOTIFICATION_LOGIN_USER_PROFILE_PREPARED and update the info when a user profile is loaded. This happens sometime after when SendUserSession is currently called [1]. Maybe we do another SendUserSession when user profile is loaded.

Longer term, NOTIFICATION_LOGIN_USER_PROFILE_PREPARED would be replaced with a SessionManagerObserver method. Thinking of adding a "started" flag to UserSession record. Set the flag when user profile is loaded and notify the observers.

[1]: https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/session_controller_client.cc?rcl=d024b37af5999584efcec7d3431662203d73ee7d&l=142,150
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 19 2017 

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

commit 0e518b6113db1f4d0c30e2831288a08bc457f639
Author: jamescook <jamescook@chromium.org>
Date: Wed Apr 19 23:00:19 2017

cros: Move IsUserSupervised and IsUserChild off SystemTrayDelegate

Put them on SessionController so they work properly in mustash.

Remove redundant TraySupervisedTest for login crash (the underlying tray
notification code has been refactored so that the crashing code path
no longer exists, and the other test covers the login case well
enough). See  crbug.com/275697  for the crash.

Eliminate ScopedInitialLoginStatus, which was only used by the
redundant test.

BUG= 712799 
TEST=ash_unittests, added to SessionControllerTest, manually testing of
login with supervised users

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

[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/ash/session/session_controller.cc
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/ash/session/session_controller.h
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/ash/session/session_controller_unittest.cc
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/ash/shelf/wm_shelf.cc
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/ash/system/supervised/tray_supervised_user.cc
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/ash/system/supervised/tray_supervised_user_unittest.cc
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/ash/system/tiles/tray_tiles_unittest.cc
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/ash/system/tray/system_tray_delegate.cc
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/ash/system/tray/system_tray_delegate.h
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/ash/system/user/tray_user.cc
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/ash/system/user/user_card_view.cc
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/ash/test/test_session_controller_client.cc
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/ash/test/test_session_controller_client.h
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/ash/test/test_system_tray_delegate.cc
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/ash/test/test_system_tray_delegate.h
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc
[modify] https://crrev.com/0e518b6113db1f4d0c30e2831288a08bc457f639/chrome/browser/ui/ash/system_tray_delegate_chromeos.h

Comment 3 by jamescook@chromium.org, Apr 20 2017

Blocking: 713317
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 24 2017 

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

commit b7360f79e32c16690c062f0b44fecd7d5d452c1e
Author: jamescook <jamescook@chromium.org>
Date: Mon Apr 24 20:44:52 2017

cros: Remove supervised user methods from ash::SystemTrayDelegate

For mustash we need to replace SystemTrayDelegate. Use SessionController
for supervised user data instead.

Send a UserSession update when the Profile becomes available, because
supervised user data is kept in a profile keyed service.

Don't try to update the system tray item text while the menu is open.
The supervised user custodian almost never changes, so we'll see the new
value when the menu is opened again.

For testing, allow FakeChromeUserManager to skip the fake-addition of a
profile to a user on login. It shouldn't be doing this -- in production
the profile is loaded later.

BUG= 712799 
TEST=added to ash_unittests and unit_tests

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

[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/ash/BUILD.gn
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/ash/ash_strings.grd
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/ash/public/interfaces/session_controller.mojom
[delete] https://crrev.com/15e38286df8e3aa77c5be34472ba43045cb4a866/ash/system/supervised/custodian_info_tray_observer.h
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/ash/system/supervised/tray_supervised_user.cc
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/ash/system/supervised/tray_supervised_user.h
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/ash/system/supervised/tray_supervised_user_unittest.cc
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/ash/system/tray/hover_highlight_view.h
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/ash/system/tray/label_tray_view.h
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/ash/system/tray/system_tray.cc
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/ash/system/tray/system_tray.h
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/ash/system/tray/system_tray_delegate.cc
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/ash/system/tray/system_tray_delegate.h
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/ash/test/test_system_tray_delegate.cc
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/ash/test/test_system_tray_delegate.h
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/chrome/app/chromeos_strings.grdp
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/chrome/browser/chromeos/login/users/fake_chrome_user_manager.h
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/chrome/browser/ui/ash/session_controller_client.cc
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/chrome/browser/ui/ash/session_controller_client.h
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/chrome/browser/ui/ash/session_controller_client_unittest.cc
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/chrome/browser/ui/ash/system_tray_delegate_chromeos.h
[modify] https://crrev.com/b7360f79e32c16690c062f0b44fecd7d5d452c1e/chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 26 2017 

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

commit 714f3da6005d557c7dff987fb3ee097201cb31fd
Author: jamescook <jamescook@chromium.org>
Date: Wed Apr 26 00:11:17 2017

cros: Don't send duplicate UserSession mojo messages to ash

After the profile is loaded the SessionControllerClient in the browser
sends an updated ash::mojom::UserSession to ash. Most of the time the
message is the same as the one just sent when the session was started.

BUG= 714689 , 712799 
TEST=added to SessionControllerClientTest in unit_tests

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

[modify] https://crrev.com/714f3da6005d557c7dff987fb3ee097201cb31fd/chrome/browser/ui/ash/session_controller_client.cc
[modify] https://crrev.com/714f3da6005d557c7dff987fb3ee097201cb31fd/chrome/browser/ui/ash/session_controller_client.h
[modify] https://crrev.com/714f3da6005d557c7dff987fb3ee097201cb31fd/chrome/browser/ui/ash/session_controller_client_unittest.cc

Comment 6 by jamescook@chromium.org, Apr 26 2017

Status: Fixed (was: Started)

Comment 7 by lafo...@chromium.org, Feb 26 2018

Components: -Internals>MUS Internals>Services>WindowService
►

Sign in to add a comment