New issue
Advanced search Search tips

Issue 778888 link

Starred by 1 user
Status: Verified
Owner:
derat@chromium.org
Closed: Oct 2017
Cc:
xiy...@chromium.org
jiameng@chromium.org
jamescook@chromium.org
steve...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Task



Sign in to add a comment

Remove screen-lock stuff from chromeos::SessionManagerClient::Observer

Project Member Reported by derat@chromium.org, Oct 27 2017 
I noticed that there are (at least) two ways to watch for changes to the screen-lock state:

- ash::SessionObserver (and ash::SessionController)
- chromeos::SessionManagerClient::Observer

SessionManagerClient is low-level and implementation-specific. I think we should dissuade use of it for monitoring this. I'd like to drop the ScreenIsLocked and ScreenIsUnlocked methods from SessionManagerClient::Observer.

I think that we unfortunately need to keep SessionManagerClient::IsScreenLocked for now, since it's called by CheckIdleStateIsLocked in ui/base/idle/idle_chromeos.cc. (This is our usual problem where there's code in //base or //ui or wherever that wants to get system state that's provided by the OS on other platforms but by Chrome/ash/etc. on Chrome OS.)

Comment 1 by xiy...@chromium.org, Oct 27 2017 

If code lives in ash, use ash::SessionObserver and ash::SessionController.
If code lives in browser, use session_manager::SessionManagerObserver and session_manager::SessionManager.

And it is tricky for code in //ui since it could live in either ash or browser.
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 31 2017 

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

commit 13eec1d1c31d2291b9d0187d8bd409e7eb3737d1
Author: Daniel Erat <derat@chromium.org>
Date: Tue Oct 31 19:10:40 2017

chromeos: Drop session_manager screen lock notifications.

Remove ScreenIsLocked and ScreenIsUnlocked from
SessionManagerClient::Observer. Callers should use
ash::SessionObserver (in ash) or
session_manager::SessionManagerObserver (in the
browser) instead.

Update the following classes to do that:
- ash::PowerEventObserver
- chromeos::ExtensionSystemEventObserver
- TetherService

Bug:  778888 
Change-Id: I9792338a8c5cce6f0aeaddb11dad8adfd951683c
Reviewed-on: https://chromium-review.googlesource.com/741262
Commit-Queue: Dan Erat <derat@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512898}
[modify] https://crrev.com/13eec1d1c31d2291b9d0187d8bd409e7eb3737d1/ash/system/power/power_event_observer.cc
[modify] https://crrev.com/13eec1d1c31d2291b9d0187d8bd409e7eb3737d1/ash/system/power/power_event_observer.h
[modify] https://crrev.com/13eec1d1c31d2291b9d0187d8bd409e7eb3737d1/ash/system/power/power_event_observer_unittest.cc
[modify] https://crrev.com/13eec1d1c31d2291b9d0187d8bd409e7eb3737d1/chrome/browser/chromeos/extensions/extension_system_event_observer.cc
[modify] https://crrev.com/13eec1d1c31d2291b9d0187d8bd409e7eb3737d1/chrome/browser/chromeos/extensions/extension_system_event_observer.h
[modify] https://crrev.com/13eec1d1c31d2291b9d0187d8bd409e7eb3737d1/chromeos/dbus/session_manager_client.cc
[modify] https://crrev.com/13eec1d1c31d2291b9d0187d8bd409e7eb3737d1/chromeos/dbus/session_manager_client.h

Comment 3 by derat@chromium.org, Oct 31 2017

Status: Verified (was: Started)
►

Sign in to add a comment