Created attachment 221596 [details] [review]
Consolidate ConsoleKit and Systemd code

In preparation for accessing it in the screenshield, factor out
common code for ConsoleKit and Systemd.
Also, clean up ConsoleKit manager, as the daemon is required in
non systemd installation.

Created attachment 221597 [details] [review]
ScreenShield: listen to login managers to unlock the session

Handle locking and unlocking from outside. This fixes coming
back from fast user switching.

Review of attachment 221596 [details] [review]:

You should probably summarize the clean ups that you've made to ConsoleKit in the commit message.
1) allow it to be autostarted when accessed since it's only accessed when systemd isn't running
2) remove code that assumes sessoin is active where it's not running.

::: js/misc/consoleKit.js
@@ +48,3 @@
+        });
+        self.ckSession.IsActiveRemote(function([isActive]) {
+            self.sessionActive = isActive;

I guess there's a small window at ConsoleKit start up where sessionActive is invalid.  This combined with ConsoleKit --timed-exit could cause issues, but I think we can just say ConsoleKit --timed-exit is wrong and people shouldn't use it in production.  I think it's mostly a debugging option anyway.

Review of attachment 221597 [details] [review]:

::: js/ui/screenShield.js
@@ +347,3 @@
+            this._systemdProxy = new Systemd.SystemdLoginSession(GLib.getenv('XDG_SESSION_ID'));
+            this._systemdProxy.connectSignal('Lock', Lang.bind(this, function() { this.lock(false); }));
+            this._systemdProxy.connectSignal('Unlock', Lang.bind(this, function() { this.unlock(); }));

So a couple of questions:

1) What happens if lock Lock is emitted multiple times in a row?
2) What happens when unlock is emitted when already unlocked?
3) What happens when unlock is emitted at the login screen?

These things shouldn't happen in practice, but would be good to make sure we react relatively benigning to it.

Review of attachment 221597 [details] [review]:

::: js/ui/screenShield.js
@@ +347,3 @@
+            this._systemdProxy = new Systemd.SystemdLoginSession(GLib.getenv('XDG_SESSION_ID'));
+            this._systemdProxy.connectSignal('Lock', Lang.bind(this, function() { this.lock(false); }));
+            this._systemdProxy.connectSignal('Unlock', Lang.bind(this, function() { this.unlock(); }));

So a couple of questions:

1) What happens if lock Lock is emitted multiple times in a row?
2) What happens when unlock is emitted when already unlocked?
3) What happens when unlock is emitted at the login screen?

These things shouldn't happen in practice, but would be good to make sure we react relatively benigning to it.

1 / 2) Both lock and unlock are expected to be completely idempotent. There was one small bug in .unlock(), it's fixed by the next patch
3) unlock() checks if the dialog is for unlocking or login (that's the _keepDialog property), and for the login case it just ensures the dialog is there and returns

Created attachment 221619 [details] [review]
ScreenShield: listen to login managers to unlock the session

Handle locking and unlocking from outside. This fixes coming
back from fast user switching.
Also, make .lock() and .unlock() idempotent, in case multiple
signals come from the system.

I wonder if it makes sense to have:

function getSessionWatcher() {
    if (Systemd.hasSystemd())
        return new SystemdSessionWatcher();
    else
        return new ConsoleKitSessionWatcher();
}

which both have the same API, so that we don't have to care about Systemd all over the codebase, but instead just get the session watcher or whatever, connect to signals, and call various methods.

That's the approach gnome-session uses for dealing with systemd/consolekit (see gsm-system.c).

I like that approach better, but on the other hand, the current patches already leave the codebase cleaner than it was before, and that clean up could go in any time.

I totally agree though.

Review of attachment 221619 [details] [review]:

Seems fine, but can you split it into two commits?

Attachment 221596 [details] pushed as 6cb713b - Consolidate ConsoleKit and Systemd code
Attachment 221619 [details] pushed as b6375d3 - ScreenShield: listen to login managers to unlock the session

This seems to have broken the consolekit path. Causing the following error:
    JS ERROR: !!!   Exception was: TypeError: this._consoleKitProxy.ckSession is undefined
    JS ERROR: !!!     message = '"this._consoleKitProxy.ckSession is undefined"'
    JS ERROR: !!!     fileName = '"/usr/share/gnome-shell/js/ui/screenShield.js"'
    JS ERROR: !!!     lineNumber = '350'
    JS ERROR: !!!     stack = '"()@/usr/share/gnome-shell/js/ui/screenShield.js:350
wrapper()@/usr/share/gjs-1.0/lang.js:204
()@/usr/share/gjs-1.0/lang.js:145
()@/usr/share/gjs-1.0/lang.js:239
start()@/usr/share/gnome-shell/js/ui/main.js:215
@<main>:1
"'

Created attachment 221693 [details] [review]
Fix ConsoleKit regression from b6375d3e4066a34d530b91e89035a9f2408ff81d

ConsoleKitManager.ckSession is set asynchronously, we need to wait
for it to appear before connecting signals.

The only two places you use ConsoleKit.ConsoleKitManager, you're actually using

ConsoleKit.ConsoleKitManager().ckSession

(well there's another bug in automountManager.js where you do this.ckListener.sessionActive instead of this.ckListener.ckSession.sessionActive that we missed earlier)

Given that:

1) you aren't using ConsoleKitManager for anything but a means toward the ckSession end
2) you're already using it as more than a dbus proxy by adding ckSession to it anyway
3) you aren't using the ckSession for more than those two signals

why not make it be a normal object that has two proxies on it and that emits "lock" and "unlock" when appropriate?

Created attachment 221702 [details] [review]
Consolidate systemd and consolekit in a common abstract class

Various code around had different paths for ConsoleKit and
logind. Consolidate it by making an abstract class that all
callers can use, which hides the implementation details of the
two daemons.

Previous approach was a very quick and very dirty hack. This
one works, and further cleans up the code using consolekit/systemd.

Created attachment 221703 [details] [review]
Consolidate systemd and consolekit in a common abstract class

Various code around had different paths for ConsoleKit and
logind. Consolidate it by making an abstract class that all
callers can use, which hides the implementation details of the
two daemons.

Forgot to amend the commit...

*** Bug 682159 has been marked as a duplicate of this bug. ***

Review of attachment 221703 [details] [review]:

What's the point of the abstract class?

Cleanliness, documentation, instanceof, and a place to hook the static method on.

(In reply to comment #19)
> Cleanliness, documentation, instanceof, and a place to hook the static method
> on.

Well, the static method could just be a function.

    LoginManager.getLoginManager();

I can't quite see the utility of instanceof in there, and there is no documentation :)

Review of attachment 221703 [details] [review]:

::: js/ui/screenShield.js
@@ +344,3 @@
+        this._loginSession = this._loginManager.getCurrentSessionProxy();
+        this._loginSession.connectSignal('Lock', Lang.bind(this, function() { this.lock(false); }));
+        this._loginSession.connectSignal('Unlock', Lang.bind(this, function() { this.unlock(); }));

I wonder if you should add 'lock'/'unlock' signals to the login manager, and make the session proxy entirely internal.

Review of attachment 221703 [details] [review]:

Mostly looks good as far as I can tell - OK to commit with the two fixes below. (Would be nice to get some testing with ConsoleKit - hopefully ricotz will do that.)

::: js/misc/loginManager.js
@@ +93,3 @@
+    },
+
+    getCurrentSessionProxy: function() {

Might be good to have a comment along the lines of:

 // Having this function is a bit of a hack since the Systemd and ConsoleKit
 // session objects have different interfaces - but in both cases there are
 // Lock/Unlock signals, and that's all we count upon at the moment.

@@ +94,3 @@
+
+    getCurrentSessionProxy: function() {
+        this._currentSession = new SystemdLoginSession(Gio.DBus.system,

Do you really want to reassign this on every call to getCurrentSessionProxy?

@@ +144,3 @@
+    getCurrentSessionProxy: function() {
+        if (!this._currentSession) {
+            let [currentSessionId] = this._proxy.GetCurrentSessionSync();

Seems like a reasonable compromise for simplicity in the rarer ConsoleKit case

Attachment 221703 [details] pushed as 9a7914e - Consolidate systemd and consolekit in a common abstract class