chromeos: Remove old LockScreen signal.

chromeos: Remove old LockScreen signal.

Remove code that listened for a LockScreen D-Bus signal,
formerly emitted by the session manager to instruct Chrome
to lock the screen. Chrome now exports a LockScreen method
instead.

BUG=341921
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256581

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

https://codereview.chromium.org/167663004/diff/40001/chromeos/dbus/session_ma...
File chromeos/dbus/session_manager_client.cc (left):

https://codereview.chromium.org/167663004/diff/40001/chromeos/dbus/session_ma...
chromeos/dbus/session_manager_client.cc:532: FOR_EACH_OBSERVER(Observer,
observers_, LockScreen());
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]

https://codereview.chromium.org/167663004/diff/40001/chromeos/dbus/session_ma...
File chromeos/dbus/session_manager_client.cc (left):

https://codereview.chromium.org/167663004/diff/40001/chromeos/dbus/session_ma...
chromeos/dbus/session_manager_client.cc:532: FOR_EACH_OBSERVER(Observer,
observers_, LockScreen());
On 2014/02/14 23:36:56, Daniel Erat wrote:
> 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.

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]

https://codereview.chromium.org/167663004/diff/40001/chromeos/dbus/session_ma...
File chromeos/dbus/session_manager_client.cc (left):

https://codereview.chromium.org/167663004/diff/40001/chromeos/dbus/session_ma...
chromeos/dbus/session_manager_client.cc:532: FOR_EACH_OBSERVER(Observer,
observers_, LockScreen());
On 2014/02/18 17:32:14, stevenjb wrote:
> On 2014/02/14 23:36:56, Daniel Erat wrote:
> > 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.
> 
> 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).
> 

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]

lgtm

https://codereview.chromium.org/167663004/diff/40001/chromeos/dbus/session_ma...
File chromeos/dbus/session_manager_client.cc (left):

https://codereview.chromium.org/167663004/diff/40001/chromeos/dbus/session_ma...
chromeos/dbus/session_manager_client.cc:532: FOR_EACH_OBSERVER(Observer,
observers_, LockScreen());
On 2014/02/18 17:47:21, Daniel Erat wrote:
> On 2014/02/18 17:32:14, stevenjb wrote:
> > On 2014/02/14 23:36:56, Daniel Erat wrote:
> > > 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.
> > 
> > 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).
> > 
> 
> 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.

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.

[Nikita (slow), 2014-03-11 17:40:52]

Got this on sign out on desktop.

[29603:29603:0311/213933:FATAL:dbus_thread_manager.cc(513)] Check failed:
g_dbus_thread_manager. DBusThreadManager::Get() called before Initialize()
 [0x7f0d82f5e724] base::debug::StackTrace::StackTrace()
 [0x7f0d82fcd17f] logging::LogMessage::~LogMessage()
 [0x7f0d78356dec] chromeos::DBusThreadManager::Get()
 [0x7f0d88e9402f] chromeos::(anonymous
namespace)::ScreenLockObserver::~ScreenLockObserver()
 [0x7f0d88e97169] base::DefaultLazyInstanceTraits<>::Delete()
 [0x7f0d88e9711c] base::LazyInstance<>::OnExit()
 [0x7f0d82f4a9f0] base::internal::RunnableAdapter<>::Run()
 [0x7f0d82f4a426] base::internal::InvokeHelper<>::MakeItSo()
 [0x7f0d82f49adf] base::internal::Invoker<>::Run()
 [0x7f0d82f491c3] base::Callback<>::Run()
 [0x7f0d82f48c1f] base::AtExitManager::ProcessCallbacksNow()
 [0x7f0d82f4892b] base::AtExitManager::~AtExitManager()
 [0x7f0d74908a98] base::DefaultDeleter<>::operator()()
 [0x7f0d74908b01] base::internal::scoped_ptr_impl<>::reset()
 [0x7f0d749082e7] scoped_ptr<>::reset()
 [0x7f0d74907f48] content::ContentMainRunnerImpl::Shutdown()
 [0x7f0d74905de9] content::ContentMain()
 [0x7f0d86847e89] ChromeMain
 [0x7f0d86847e3c] main
 [0x7f0d700d176d] __libc_start_main
 [0x7f0d86847d49] <unknown>

[Daniel Erat, 2014-03-12 00:15:29]

On 2014/03/11 17:40:52, Nikita Kostylev wrote:
> Got this on sign out on desktop.
> 
> [29603:29603:0311/213933:FATAL:dbus_thread_manager.cc(513)] Check failed:
> g_dbus_thread_manager. DBusThreadManager::Get() called before Initialize()
>  [0x7f0d82f5e724] base::debug::StackTrace::StackTrace()
>  [0x7f0d82fcd17f] logging::LogMessage::~LogMessage()
>  [0x7f0d78356dec] chromeos::DBusThreadManager::Get()
>  [0x7f0d88e9402f] chromeos::(anonymous
> namespace)::ScreenLockObserver::~ScreenLockObserver()
>  [0x7f0d88e97169] base::DefaultLazyInstanceTraits<>::Delete()
>  [0x7f0d88e9711c] base::LazyInstance<>::OnExit()
>  [0x7f0d82f4a9f0] base::internal::RunnableAdapter<>::Run()
>  [0x7f0d82f4a426] base::internal::InvokeHelper<>::MakeItSo()
>  [0x7f0d82f49adf] base::internal::Invoker<>::Run()
>  [0x7f0d82f491c3] base::Callback<>::Run()
>  [0x7f0d82f48c1f] base::AtExitManager::ProcessCallbacksNow()
>  [0x7f0d82f4892b] base::AtExitManager::~AtExitManager()
>  [0x7f0d74908a98] base::DefaultDeleter<>::operator()()
>  [0x7f0d74908b01] base::internal::scoped_ptr_impl<>::reset()
>  [0x7f0d749082e7] scoped_ptr<>::reset()
>  [0x7f0d74907f48] content::ContentMainRunnerImpl::Shutdown()
>  [0x7f0d74905de9] content::ContentMain()
>  [0x7f0d86847e89] ChromeMain
>  [0x7f0d86847e3c] main
>  [0x7f0d700d176d] __libc_start_main
>  [0x7f0d86847d49] <unknown>

Thanks for testing it! And: another singleton class that outlives
DBusThreadManager. :-( I've cleaned up ScreenLockObserver so it doesn't outlive
DBusThreadManager now.

Steven, do you mind taking another look? I'm not crazy about putting
stub-related behavior in the real implementation like this, but I can't think of
a cleaner way to keep screen-lock working in the stub (which I think is useful
for development).

[Daniel Erat, 2014-03-12 00:30:43]

https://codereview.chromium.org/167663004/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screen_locker.cc (right):

https://codereview.chromium.org/167663004/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:391:
ash::Shell::GetInstance()->lock_state_controller()->OnStartingLock();
note that this was previously getting called unconditionally after
HandleLockScreenRequest() returned, which means it would get called in the
UserAddingScreen::Get()->IsRunning() case and then get called again after the
screen was dismissed. this seems wrong, so i'm it only get called when the lock
screen is actually shown. let me know if i'm misunderstanding how this works.

[stevenjb, 2014-03-12 00:40:02]

So this lgtm, but +1 to your own reservation and a suggestion that is arguably
not much of an improvement.

In the NetworkHandler classes, we have a TestInterface class that provides
methods along the lines of "SetStubDelegate". Fake/Stub implementations
implement and return a valid TestInterface; the real interface returns NULL.
External code can test against TestInterface() before calling it. With a single
Fake/Test/Stub method I'm not sure it's worth making the distinction, but I
figured I'd throw that out there.

https://codereview.chromium.org/167663004/diff/120001/chromeos/dbus/session_m...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/167663004/diff/120001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.cc:488: class SessionManagerClientStubImpl
: public SessionManagerClient {
I'm a bit confused as to what the difference between FakeSessionManagerClient
and SessionManagerClientStubImpl is. Not something to clean up here, but I think
we should consider whether we need both (not to mention a Mock imp).

[Daniel Erat, 2014-03-12 01:21:12]

thanks for the review. i think that something like TestInterface would make
sense if there were more methods but agree that it's probably overkill at this
point when there's just one.

https://codereview.chromium.org/167663004/diff/120001/chromeos/dbus/session_m...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/167663004/diff/120001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.cc:488: class SessionManagerClientStubImpl
: public SessionManagerClient {
On 2014/03/12 00:40:03, stevenjb wrote:
> I'm a bit confused as to what the difference between FakeSessionManagerClient
> and SessionManagerClientStubImpl is. Not something to clean up here, but I
think
> we should consider whether we need both (not to mention a Mock imp).

yeah, i don't know the history here either and doubt that we need all of these.

[Daniel Erat, 2014-03-12 05:37:36]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-03-12 05:41:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/167663004/120001

[Daniel Erat, 2014-03-12 08:25:18]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-03-12 08:27:23]

Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg

[commit-bot: I haz the power, 2014-03-12 08:27:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/167663004/160001

[commit-bot: I haz the power, 2014-03-12 16:51:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/167663004/160001

[commit-bot: I haz the power, 2014-03-12 18:02:37]

Change committed as 256581