New issue
Advanced search Search tips

Issue 827680 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Task

Blocking:
issue 692246



Sign in to add a comment

Move screen-locking out of org.chromium.LibCrosService D-Bus service

Project Member Reported by derat@chromium.org, Mar 30 2018

Issue description

Chrome's LockScreen D-Bus method, currently exported via org.chromium.LibCrosService and called by session_manager, should be moved to a new service that lives inside of the browser process.

I was considering something like this:

name:      org.chromium.LockScreenService
path:      /org/chromium/LockScreenService
interface: org.chromium.LockScreenServiceInterface

(or ScreenLockService? we're always inconsistent about this :-/)

I'm a bit concerned that the name may lead developers to believe that they can lock the screen using this service, though. The actual way to lock the screen is by calling the org.chromium.SessionManager service's LockScreen method. If anyone has suggestions to make Chrome's service name clearer, I'm all ears.

In any case, I think that I should also rename Chrome's method, which is only called by session_manager, to something like ShowLockScreen to make its purpose clearer. This fits nicely with Chrome calling session_manager's HandleLockScreenShown D-Bus method after the screen is visible. See https://chromium.googlesource.com/chromiumos/platform2/+/master/login_manager/README.md#screen-locking for more details.
 

Comment 1 Deleted

I am not a dbus guy, but +1 to renaming Chrome's method to ShowLockScreen.

Comment 3 by derat@chromium.org, Apr 3 2018

Status: Started (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 3 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/system_api/+/28ef267edc58dbfd6be675f28ca2b5ae93a402b3

commit 28ef267edc58dbfd6be675f28ca2b5ae93a402b3
Author: Daniel Erat <derat@chromium.org>
Date: Tue Apr 03 23:12:38 2018

system_api: Add org.chromium.LockScreenService constants.

Add constants for a new org.chromium.LockScreenService D-Bus
service that will be exported by Chrome.

BUG= chromium:827680 
TEST=built it

Change-Id: I71e6c03656e5f74b088f9f5473b4cd968ad338f7
Reviewed-on: https://chromium-review.googlesource.com/991454
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>

[modify] https://crrev.com/28ef267edc58dbfd6be675f28ca2b5ae93a402b3/dbus/service_constants.h

Comment 5 by derat@chromium.org, Apr 4 2018

Sigh, wrong description on https://crrev.com/c/991454. I named it org.chromium.ScreenLockService after all.
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 5 2018

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

commit dd0b658d894fab59adf139329b2c96256d407e31
Author: Daniel Erat <derat@chromium.org>
Date: Thu Apr 05 21:51:19 2018

chromeos: Add org.chromium.ScreenLockService D-Bus service.

Add a new org.chromium.ScreenLockService D-Bus service that
exports a single ShowLockScreen method. This uses the
existing ScreenLockServiceProvider class that already
exports an identical LockScreen method on
org.chromium.LibCrosService.

The org.chromium.LibCrosService method will need to continue
to be exported until session_manager has been updated to use
org.chromium.ScreenLockService instead.

Bug:  827680 
Change-Id: I8c907f4a3304ed9f1bb3953a138b10b9ce90363e
Reviewed-on: https://chromium-review.googlesource.com/998437
Commit-Queue: Dan Erat <derat@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548571}
[modify] https://crrev.com/dd0b658d894fab59adf139329b2c96256d407e31/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/dd0b658d894fab59adf139329b2c96256d407e31/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
[add] https://crrev.com/dd0b658d894fab59adf139329b2c96256d407e31/chrome/browser/chromeos/dbus/org.chromium.ScreenLockService.conf
[modify] https://crrev.com/dd0b658d894fab59adf139329b2c96256d407e31/chrome/browser/chromeos/dbus/screen_lock_service_provider.cc
[modify] https://crrev.com/dd0b658d894fab59adf139329b2c96256d407e31/chrome/browser/chromeos/dbus/screen_lock_service_provider.h
[modify] https://crrev.com/dd0b658d894fab59adf139329b2c96256d407e31/chrome/browser/chromeos/login/lock/screen_locker.cc
[modify] https://crrev.com/dd0b658d894fab59adf139329b2c96256d407e31/chrome/browser/chromeos/login/lock/screen_locker.h
[modify] https://crrev.com/dd0b658d894fab59adf139329b2c96256d407e31/chrome/browser/chromeos/login/lock/screen_locker_browsertest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Apr 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/817f48248b3c24fdb83b4bfecc508a540f7874b7

commit 817f48248b3c24fdb83b4bfecc508a540f7874b7
Author: Daniel Erat <derat@chromium.org>
Date: Mon Apr 16 16:20:36 2018

login: Use org.chromium.ScreenLockService D-Bus service.

Update session_manager to call Chrome's new
org.chromium.ScreenLockService D-Bus service rather than its
old org.chromium.LibCrosService service when locking the
screen. The service's method has also been renamed to
ShowLockScreen.

BUG= chromium:827680 
TEST=manually verified that screen-locking still works

Change-Id: I513d35ac391624473a37ef9682cc64eb22b0cc15
Reviewed-on: https://chromium-review.googlesource.com/1001093
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>

[modify] https://crrev.com/817f48248b3c24fdb83b4bfecc508a540f7874b7/login_manager/README.md
[modify] https://crrev.com/817f48248b3c24fdb83b4bfecc508a540f7874b7/login_manager/session_manager_service.cc
[modify] https://crrev.com/817f48248b3c24fdb83b4bfecc508a540f7874b7/login_manager/session_manager_service.h

Comment 8 by derat@chromium.org, May 12 2018

Status: Fixed (was: Started)

Sign in to add a comment