Created attachment 255643 [details] [review]
loginManager: fix versionCompare function

It's important to compare the version components as integers,
not strings, so "10" evaulates as greater than "5"

This fixes the login screen in gnome 3.10.

Review of attachment 255643 [details] [review]:

This is correct, but as I mentioned in IRC, we should just remove the version check completely.

Created attachment 255644 [details] [review]
ui: drop canLock() function

Realistically, we require GDM and gnome-shell to be coupled.

This commit removes the canLock function and associated version check.

(i think both patches should go in, so we leave versionCompare in a known "good" state before we remove it in case it ever gets revived from history.)

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

Review of attachment 255644 [details] [review]:

The canLock() check exists primarily to support gnome-shell on lightdm/kdm, I don't think we can remove it.

::: js/ui/status/system.js
@@ +247,3 @@
         let showLock = !Main.sessionMode.isLocked && !Main.sessionMode.isGreeter;
         let allowLockScreen = !this._lockdownSettings.get_boolean(DISABLE_LOCK_SCREEN_KEY);
+        this._lockScreenAction.visible = showLock && allowLockScreen;

We're making a sync dbus call everytime you open the menu? Bad!
Main.screenShield != undefined should do the trick without IO.

Review of attachment 255644 [details] [review]:

Yes.

(In reply to comment #6)
> The canLock() check exists primarily to support gnome-shell on lightdm/kdm, I
> don't think we can remove it.

Do we care about that usecase?

> We're making a sync dbus call everytime you open the menu? Bad!

Not anymore we aren't.

(In reply to comment #8)
> (In reply to comment #6)
> > The canLock() check exists primarily to support gnome-shell on lightdm/kdm, I
> > don't think we can remove it.
> 
> Do we care about that usecase?

Yes, we do. People are using gnome in ubuntu, we want to support them at least to some extent.

we've talked about it more, and checking if GDM is on the bus isn't really right. We should check if the session is a GDM session.  That's easiest done by if (GLib.getenv("GDMSESSION")) so that's probably the best way to go.

I think for the current "crisis" though just doing attachment 255643 [details] [review] is right.

Comment on attachment 255643 [details] [review]
loginManager: fix versionCompare function

(this went in but git-bz failed because bugzilla was being moved or something)

(In reply to comment #11)
> (From update of attachment 255643 [details] [review])
> (this went in but git-bz failed because bugzilla was being moved or something)

From that patch:

+        let requiredInt = parseInt(required[i]);
+        let referenceInt = parseInt(reference[i]);

could be improved as:

+        let requiredInt = parseInt(required[i], 10);
+        let referenceInt = parseInt(reference[i], 10);

Because adding the 'base' parameter is a recommended practice, otherwise parseInt will return 0 for the strings '08' and '09' as they are recognized as octal. 

See http://stackoverflow.com/questions/850341/how-do-i-work-around-javascripts-parseint-octal-behavior for more info about this.

This is corrected in latest versions of javascript, and I checked it works ok in the firefox 24 of fedora 19, but still fails for the javascript that gjs uses (checked in fedora 19 too).

I agree that would have been better.  A couple of things though:

1) in practice, gnome-shell will never have a version component that starts with 08 or 09.
2) the patch already went in, so we sort of missed that boat.  The next change will be to remove the version check entirely.  

You are right, though, not passing the base was a little sloppy.

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

Is there anything left to do here? Ray?

don't think so. these days, checking if GDM is on the bus *is* right because gdm allows unlocking sessions it didn't start now.