Created attachment 176367 [details] [review]
main: use Gdk.CURRENT_TIME in pushModal()

Right now pushModal() passes global.get_current_time() for
its begin_modal() call.  global.get_current_time() is the
timestamp of the last gdk or clutter event processed by the
shell's mutter process.  These values could potentially be
be too stale to use if pushModal() were to get called in
response to an event by another process.

This commit changes pushModal() to use gdk.CURRENT_TIME instead,
which is the current time as the X server understands it.

Created attachment 176368 [details] [review]
modalDialog: Add modal dialog base class

This is a base class for extensions to subclass
to gain a consistent look in their system dialogs.

It handles creating a darkened backdrop behind the dialog, setting
up buttons in the dialog, keynav, etc.

Created attachment 176369 [details] [review]
runDialog: subclass from modalDialog

Now that we have a modalDialog base class in gnome-shell,
it makes sense to use it for the run dialog.

Note, the run dialog doesn't currently have buttons, so
it isn't exercising all the API of the base class.

Created attachment 176370 [details] [review]
runDialog: animate to new height on error

When an error message is displayed the run dialog
pops to the new height instantly.  It needs a
a transition.

This commit makes the dialog quickly grow to its
ultimate height, making room for the error message,
before showing it.

Do we really want system modal dialogs to be used primarily through extensions?

(In reply to comment #5)
> Do we really want system modal dialogs to be used primarily through extensions?

I don't, but it doesn't really make any difference for these patches. They look useful whether the code for the system dialogs is distributed throughout gnome or shipped with the shell.

I give some justification for using extensions on the SystemDialogs wiki page.

It boils down to:

- There's basically a direct mapping between system dialog and service using that system dialog.  

- In fact, system dialogs should only be directly invoked by parts of the "core os".  One service arbitrates access to its dialog, and clients communicate with that service instead of "conjuring" the dialog directly.

- As the service changes it makes sense to be able to update the system dialog in lock step.  Keeping the code "close to home" facilitates that.

- While we're using the extension system, this is entirely an implementation detail.  In fact, it probably makes sense to add some metadata to the extension or some such that says "this is a system extension" (so it doesn't show up in the list of user extensions, etc).  What i'm saying is, while it leverages the same mechanism as user extensions, these extensions serve a very different purpose and shouldn't be thought of as similar.

- Since the components of the core OS that we need dialogs for are software we have control over (with a common goal of keeping a tight level of integration across components), there's no expectation for ABI/API stability.  If we need to change things in the shell, we can just fix the callers at the same time.

- We can put common between dialogs in the shell tree and leverage it by subclassing.

(also see bug 637188)

(In reply to comment #7)
> I give some justification for using extensions on the SystemDialogs wiki page.
> 
> It boils down to:
> 
> - There's basically a direct mapping between system dialog and service using
> that system dialog.  
> 
> - In fact, system dialogs should only be directly invoked by parts of the "core
> os".  One service arbitrates access to its dialog, and clients communicate with
> that service instead of "conjuring" the dialog directly.
> 
> - As the service changes it makes sense to be able to update the system dialog
> in lock step.  Keeping the code "close to home" facilitates that.
> 
> - While we're using the extension system, this is entirely an implementation
> detail.  In fact, it probably makes sense to add some metadata to the extension
> or some such that says "this is a system extension" (so it doesn't show up in
> the list of user extensions, etc).  What i'm saying is, while it leverages the
> same mechanism as user extensions, these extensions serve a very different
> purpose and shouldn't be thought of as similar.
> 
> - Since the components of the core OS that we need dialogs for are software we
> have control over (with a common goal of keeping a tight level of integration
> across components), there's no expectation for ABI/API stability.  If we need
> to change things in the shell, we can just fix the callers at the same time.
> 
> - We can put common between dialogs in the shell tree and leverage it by
> subclassing.
> 
> (also see bug 637188)

I think it's very neat that you were able to get the extension system going for this, and these are good points. But I don't think it changes the fundamental equation that the interfaces to invoke a dialog are just going to be much smaller and stabler than the interfaces between the dialog and GNOME Shell, and ST, and the GNOME Shell CSS. And at the current time, there is zero guarantee of stability for any of that.

Also, we made the decision *not* to go this way for the status icons, which could be as very similar - the bluetooth status icon doesn't go in gnome-bluetooth, the power status icon doesn't go in gnome-power-manager.

And, at this point, I'd think it's more convenient if there's a single place that our graphic artists can go and find all the stylesheets and images related to the "GNOME Shell" looks. If we decide to change fonts or spacings, I don't want that to involve changes across a bunch of different modules.

I'm happy to take a very hands-off approach to whatever changes are needed to keep the GNOME Shell logout dialog working with gnome-session, etc. If some change to what GNOME Shell exports are needed to keep it working with the current gnome-session I'm OK if you just go ahead and make it ... it doesn't need to block on a big review process.

I can certainly see a role for invisible system-supplied extensions in the future for customization of gnome-shell to non-standard hardware, etc, but really we're just talking about a single code base here - all of GNOME is in the same place and it's no harder to put code in one module than another module. So we should keep it simple for now, and to me that means that the dialogs go in js/ui/systemDialogs rather than using the extension system.

Okay, I've updated bug 637188 with a comment saying we don't want to do logout/reboot as an extension.  We can do it and (other system modal dialogs like say keyring) with all gnome-shell code in-tree.

Given that we're here, what is the correct (designed) implementation of the non-UI part of the system dialogs?

- Run dialog -> fully in the shell (obviously)
- Logout / Restart / Shutdown -> DBus to gnome-session
- PolicyKit -> reimplement the policykit agent or add dbus to polkit-gnome-authentication-agent-1?
- Network secrets -> implement in JS? there was the idea to kill nm-applet when in shell mode
- Keyring dialogs -> should not use DBus to keyring, or else should use peer connections
- other password dialogs -> what to do?

The main problem is that there is no memlock in JS, so we have no way to secure secrets and password going through Javascript UIs (including WPA keys, keyring passwords, root passwords...).
Other problems are code duplication (reimplemented) vs. code bloat (delegated), safety of DBus API, stability of internal protocols and libraries.

(In reply to comment #10)
> - Keyring dialogs -> should not use DBus to keyring, or else should use peer
> connections
> - other password dialogs -> what to do?
> 
> The main problem is that there is no memlock in JS, so we have no way to secure
> secrets and password going through Javascript UIs (including WPA keys, keyring
> passwords, root passwords...).
> Other problems are code duplication (reimplemented) vs. code bloat (delegated),
> safety of DBus API, stability of internal protocols and libraries.

I talked some to Stef Walters about the keyring dialog. My understanding is that the way it works currently is that the keyring daemon, when it needs a password, negotiates encryption keys with the password dialog using Diffie-Hellman key exchange to allow passing the password over DBus without the risk that the DBus message containing the password would be swapped out to disk when being processed by the D-Bus daemon. We can simply extend the length of time that the key is encrypted so that we have:

 ShellPasswordEntry <=> js <=> dbus <=> gnome-keyring-daemon

Where the password is encrpyted by the ShellPasswordEntry (written in C, using memlock) and never decrpyted until it gets to gnome-keyring-daemon.

Retitling to broaden the scope of this bug from "modalDialog base class" to "logout dialog".

Created attachment 177681 [details] [review]
main: use Gdk.CURRENT_TIME in pushModal()

Right now pushModal() passes global.get_current_time() for
its begin_modal() call.  global.get_current_time() is the
timestamp of the last gdk or clutter event processed by the
shell's mutter process.  These values could potentially be
be too stale to use if pushModal() were to get called in
response to an event by another process.

This commit changes pushModal() to use gdk.CURRENT_TIME instead,
which is the current time as the X server understands it.

Created attachment 177682 [details] [review]
modalDialog: Add modal dialog base class

This is a base class for extensions to subclass
to gain a consistent look in their system dialogs.

It handles creating a darkened backdrop behind the dialog, setting
up buttons in the dialog, keynav, etc.

Created attachment 177683 [details] [review]
runDialog: subclass from modalDialog

Now that we have a modalDialog base class in gnome-shell,
it makes sense to use it for the run dialog.

Note, the run dialog doesn't currently have buttons, so
it isn't exercising all the API of the base class.

Created attachment 177684 [details] [review]
runDialog: animate to new height on error

When an error message is displayed the run dialog
pops to the new height instantly.  It needs a
a transition.

This commit makes the dialog quickly grow to its
ultimate height, making room for the error message,
before showing it.

Created attachment 177685 [details] [review]
endSessionDialog: Add logout/shutdown dialog

This commit adds a dialog for gnome-session to
privately use when initiating log outs and shut
downs.

Coordination is done over the bus.

(In reply to comment #14)
> Created an attachment (id=177682) [details] [review]
> modalDialog: Add modal dialog base class
> 
> This is a base class for extensions to subclass
> to gain a consistent look in their system dialogs.
oops, I need to redo this commit message, since we aren't going the way of extensions anymore.

Created attachment 177686 [details] [review]
modalDialog: Add modal dialog base class

This is a base class to make it easier to
gain a consistent look for system modal dialogs.

It handles creating a darkened backdrop behind the dialog, setting
up buttons in the dialog, keynav, etc.

Review of attachment 177681 [details] [review]:

Need to find an alternate approach here. In generall, I thnk if a caller wants gnome-shell to display a dialog on its behalf, then it should provide an event timestamp, just like an application needs to provide an appropriate timestamp for each new window.

Review of attachment 177686 [details] [review]:

::: js/ui/modalDialog.js
@@ +1,2 @@
+
+/* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- */

Stray blank first line

@@ +26,3 @@
+ModalDialog.prototype = {
+    _init: function(properties) {
+        this._isClosed = true;

Reverse this to isOpen

@@ +52,3 @@
+        if (properties && properties['style_class']) {
+            this._dialogLayout.add_style_class_name(properties['style_class']);
+        }

Use Params for this, 

  params = Params.parse(params, { styleClass: null });
  [...]

  if (params.styleClass != null) {
       this._dialogLayout.add_style_class_name(params.styleClass);
  }

(Note javascript style naming

@@ +63,3 @@
+
+        this._buttonLayout = new St.BoxLayout({ opacity:  220,
+                                               vertical: false });

alignment issues (we aren't 100% consistent, but the start of the property names should definitely line up. We don't generally do the column alignment that you have here for properties, but it's fine as well.)

@@ +82,3 @@
+                                         can_focus:   true,
+                                         label:       label });
+            let x_alignment;

Weird to have no blank line before and a blank line after - it's clearly more connected to the following code

@@ +89,3 @@
+                x_alignment = St.Align.START;
+            else if (i == buttons.length - 1)
+                x_alignment = St.Align.END;

This isn't exactly right - with 4 equal sized buttons, you'll get:

 [Button]      [Button]    [Button]        [Button]

That is, more space at the edges then in the middle, but >= 4 buttons is a UI disaster anyways, so OK.

@@ +102,3 @@
+
+            button.connect('clicked',
+                            Lang.bind(this, function() { this._onButtonClicked(action); }));

Why not simply:

 button.connect('clicked', action);

?

@@ +117,3 @@
+        if (action) {
+            action();
+        }

Excess {}

@@ +157,3 @@
+
+        if (!this._group.has_key_focus())
+            return false;

Needs a comment explaining why you are checking this - in what circumstances it might fail. You also need to popModal on this return.

@@ +181,3 @@
+    },
+
+    _fadeOutDialog: function() {

Not used

@@ +187,3 @@
+        Tweener.addTween(this._dialogLayout,
+                         { opacity: 0,
+                           time:    1.0,

Number here rather than DIALOG_FADE_TIME or a different constant

@@ +191,3 @@
+    },
+
+    _fadeInDialog: function() {

Not used

Review of attachment 177683 [details] [review]:

Generally looks good

::: js/ui/runDialog.js
@@ +220,3 @@
         this._entryText = entry.clutter_text;
+        this._contentLayout.add(entry, { y_align: St.Align.START });
+        this._initialKeyFocus = this._entryText;

In my opinion, private members with a leading underscore should be considered private to the class not "protected" and API should be added for any manipulation that is needed. Something like contentLayout which is accessed read-only could be lose the initial underscore.

Actually, for _initialKeyFocus, maybe the best thing would be a "vfunc" - let the subclass do whatever it wants on initial key focus?

Review of attachment 177684 [details] [review]:

one comment, leaving the rest to danw to review

::: js/ui/runDialog.js
@@ +363,3 @@
                     this._errorMessage.set_text(errorStr);
 
+                    if (!(this._errorBox.get_flags() & Clutter.ActorFlags.VISIBLE)) {

this._errorBox.visible

Review of attachment 177685 [details] [review]:

Looks great. Lots of style comments, a few more substantial things embedded.

::: js/misc/gnomeSession.js
@@ +72,3 @@
+}
+
+Inhibitor.prototype = {

Hmm, think you should have a comment here noting that you aren't handling change notification and explaining why that doesn't matter.

::: js/ui/endSessionDialog.js
@@ +1,3 @@
+/* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*-
+ *
+ * Copyright (C) 2010 Red Hat, Inc

Omit the (C) - it has no legal meaning.

@@ +27,3 @@
+
+const Clutter = imports.gi.Clutter;
+const Gio = imports.gi.Gio;

Not used

@@ +30,3 @@
+const Gdm = imports.gi.Gdm;
+const GLib = imports.gi.GLib;
+const GObject = imports.gi.GObject;

Not used

@@ +32,3 @@
+const GObject = imports.gi.GObject;
+const Gtk = imports.gi.Gtk;
+const Meta = imports.gi.Meta;

Not used

@@ +37,3 @@
+const Shell = imports.gi.Shell;
+
+const GnomeSession = imports.misc.gnomeSession

Should be mixed with the imports.ui imports

@@ +62,3 @@
+};
+
+const LogoutDialogContent = {

Not sure our exact rules here, but for a constant that isn't an "enum" or "interface", the leading capital is wrong. I'd probably name this as logoutDialogContent and save ALL_CAPS for stuff numeric/string/color constants

@@ +103,3 @@
+    let candidateDesktopFiles = [];
+    let appSystem;
+    let app;

Define variables where you use them, not at a block at the top. Where it makes sense, do 'let appSystem = ....'

@@ +108,3 @@
+
+    if (!GLib.str_has_suffix(desktopFile, '.desktop'))
+      desktopFile = desktopFile + '.desktop';

Can use +=, which I think is slightly clearer here

@@ +121,3 @@
+                break;
+        } catch(e) {
+            app = null;

There is no point to this, if get_app() throws, then app will not have been assigned

@@ +137,3 @@
+        this._reason = reason;
+
+        if (!this._reason)

Rather see == null here, don't think you really intended (or thought about) whether '' would trigger this or not.

@@ +140,3 @@
+          this._reason = '';
+
+        let layout = new St.BoxLayout({ vertical:    false});

Weird alignment; columns are OK in a single property list, but definitely not OK over multiple

@@ +173,3 @@
+        this.actor.connect('clicked',
+                           Lang.bind(this, function() {
+                               this._onClicked();

Looks like you mean Lang.bind(this, this._onClicked) - the anon function serves no role here

@@ +185,3 @@
+Signals.addSignalMethods(ListItem.prototype);
+
+function _roundSecondsToInterval(totalSeconds, secondsLeft, interval) {

Needs an explanatory comment for what this function is about

@@ +191,3 @@
+    if (totalSeconds > interval
+        && ((totalSeconds - secondsLeft) < (totalSeconds % interval))) {
+        time = Math.ceil(totalSeconds);

If I'm interpreting this correctly, it would be vastly clearer to just tack a 

 if (time > totalSeconds)
    time = Math.ceil(totalSeconds);

onto the other branch

@@ +213,3 @@
+    }
+
+    log('setting label ' + label + ' text to "' + text + '"');

Remove

@@ +217,3 @@
+
+function EndSessionDialog() {
+    if (_endSessionDialog == null) {

Define this with a let at the global scope

@@ +244,3 @@
+        this._secondsLeft = 0;
+        this._totalSecondsToStayOpen = 0;
+        this._inhibitors=[];

Missing spaces

@@ +252,3 @@
+
+        this._userLoadedId = this._user.connect('notify::is_loaded',
+                                                Lang.bind(this, function() { this._updateContent(); }));

Pointless anonymous function

@@ +255,3 @@
+
+        this._userChangedId = this._user.connect('changed',
+                                                 Lang.bind(this, function() { this._updateContent(); }));

And here

@@ +309,3 @@
+        if (styleClass) {
+            this._iconBin.set_style_class_name(styleClass);
+        }

Pointless {}

@@ +324,3 @@
+        if (styleClass) {
+            this._iconBin.set_style_class_name(styleClass);
+        }

Pointless {}

@@ +332,3 @@
+                                                   iconName,
+                                                   St.IconType.SYMBOLIC,
+                                                   32);

If not going to pull from CSS, this should be a constant (which means making your _ICON_SIZE constant more specific)

@@ +343,3 @@
+
+    _updateContent: function() {
+

Various places where you have this initial blank line in functions, shouldn't be there

@@ +346,3 @@
+        if (!(this._type in DialogContent)) {
+            return;
+        }

You should make OpenAsync a no-op in this case, not get all the way here with an invalid value

@@ +387,3 @@
+            if (!description) {
+                description = dialogContent.uninhibitedDescription.format(displayTime);
+            }

Pointless {}

@@ +399,3 @@
+
+        if (!(this._type in DialogContent)) {
+            return;

Again should have been caught earlier

@@ +407,3 @@
+        if (dialogContent.confirmButtonText) {
+            confirmButtonText = dialogContent.confirmButtonText;
+        }

Pointless {}. do the else as an else

@@ +410,3 @@
+
+        this.setButtons([{ label: _("Cancel"),
+                           action: Lang.bind(this, function() { this.cancel(); }),

Pointless anon function

@@ +414,3 @@
+                         },
+                         { label:  confirmButtonText,
+                           action: Lang.bind(this, function() { this._confirm(); })

Pointles anon function

@@ +436,3 @@
+    _onOpened: function() {
+        if (this._inhibitors.length == 0) {
+            this._startTimer();

You like those pointless {} don't you :-)

@@ +484,3 @@
+    OpenAsync: function(type, totalSecondsToStayOpen, inhibitorObjectPaths, callback) {
+        this._totalSecondsToStayOpen = totalSecondsToStayOpen;
+        this._inhibitors=[];

Missing spaces

@@ +496,3 @@
+            inhibitor.connect('is-loaded',
+                              Lang.bind(this, function() {
+                                  this._onInhibitorLoaded(inhibitor);

Problems if OpenAsync is called again before is-loaded fires for the current set of indicators, could do:

 if (this_inhibitors.indexOf(inhibitor) >= 0) {
 }

@@ +503,3 @@
+        if (!this.open()) {
+            throw new DBus.DBusError('org.gnome.Shell.ModalDialog.GrabError',
+                                     "Error taking control of the session");

Confusing message - something like "Cannot grab pointer and keyboard" would be better - either way if it show to the user something is badly wrong

@@ +506,3 @@
+        }
+
+        this.connect('opened',

You are getting another copy of this connection every time OpenAsync is called, and will call the old callbacks after they are dead

@@ +508,3 @@
+        this.connect('opened',
+                     Lang.bind(this, function() {
+                         callback();

Pointles bind, pointless anon function

Review of attachment 177684 [details] [review]:

::: js/ui/runDialog.js
@@ +364,3 @@
 
+                    if (!(this._errorBox.get_flags() & Clutter.ActorFlags.VISIBLE)) {
+                        let [errorBoxMinHeight, errorBoxNaturalHeight] = this._errorBox.get_preferred_height(-1);

So, talking to danw, this was the main concern we had - that the width that might be used could give a different preferred height than -1, since the contents of errorBox wrap. Now, it turns out that the error message actually never wraps - we *don't* set any explicit width so the natural width of the error message wins.

Considering that, and that doing things the right way is a lot of code -  it would involve writing a StGenericContainer "subclass" with signal connections that has a property that determines the fraction of its child's height that it should request (or doing the same thing in C as a StBin subclass), I think this is OK like this.

OK to commit with the above fix for visible.

(In reply to comment #20)
> Review of attachment 177681 [details] [review]:
> 
> Need to find an alternate approach here. In generall, I thnk if a caller wants
> gnome-shell to display a dialog on its behalf, then it should provide an event
> timestamp, just like an application needs to provide an appropriate timestamp
> for each new window.
Okay.  It's going to complicate things a bit (since the logout dialog can be conjured via XSMP-fu, the timestamp of the originating user event isn't always available), but I'll see what I can come up with.

(In reply to comment #21)
> @@ +26,3 @@
> +ModalDialog.prototype = {
> +    _init: function(properties) {
> +        this._isClosed = true;
> 
> Reverse this to isOpen
I don't think I can just invert it here.  There's an implicit "opening" state, so isClosed actually means isNotOpenOrOpening.

I've updated it to use a full enum, which helps address other areas of your review, too

(In reply to comment #22)
> Review of attachment 177683 [details] [review]:
> ::: js/ui/runDialog.js
> @@ +220,3 @@
>          this._entryText = entry.clutter_text;
> +        this._contentLayout.add(entry, { y_align: St.Align.START });
> +        this._initialKeyFocus = this._entryText;
> 
> In my opinion, private members with a leading underscore should be considered
> private to the class not "protected" and API should be added for any
> manipulation that is needed. Something like contentLayout which is accessed
> read-only could be lose the initial underscore.
Okay, i've dropped the leading underscore in contentLayout.  I wish we had a more rigid convention for protected members though.

> Actually, for _initialKeyFocus, maybe the best thing would be a "vfunc" - let
> the subclass do whatever it wants on initial key focus?
Well, we already emit a signal at around that time, so I've changed it to just hook into that instead.

Review of attachment 177685 [details] [review]:

::: js/misc/gnomeSession.js
@@ +81,3 @@
+        for (let i = 0; i < InhibitorIface.properties.length; i++) {
+            let propertyName = InhibitorIface.properties[i].name;
+            this.GetRemote(propertyName, Lang.bind(this,

Just a random comment: can't you use GetAllRemote, instead of querying each property? It will help with race conditions also.

(In reply to comment #26)
> (In reply to comment #20)
> > Review of attachment 177681 [details] [review] [details]:
> > 
> > Need to find an alternate approach here. In generall, I thnk if a caller wants
> > gnome-shell to display a dialog on its behalf, then it should provide an event
> > timestamp, just like an application needs to provide an appropriate timestamp
> > for each new window.
> Okay.  It's going to complicate things a bit (since the logout dialog can be
> conjured via XSMP-fu, the timestamp of the originating user event isn't always
> available), but I'll see what I can come up with.

If you absolutely don't have a timestamp, then using CURRENT_TIME or getting a timestamp from the server with a roundtrip may be the only approach *for that case*, but that doesn't justify just dumping all timestamp information on the floor and always using CURRENT_TIME.

(In reply to comment #24)
> Not sure our exact rules here, but for a constant that isn't an "enum" or
> "interface", the leading capital is wrong. I'd probably name this as
> logoutDialogContent and save ALL_CAPS for stuff numeric/string/color constants
I've changed it, but that seems a little weird to me.  What makes the DBus interface array "special" here (versus any other data array)? 

> @@ +173,3 @@
> +        this.actor.connect('clicked',
> +                           Lang.bind(this, function() {
> +                               this._onClicked();
> 
> Looks like you mean Lang.bind(this, this._onClicked) - the anon function serves
> no role here
The main reason i had this (and all the others) was to keep a common idiom for signal connections. e.g. so the code doesn't change a lot just because the function getting gains a parameter that's not expected by the signal or whatever.

I've removed them now, though.

> If I'm interpreting this correctly, it would be vastly clearer to just tack a 
> 
>  if (time > totalSeconds)
>     time = Math.ceil(totalSeconds);
> 
> onto the other branch
I've cleaned up the function now.

> @@ +217,3 @@
> +
> +function EndSessionDialog() {
> +    if (_endSessionDialog == null) {
> 
> Define this with a let at the global scope
I do already, right?


> @@ +346,3 @@
> +        if (!(this._type in DialogContent)) {
> +            return;
> +        }
> 
> You should make OpenAsync a no-op in this case, not get all the way here with
> an invalid value
Well this check isn't just for OpenAsync.  It's also for the "dialog isn't opened yet" case (since updateContent is called in response to various asynchronous events).

I've made OpenAsync return an error for unknown types, and changed this code to explicitly check the state of the dialog instead of relying on this._type.

(In reply to comment #29)
> Review of attachment 177685 [details] [review]:
> 
> ::: js/misc/gnomeSession.js
> @@ +81,3 @@
> +        for (let i = 0; i < InhibitorIface.properties.length; i++) {
> +            let propertyName = InhibitorIface.properties[i].name;
> +            this.GetRemote(propertyName, Lang.bind(this,
> 
> Just a random comment: can't you use GetAllRemote, instead of querying each
> property? It will help with race conditions also.

I'm pretty sure I tried GetAll at first and it wasn't viable, but I don't remember the details.  I'll have to play around again and let you know.

Created attachment 177769 [details] [review]
main: add timestamp parameter to push/popModal

Right now popModal() passes global.get_current_time() for
its begin_modal() call.  global.get_current_time() is the
timestamp of the last gdk or clutter event processed by the
shell's mutter process.  These values could potentially be
be too stale to use if pushModal() were to get called in
response to an event by another process.

This commit changes pushModal() to have an optional timestamp
argument, which can be used to associate the call with the
event that initiated it.

Created attachment 177770 [details] [review]
modalDialog: Add modal dialog base class

This is a base class to make it easier to
gain a consistent look for system modal dialogs.

It handles creating a darkened backdrop behind the dialog, setting
up buttons in the dialog, keynav, etc.

Created attachment 177771 [details] [review]
runDialog: subclass from modalDialog

Now that we have a modalDialog base class in gnome-shell,
it makes sense to use it for the run dialog.

Note, the run dialog doesn't currently have buttons, so
it isn't exercising all the API of the base class.

Created attachment 177772 [details] [review]
runDialog: animate to new height on error

When an error message is displayed the run dialog
pops to the new height instantly.  It needs a
a transition.

This commit makes the dialog quickly grow to its
ultimate height, making room for the error message,
before showing it.

Created attachment 177773 [details] [review]
endSessionDialog: Add logout/shutdown dialog

This commit adds a dialog for gnome-session to
privately use when initiating log outs and shut
downs.

Coordination is done over the bus.

Review of attachment 177769 [details] [review]:

Looks good

Review of attachment 177770 [details] [review]:

One piece of missing docs, otherwise looks good to commit to me.

::: js/ui/modalDialog.js
@@ +202,3 @@
+                           onComplete: Lang.bind(this,
+                               function() {
+                                   this.state = State.FADED_OUT;

This is a weird state - from this state you can either close or open the dialog? Please add an explanatory comment to _fadeOutDialog that is sufficient so that someone reading the code will understand why this state makes sense... (in fact, since _fadeOutDialog is only here for future subclasses to call, apparently, and not used in this patch, it very much needs docs in any case - you can't have a patch that adds an undocumented function that nothing uses.)

Review of attachment 177771 [details] [review]:

OK

Review of attachment 177772 [details] [review]:

good to commit

Review of attachment 177773 [details] [review]:

Looks good, two tiny things to fix before committing.

::: js/ui/endSessionDialog.js
@@ +461,3 @@
+            this._stopTimer();
+        } else {
+            log("Service with desktop file " + inhibitor.app_id + " inhibited endSession: " + inhibitor.reason);

Left-over log - we dont' have a debug logging facility at the moment, so anything you put into log() will end up in xsession-errors

@@ +470,3 @@
+    OpenAsync: function(type, timestamp, totalSecondsToStayOpen, inhibitorObjectPaths, callback) {
+        this._totalSecondsToStayOpen = totalSecondsToStayOpen;
+        this._inhibitors=[];

Missing spaces

Alright, it's in now.

Now we just need to get the gnome-session half in and the css work in bug 636976