chromeos: Remove old LockScreen signal.

chromeos/dbus/session_manager_client.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/dbus/session_manager_client.h"
 
 #include <map>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 220 matching lines @@
   }
 
  protected:
   virtual void Init(dbus::Bus* bus) OVERRIDE {
     session_manager_proxy_ = bus->GetObjectProxy(
         login_manager::kSessionManagerServiceName,
         dbus::ObjectPath(login_manager::kSessionManagerServicePath));
     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,
         login_manager::kOwnerKeySetSignal,
         base::Bind(&SessionManagerClientImpl::OwnerKeySetReceived,
                    weak_ptr_factory_.GetWeakPtr()),
         base::Bind(&SessionManagerClientImpl::SignalConnected,
                    weak_ptr_factory_.GetWeakPtr()));
     session_manager_proxy_->ConnectToSignal(
         login_manager::kSessionManagerInterface,
@@ skipping 195 matching lines @@
     dbus::MessageReader reader(signal);
     std::string result_string;
     if (!reader.PopString(&result_string)) {
       LOG(ERROR) << "Invalid signal: " << signal->ToString();
       return;
     }
     const bool success = StartsWithASCII(result_string, "success", false);
     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());
   }
 
   void ScreenIsUnlockedReceived(dbus::Signal* signal) {
     FOR_EACH_OBSERVER(Observer, observers_, ScreenIsUnlocked());
   }
 
   // Called when the object is connected to the signal.
   void SignalConnected(const std::string& interface_name,
@@ skipping 41 matching lines @@
     observers_.RemoveObserver(observer);
   }
   virtual bool HasObserver(Observer* observer) OVERRIDE {
     return observers_.HasObserver(observer);
   }
   virtual void EmitLoginPromptVisible() OVERRIDE {}
   virtual void RestartJob(int pid, const std::string& command_line) OVERRIDE {}
   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());
   }
   virtual void NotifyLockScreenDismissed() OVERRIDE {
     FOR_EACH_OBSERVER(Observer, observers_, ScreenIsUnlocked());
   }
   virtual void RetrieveActiveSessions(
       const ActiveSessionsCallback& callback) OVERRIDE {}
   virtual void RetrieveDevicePolicy(
       const RetrievePolicyCallback& callback) OVERRIDE {
@@ skipping 88 matching lines @@
 
 SessionManagerClient* SessionManagerClient::Create(
     DBusClientImplementationType type) {
   if (type == REAL_DBUS_CLIENT_IMPLEMENTATION)
     return new SessionManagerClientImpl();
   DCHECK_EQ(STUB_DBUS_CLIENT_IMPLEMENTATION, type);
   return new SessionManagerClientStubImpl();
 }
 
 }  // namespace chromeos