chromeos: Remove old LockScreen signal.

chromeos/dbus/session_manager_client.cc

Index: chromeos/dbus/session_manager_client.cc
diff --git a/chromeos/dbus/session_manager_client.cc b/chromeos/dbus/session_manager_client.cc
index 7def286cda74cd1d982eea943fa10d92a0d5f8b3..28b28e132632049b41f2534c5287a64e8fb3bde2 100644
--- a/chromeos/dbus/session_manager_client.cc
+++ b/chromeos/dbus/session_manager_client.cc
@@ -238,16 +238,6 @@ class SessionManagerClientImpl : public SessionManagerClient {
     blocking_method_caller_.reset(
         new BlockingMethodCaller(bus, session_manager_proxy_));
 
-    // Signals emitted on Chromium's interface.  Many of these ought to be
-    // method calls instead.
-    session_manager_proxy_->ConnectToSignal(
-        chromium::kChromiumInterface,
-        chromium::kLockScreenSignal,
-        base::Bind(&SessionManagerClientImpl::ScreenLockReceived,
-                   weak_ptr_factory_.GetWeakPtr()),
-        base::Bind(&SessionManagerClientImpl::SignalConnected,
-                   weak_ptr_factory_.GetWeakPtr()));
-
     // Signals emitted on the session manager's interface.
     session_manager_proxy_->ConnectToSignal(
         login_manager::kSessionManagerInterface,
@@ -463,10 +453,6 @@ class SessionManagerClientImpl : public SessionManagerClient {
     FOR_EACH_OBSERVER(Observer, observers_, PropertyChangeComplete(success));
   }
 
-  void ScreenLockReceived(dbus::Signal* signal) {
-    FOR_EACH_OBSERVER(Observer, observers_, LockScreen());
-  }
-
   void ScreenIsLockedReceived(dbus::Signal* signal) {
     FOR_EACH_OBSERVER(Observer, observers_, ScreenIsLocked());
   }
@@ -528,9 +514,7 @@ class SessionManagerClientStubImpl : public SessionManagerClient {
   virtual void StartSession(const std::string& user_email) OVERRIDE {}
   virtual void StopSession() OVERRIDE {}
   virtual void StartDeviceWipe() OVERRIDE {}
-  virtual void RequestLockScreen() OVERRIDE {
-    FOR_EACH_OBSERVER(Observer, observers_, LockScreen());

[Daniel Erat, 2014/02/14 23:36:56]

Steven, curious about your thoughts about this. The old flow was:

a) RequestLockScreen() makes a LockScreen call to session_manager
b) session_manager emits LockScreen signal
c) SessionManagerClient calls Observer::LockScreen()
d) code deep in chrome/ locks the screen
e) NotifyLockScreenShown() makes HandleLockScreenShown call to session_manager
f) session_manager emits ScreenIsLocked signal
g) SessionManagerClient calls Observer::ScreenIsLocked()

The stub implementation bypassed the session manager's role by making
RequestLockScreen() call Observer::LockScreen() and making
NotifyLockScreenShown() call Observer::ScreenIsLocked().

b and c have now been replaced by a method call to a new LockScreen D-Bus method
exported by Chrome (see c/b/chromeos/dbus/screen_lock_service_provider.h). This
doesn't fit well with the stub implementation, though -- RequestLockScreen()
isn't able to call into c/b/chromeos/dbus to start Chrome's lock sequence.

Any suggestions for working around this? I could e.g. add
Observer::LockScreenForStub(), making the stub's RequestLockScreen() call it,
and then make ScreenLockServiceProvider implement Observer and handle
LockScreenForStub() the same way as a D-Bus method call from session_manager.
Putting stub-specific code into the interface is gross, though.

[stevenjb, 2014/02/18 17:32:14]

The stub should try to mimic the behavior without any dbus services as minimally
as possible, but it sounds like this is complicated here because calling this
should trigger "code deep in chrome/"? I'm not familiar enough with
c/b/chromeos/dbus/screen_lock_service_provider.h, but I assume there is good
reason why it needs to be in c/b/chromeos/dbus instead of chromeos/dbus (in
which case the two stubs could maybe call each other; we do that a fair bit in
the Network stubs).

[Daniel Erat, 2014/02/18 17:47:21]

org.chromium.LibCrosService is implemented in c/b/chromeos/dbus and the new
LockScreen method call is a member of LibCrosService. More fundamentally, all of
the screen-locking code is in c/b/chromeos/login, so if the LockScreen handler
were outside of chrome/, we'd probably have to switch to an observer- or
delegate-based approach.

[stevenjb, 2014/02/18 17:52:40]

OK. I personally would like to see dbus calls moved to chromeos/dbus with an
observer/delegate interface, but in the meanwhile unless there is a reason we
need the Stub behavior to do something, doing nothing here (or logging) seems
fine.

-  }
+  virtual void RequestLockScreen() OVERRIDE {}
   virtual void NotifyLockScreenShown() OVERRIDE {
     FOR_EACH_OBSERVER(Observer, observers_, ScreenIsLocked());
   }