GNOME Bugzilla – Bug 654770
Add necessary infrastructure to support extensions website
Last modified: 2011-08-24 17:59:48 UTC
This is most of the necessary infrastructure that I've been working on to support the extension website (SweetTooth-WWW). Most of it has been rebased/squashed to not commit myself to "bug hell" as I unleashed upon the unfateful reviewers previously. I'm sorry. Three things of note: 1. The website's source is currently at: https://github.com/magcius/sweettooth 2. The website will talk to this DBus API through an NPAPI plugin: https://github.com/magcius/sweettooth-plugin 3. All the relevant patches here should be up to date with a GitHub branch if you prefer their UI: https://github.com/magcius/gnome-shell/tree/extension-work
Created attachment 192109 [details] [review] lookingGlass: Recognize new extensions as they are added https://bugzilla.gnome.org/show_bug.cgi?id=652614
Created attachment 192110 [details] [review] shellDBus: Add ListExtensions() and GetExtensionInfo() GetExtensionInfo() takes a UUID and returns a JSON object with information about that extension including their metadata, path and current state. ListExtensions() takes no arguments and returns a JSON object mapping UUIDs to the same information objects described above. https://bugzilla.gnome.org/show_bug.cgi?id=653207
Created attachment 192111 [details] [review] shellDBus: Add a few version parameters Add ShellVersion, designed for detecting OUT_OF_DATE extensions so they can't be installed, as well as ApiVersion, designed for backwards-compatibility with the SweetTooth web-app, which must support all shell versions.
Created attachment 192112 [details] [review] extensionSystem: Save extension errors per-extension Extension developers may be confused about why their extensions aren't working: the LookingGlass isn't a very obvious place, or even which errors are theirs. To remedy this, save all errors per-UUID which allows them to be retrieved later. https://bugzilla.gnome.org/show_bug.cgi?id=653208
Created attachment 192113 [details] [review] extensionSystem: Implement new live enable/disable system The rough sketches of the system are here: http://mail.gnome.org/archives/gnome-shell-list/2011-June/msg00283.html https://bugzilla.gnome.org/show_bug.cgi?id=653209
Created attachment 192114 [details] [review] extensionSystem: Add install-from-HTTP capability This adds a new DBus method: InstallExtensionRemote(url : s) Pass it the URL of a manifest file: the same as a metadata.json, but with a special key, '__installer', and use it to install the extension. In the future, the manifest file may be used to automatically detect and install updates.
Created attachment 192115 [details] [review] extensionSystem: Add 'extension-status-changed' signal
Created attachment 192116 [details] [review] extensionSystem: Add a DOWNLOADING state
Created attachment 192117 [details] [review] extensionSystem: Start using OUT_OF_DATE We were defining OUT_OF_DATE as a possible state, but never using it properly.
Comment on attachment 192114 [details] [review] extensionSystem: Add install-from-HTTP capability >+const _httpSession = new Soup.SessionAsync(); add: _httpSession.add_feature(new Soup.ProxyResolverDefault()); so it will work for people with HTTP proxies >+ logError(uuid, '%s is not compatible with current GNOME Shell version'.format(name)); logError()'s first argument is supposed to be an Error (ie, a thrown exception). Also, you don't need to use format() for things like this. You can just do name + ' is not compatible with current GNOME Shell version' >+ let url = manifest['__installer']; >+ _httpSession.queue_message( >+ Soup.Message.new('GET', url), probably want to make sure url is on the same server as the manifest was downloaded from or something? >+ let tmpzip = '/tmp/%s.shell-extension.zip'.format(uuid); >+ let f = Gio.file_new_for_path(tmpzip); boo. Use a safe temp file creating method, and don't put uuid into the pathname since you don't control it.
(In reply to comment #10) > (From update of attachment 192114 [details] [review]) > > >+const _httpSession = new Soup.SessionAsync(); > > add: > > _httpSession.add_feature(new Soup.ProxyResolverDefault()); This poops out: (lt-gnome-shell-real:21997): libsoup-WARNING **: soup-session.c:1000: invalid property id 13 for "add-feature" of type `GParamObject' in `SoupSessionAsync' You seem to have added a property of the same name and didn't fill in the getter, and gobject-introspection is picking up the property instead of the method. I'm not sure I like the property API: _httpSession.add_feature = new Soup.ProxyResolverDefault(); It just looks wrong. So you should probably fix that (remove the property-based API or something). For old version compatibility I can try and wrap it in a try/except, but that will still trigger the WARN_INVALID_PROPERTY_ID. > so it will work for people with HTTP proxies > > >+ logError(uuid, '%s is not compatible with current GNOME Shell version'.format(name)); > > logError()'s first argument is supposed to be an Error (ie, a thrown > exception). A previous patch replaced added a separate logError function that's not global.logError. I should probably rename the new function "logExtensionError" > Also, you don't need to use format() for things like this. You can just do > > name + ' is not compatible with current GNOME Shell version' Sure. Is there a style preference on whether to use one or the other? Only use concatenation for extremely simple cases? > >+ let url = manifest['__installer']; > >+ _httpSession.queue_message( > >+ Soup.Message.new('GET', url), > probably want to make sure url is on the same server as the manifest was > downloaded from or something? Not sure we want this -- it wouldn't take much to get around it (server redirect) and would kill any static file caching (CDN or such). > >+ let tmpzip = '/tmp/%s.shell-extension.zip'.format(uuid); > >+ let f = Gio.file_new_for_path(tmpzip); > > boo. Use a safe temp file creating method, and don't put uuid into the pathname > since you don't control it. glib/gio isn't making this easy, so I'm just creating a filename from Math.random() and the standard 36-char alphabet. I'm not sure it's worth devoting time to making a GFile tmpfile()/mktemp() wrapper in gio itself but if other people want it, I may look into it.
John: you expressed interest in a good DBus API for extensions for use in gnome-tweak-tool. Can you quickly go over these patches and see if this is fine?
(In reply to comment #12) > John: you expressed interest in a good DBus API for extensions for use in > gnome-tweak-tool. Can you quickly go over these patches and see if this is > fine? I just looked over them. I see you now return the state in Enable/DisableExtension, so that should be fine with me. I like that ListExtensions returns all the info for all the extensions, so we do not have to call GetExtensionInfo in a loop. In conclusion; looks good! I'm looking forward to this landing.
(In reply to comment #10) > (From update of attachment 192114 [details] [review]) > > >+const _httpSession = new Soup.SessionAsync(); > > add: > > _httpSession.add_feature(new Soup.ProxyResolverDefault()); > > so it will work for people with HTTP proxies Due to difficulties implementing this that has required changed in gobject-introspection, libsoup and gjs, I'm going to punt on this for now.
Created attachment 192706 [details] [review] extensionSystem: Add install-from-HTTP capability This adds a new DBus method: InstallExtensionRemote(url : s) Pass it the URL of a manifest file: the same as a metadata.json, but with a special key, '__installer', and use it to install the extension. In the future, the manifest file may be used to automatically detect and install updates.
Created attachment 192707 [details] [review] extensionSystem: Add install-from-HTTP capability This adds a new DBus method: InstallExtensionRemote(url : s) Pass it the URL of a manifest file: the same as a metadata.json, but with a special key, '__installer', and use it to install the extension. In the future, the manifest file may be used to automatically detect and install updates.
Comment on attachment 192706 [details] [review] extensionSystem: Add install-from-HTTP capability Accidentally uploaded twice. Otherwise, can I ask some people to do their cursory reviews here so I can start getting some of this stuff in? Giovanni, drago01?
Review of attachment 192109 [details] [review]: ::: js/ui/extensionSystem.js @@ +180,3 @@ extensionMeta[meta.uuid].state = ExtensionState.ENABLED; + + _signals.emit('extension-loaded', meta.uuid); If you use _signals.emit.call(undefined, ...), you can avoid leaking an implementation detail. ::: js/ui/lookingGlass.js @@ +653,3 @@ }, + _loadExtension: function(uuid) { The first parameter of a signal handler is the object emitting the signal.
Review of attachment 192110 [details] [review]: Looks good (for now). It will require some work when porting to GDBus, given that JSON object are not treated specially there, but I consider that ok.
Review of attachment 192111 [details] [review]: LGTM
Review of attachment 192112 [details] [review]: ::: js/ui/extensionSystem.js @@ +75,3 @@ } +function logError(uuid, message) { Do we allow different log levels (info, warning, error) or just error? @@ +86,3 @@ let metadataFile = dir.get_child('metadata.json'); if (!metadataFile.query_exists(null)) { + global.logError('Missing metadata.json'); I think this should mention the uuid of the failing extension somewhere. @@ +191,3 @@ if (stylesheetPath != null) theme.unload_stylesheet(stylesheetPath); + logError(uuid, 'Failed to evaluate init function:' + e); I think it should say "main" here (not really important, as it will change in next patch anyway).
Review of attachment 192113 [details] [review]: ::: js/ui/extensionSystem.js @@ +238,3 @@ return; } + if (!extensionModule.enable) { Why do you require enable() at the top level? Isn't it enough to have it in the state object?
Review of attachment 192115 [details] [review]: ::: js/ui/extensionSystem.js @@ +189,3 @@ global.logError('Extension "%s" had error: %s'.format(uuid, message)); + _signals.emit('extension-state-changed', { uuid: uuid, + error: message, This doesn't seem to be used. You should either use it, or drop it and pass a real extensionMeta object.
Review of attachment 192116 [details] [review]: LGTM (even though I don't understand why installed extensions should be "downloading" when starting the shell)
Review of attachment 192117 [details] [review]: LGTM
Created attachment 192995 [details] [review] lookingGlass: Recognize new extensions as they are added > ::: js/ui/extensionSystem.js > @@ +180,3 @@ > extensionMeta[meta.uuid].state = ExtensionState.ENABLED; > + > + _signals.emit('extension-loaded', meta.uuid); > > If you use _signals.emit.call(undefined, ...), you can avoid leaking an > implementation detail. No I can't; signals.js uses "this" very clearly in _emit: if(!('_signalConnections' in this)) return; > ::: js/ui/lookingGlass.js > @@ +653,3 @@ > }, > > + _loadExtension: function(uuid) { > > The first parameter of a signal handler is the object emitting the signal. Oops. Fixed.
Created attachment 193007 [details] [review] extensionSystem: Save extension errors per-extension Extension developers may be confused about why their extensions aren't working: the LookingGlass isn't a very obvious place, or even which errors are theirs. To remedy this, save all errors per-UUID which allows them to be retrieved later. As I suggested, I renamed logError to logExtensionError to prevent confusion. > ::: js/ui/extensionSystem.js > @@ +75,3 @@ > } > > +function logError(uuid, message) { > > Do we allow different log levels (info, warning, error) or just error? log(Extension)?Error is mainly to set the ERROR state. If we wanted to have an API for extensions to log infos, warnings, etc., it would be separate. > @@ +86,3 @@ > let metadataFile = dir.get_child('metadata.json'); > if (!metadataFile.query_exists(null)) { > + global.logError('Missing metadata.json'); > > I think this should mention the uuid of the failing extension somewhere. Done. I use the basename of the dir. > @@ +191,3 @@ > if (stylesheetPath != null) > theme.unload_stylesheet(stylesheetPath); > + logError(uuid, 'Failed to evaluate init function:' + e); > > I think it should say "main" here (not really important, as it will change in > next patch anyway). It's not too hard. Fixed.
Created attachment 193008 [details] [review] extensionSystem: Implement new live enable/disable system The rough sketches of the system are here: http://mail.gnome.org/archives/gnome-shell-list/2011-June/msg00283.html > ::: js/ui/extensionSystem.js > @@ +238,3 @@ > return; > } > + if (!extensionModule.enable) { > > Why do you require enable() at the top level? > Isn't it enough to have it in the state object? Whoops, my bad. I now check it on the state object.
Created attachment 193009 [details] [review] extensionSystem: Add install-from-HTTP capability This adds a new DBus method: InstallExtensionRemote(url : s) Pass it the URL of a manifest file: the same as a metadata.json, but with a special key, '__installer', and use it to install the extension. In the future, the manifest file may be used to automatically detect and install updates. Attached for rebase.
Created attachment 193010 [details] [review] extensionSystem: Add 'extension-status-changed' signal > > ::: js/ui/extensionSystem.js > @@ +189,3 @@ > global.logError('Extension "%s" had error: %s'.format(uuid, message)); > + _signals.emit('extension-state-changed', { uuid: uuid, > + error: message, > > This doesn't seem to be used. You should either use it, or drop it and pass a > real extensionMeta object. If by this you mean the errror message, fixed.
Created attachment 193011 [details] [review] extensionSystem: Add a DOWNLOADING state > LGTM (even though I don't understand why installed extensions should be > "downloading" when starting the shell) They're not?
Created attachment 193012 [details] [review] extensionSystem: Start using OUT_OF_DATE We were defining OUT_OF_DATE as a possible state, but never using it properly. Attached for rebase.
Review of attachment 192995 [details] [review]: ::: js/ui/lookingGlass.js @@ +668,3 @@ + }, + + _onExtensionLoaded: function(o, uuid) { Can you merge this function with _loadExtension, and just pass a dummy parameter at startup?
Review of attachment 193009 [details] [review]: ::: js/ui/extensionSystem.js @@ +143,3 @@ + Shell.write_soup_message_to_stream(stream, message); + stream.close(null); + GLib.spawn_sync(null, ['unzip', '-d', dir.get_path(), '--', tmpzip], null, This is synchronous and has the potential to keep the shell blocked for a long time. I think it should be rewritten using GLib.spawn_async + GChildWatch.
OK. One more thing -- before this lands, I'm going to make one last ML post for extension developers, but would you mind porting the extensions in the gnome-shell-extensions if you're not busy, Giovanni?
(In reply to comment #35) > OK. One more thing -- before this lands, I'm going to make one last ML post for > extension developers, but would you mind porting the extensions in the > gnome-shell-extensions if you're not busy, Giovanni? Ok, I'll start porting and publish a branch as soon as I have something.
Actually, for some extensions this is straightforward. For those that hook inside functions and override stuff, what happened to that handy MonkeyPatch class?
(In reply to comment #37) > Actually, for some extensions this is straightforward. For those that hook > inside functions and override stuff, what happened to that handy MonkeyPatch > class? I thought we both decided on IRC that it wasn't useful enough to be used. I found a "monkeypatch.js" in one of my folders, so I uploaded it. No warranty, though. http://p.mecheye.net/monkeypatch.js/0
Attachment 192110 [details] pushed as fc59e22 - shellDBus: Add ListExtensions() and GetExtensionInfo() Attachment 192111 [details] pushed as 2466eb3 - shellDBus: Add a few version parameters Attachment 192995 [details] pushed as ff98343 - lookingGlass: Recognize new extensions as they are added Attachment 193007 [details] pushed as 5810fcb - extensionSystem: Save extension errors per-extension Pushing up to live enable/disable, waiting on porting extensions.
Comment on attachment 193009 [details] [review] extensionSystem: Add install-from-HTTP capability >+ let tmpzip = '/tmp/%s.shell-extension.zip'.format(randomLetters(6)); >+ let f = Gio.file_new_for_path(tmpzip); >+ let stream = f.replace(null, false, // FIXME: use a GFile mkstemp-type method once one exists ok to commit after that
(In reply to comment #34) > Review of attachment 193009 [details] [review]: > > ::: js/ui/extensionSystem.js > @@ +143,3 @@ > + Shell.write_soup_message_to_stream(stream, message); > + stream.close(null); > + GLib.spawn_sync(null, ['unzip', '-d', dir.get_path(), '--', > tmpzip], null, > > This is synchronous and has the potential to keep the shell blocked for a long > time. I think it should be rewritten using GLib.spawn_async + GChildWatch. OK, I've tried this, but spawn_async doesn't mark the @data param as a closure, so it fails. For now, I'd rather use spawn_sync and land this, but we can fix that later. Is this branch complete? http://git.gnome.org/browse/gnome-shell-extensions/log/?h=extension-live-disable
Created attachment 193803 [details] [review] extensionSystem: Add install-from-HTTP capability This adds a new DBus method: InstallExtensionRemote(url : s) Pass it the URL of a manifest file: the same as a metadata.json, but with a special key, '__installer', and use it to install the extension. In the future, the manifest file may be used to automatically detect and install updates. > OK, I've tried this, but spawn_async doesn't mark the @data param as a closure, > so it fails. This was all sorts of wrong -- first, I meant g_child_watch_add, but it turns out that's not the reason. g_child_watch_add_full is marked "Rename to:", so I had to pass an extra priority parameter. Thanks to Colin Walters for helping me find it!
Created attachment 193829 [details] [review] extensionSystem: Implement new live enable/disable system The rough sketches of the system are here: http://mail.gnome.org/archives/gnome-shell-list/2011-June/msg00283.html The only thing different here is that I updated the gnome-shell-extension-tool sample.
Created attachment 194005 [details] [review] extensionSystem: Add install-from-HTTP capability This adds a new DBus method: InstallExtensionRemote(url : s) Pass it the URL of a manifest file: the same as a metadata.json, but with a special key, '__installer', and use it to install the extension. In the future, the manifest file may be used to automatically detect and install updates. - let explicitlyDisabled = disabledExtensions.indexOf(uuid) == 0; + let explicitlyDisabled = disabledExtensions.indexOf(uuid) >= 0; Typo that accidentally broke disabled-extensions.
Giovanni, ping re: live enabling/disabling branch?
Created attachment 194126 [details] [review] extensionSystem: Remove 'disabled-extensions' blacklist The two similar keys were hard to manipulate to have specific effects, so just remove one. Now there is an *explicit* whitelist: all extensions must be in the 'enabled-extensions' for them to be loaded. -- OK, this is a somewhat new approach. For gnome-session's fail-whale-dialog, I needed to frob the gsettings keys directly. I had half-assed it in the live enable/disable code, as I just completely ignored the explicit whitelist there, and just went after frobbing the blacklist. It's just significantly hard to sync up with what you want. I talked to Tassilo, who initially made the extra whitelist, on IRC, and we were both fine about getting rid of one of the lists. Given the choice between blacklist and whitelist, I think whitelist is the better choice: users should have to explicitly turn on Shell extensions that have been installed. Also, getting rid of the two separate lists simplified a number of things, and as a bonus I can easily detect through a settings change which extensions have been enabled/disabled. Now, I listen to when the 'enabled-extensions' setting changes to try enabling/ disabling extensions. The implications: * Currently, users manually installing shell extensions (or distro packaged extensions) packages need to flip a bit to enable the extension. I am not sure if the distro packages can find a way to frob gsettings after installation, but I'm certain they have the capability. * Anyone (read: John Stowers) who was planning on using the EnableExtension/ DisableExtension API, it has changed. Now these two methods just frob gsettings. They will not "block" until the extension has been loaded and they will not return the extension's new state. Connect to the "ExtensionStatusChanged" signal instead.
Review of attachment 193829 [details] [review]: Looks good to me.
Review of attachment 194126 [details] [review]: I'm afraid people will complain, if this is pushed without some kind of transitioning mechanism. Maybe it should be postponed to 3.4, given that "enabled-extensions" only appeared in 3.1/3.2. (Code looks good, though)
Created attachment 194133 [details] [review] extensionSystem: Implement new live enable/disable system The rough sketches of the system are outlined here: http://mail.gnome.org/archives/gnome-shell-list/2011-June/msg00283.html Additionally, enable/disable extensions when the 'enabled-extensions' setting changes. Two new DBus methods, "EnableExtension" and "DisableExtension" allow users to manipulate this setting quite easily.
Created attachment 194134 [details] [review] extensionSystem: Add install-from-HTTP capability This adds a new DBus method: InstallExtensionRemote(url : s) Pass it the URL of a manifest file: the same as a metadata.json, but with a special key, '__installer', and use it to install the extension. In the future, the manifest file may be used to automatically detect and install updates.
Created attachment 194181 [details] [review] extensionSystem: Implement new live enable/disable system The rough sketches of the system are outlined here: http://mail.gnome.org/archives/gnome-shell-list/2011-June/msg00283.html Additionally, enable/disable extensions when the 'enabled-extensions' setting changes. Two new DBus methods, "EnableExtension" and "DisableExtension" allow users to manipulate this setting quite easily. Reuse enableExtension for the initial loading, so we get the same error handling.
Comment on attachment 194134 [details] [review] extensionSystem: Add install-from-HTTP capability >+// The unfortunate state of gjs, gobject-introspection and libsoup >+// means that this doesn't work right now. >+// _httpSession.add_feature(new Soup.ProxyResolverDefault()); Which is now fixed of course. If you want to avoid a warning with older libsoups (since we aren't jhbuilding it), you could do: // Soup.Session.add_feature was broken until libsoup 2.35.4. FIXME after 3.2 if (Soup.Session.prototype.add_feature instanceof Function) _httpSession.add_feature(new Soup.ProxyResolverDefault()); >+function installExtensionFromManifestURL(url) { >+ _httpSession.queue_message( >+ Soup.Message.new('GET', url), >+ function(session, message) { >+ let manifest = JSON.parse(message.response_body.data); if (message.status_code == Soup.KnownStatusCode.OK) { ... } else { ... } (meh, I've really got to rename that type...) >+ // FIXME: use a GFile mkstemp-type method once one exists >+ let tmpzip = '/tmp/%s.shell-extension.zip'.format(randomLetters(6)); >+ let f = Gio.file_new_for_path(tmpzip); >+ let stream = f.replace(null, false, try { let [fd, tmpzip] = GLib.file_open_tmp('XXXXXX.shell-extension.zip'); } catch (e) { ... }; let stream = new Gio.UnixOutputStream({ fd: fd }); (requires an annotation fix to g_file_open_tmp) alternately, use g_get_tmp_dir() and g_mkstemp(). You don't have access to errno, but I guess you don't really care that much. >+ GLib.child_watch_add(GLib.PRIORITY_DEFAULT, pid, function(pid, status) { >+ GLib.spawn_close_pid(pid); >+ loadExtension(dir, enabled, ExtensionType.PER_USER); You ought to check status here. Although you can't since you don't have WIFEXITED(). So I guess, you ought to check that at least one file got extracted successfully. >+ _httpSession.queue_message(Soup.Message.new('GET', url), >+ Lang.bind(null, gotExtensionZipFile, uuid)); Ew. What about function (session, msg) { gotExtensionZipFile(message, uuid); }
Created attachment 194398 [details] [review] extensionSystem: Add install-from-HTTP capability This adds a new DBus method: InstallExtensionRemote(url : s) Pass it the URL of a manifest file: the same as a metadata.json, but with a special key, '__installer', and use it to install the extension. In the future, the manifest file may be used to automatically detect and install updates. (In reply to comment #52) > (From update of attachment 194134 [details] [review]) > >+// The unfortunate state of gjs, gobject-introspection and libsoup > >+// means that this doesn't work right now. > >+// _httpSession.add_feature(new Soup.ProxyResolverDefault()); > > Which is now fixed of course. If you want to avoid a warning with older > libsoups (since we aren't jhbuilding it), you could do: > > // Soup.Session.add_feature was broken until libsoup 2.35.4. FIXME after > 3.2 > if (Soup.Session.prototype.add_feature instanceof Function) > _httpSession.add_feature(new Soup.ProxyResolverDefault()); I added a link to the bug, and this gave me an idea. Because gjs uses the ResolveOp stuff to get properties, we can just use the prototype to access the function directly. So... if (Soup.Session.prototype.add_feature != null) Soup.Session.prototype.add_feature.call(_httpSession, ...); > >+function installExtensionFromManifestURL(url) { > >+ _httpSession.queue_message( > >+ Soup.Message.new('GET', url), > >+ function(session, message) { > >+ let manifest = JSON.parse(message.response_body.data); > > if (message.status_code == Soup.KnownStatusCode.OK) { ... } else { ... } > > (meh, I've really got to rename that type...) Sure. > >+ // FIXME: use a GFile mkstemp-type method once one exists > >+ let tmpzip = '/tmp/%s.shell-extension.zip'.format(randomLetters(6)); > >+ let f = Gio.file_new_for_path(tmpzip); > >+ let stream = f.replace(null, false, > > try { > let [fd, tmpzip] = GLib.file_open_tmp('XXXXXX.shell-extension.zip'); > } catch (e) { ... }; > let stream = new Gio.UnixOutputStream({ fd: fd }); > > (requires an annotation fix to g_file_open_tmp) Done. I genuinely didn't know about UnixOutputStream. (annotations should be upstream in glib/gobject-introspection soon, right?) I filed #657085 for the GFile mkstemp-style wrapper request. > alternately, use g_get_tmp_dir() and g_mkstemp(). You don't have access to > errno, but I guess you don't really care that much. > >+ GLib.child_watch_add(GLib.PRIORITY_DEFAULT, pid, function(pid, status) { > >+ GLib.spawn_close_pid(pid); > >+ loadExtension(dir, enabled, ExtensionType.PER_USER); > > You ought to check status here. Although you can't since you don't have > WIFEXITED(). So I guess, you ought to check that at least one file got > extracted successfully. loadExtension already checks for metadata.json and extension.js -- is that good enough? > >+ _httpSession.queue_message(Soup.Message.new('GET', url), > >+ Lang.bind(null, gotExtensionZipFile, uuid)); > > Ew. What about > > function (session, msg) { gotExtensionZipFile(message, uuid); } Sure.
Comment on attachment 194398 [details] [review] extensionSystem: Add install-from-HTTP capability >+function installExtensionFromManifestURL(url) { >+ _httpSession.queue_message( >+ Soup.Message.new('GET', url), >+ function(session, message) { >+ log(url); cruft >+ if (message.status_code != Soup.KnownStatusCode.OK) >+ throw new Error('Could not grab manifest'); No, that's not what I meant. Throwing an error that nothing is going to catch is no better than what was there before. I meant, if something fails, we should provide an end-user-friendly error message in the UI explaining that it failed.
Created attachment 194496 [details] [review] extensionSystem: Add install-from-HTTP capability This adds a new DBus method: InstallExtensionRemote(uuid : s, url : s) Pass it the UUID of an extension and the URL of a manifest file: the same as a metadata.json, but with a special key, '__installer', which is an HTTP location that points to an zip file containing the extension. The Shell will download and use it to install the extension. In the future, the manifest file may be used to automatically detect and install updates. OK, so if you want to show a visible error, I'll just signal that the extension error'd out. On the website JS, I'll just parse the error to create a better message for the user. Good with you?
Comment on attachment 194496 [details] [review] extensionSystem: Add install-from-HTTP capability > OK, so if you want to show a visible error, I'll just signal that the extension > error'd out. On the website JS, I'll just parse the error to create a better > message for the user. Good with you? Hm... I haven't read any of the rest of the code. So, logExtensionError() will cause the d-bus peer to receive an error, not pop up a user-visible error? In that case, ok, I guess; the error messages need to be localized before being shown to the user, but I guess that's part of what you meant by "create a better message". So, if that's the case, then ok to commit I guess
(In reply to comment #56) > (From update of attachment 194496 [details] [review]) > > OK, so if you want to show a visible error, I'll just signal that the extension > > error'd out. On the website JS, I'll just parse the error to create a better > > message for the user. Good with you? > > Hm... I haven't read any of the rest of the code. So, logExtensionError() will > cause the d-bus peer to receive an error, not pop up a user-visible error? In > that case, ok, I guess; the error messages need to be localized before being > shown to the user, but I guess that's part of what you meant by "create a > better message". > > So, if that's the case, then ok to commit I guess logExtensionError causes the DBus ExtensionStatusChanged signal to be sent out with the parameters: the UUID, 3 (ERROR), and the error message. Anyone can pick up this message and display it. I was intending on scfraping these messages from JS, and display a better, localized version on the website, so that other DBus peers can the information raw if necessary.
Created attachment 194637 [details] [review] extensionSystem: Implement new live enable/disable system The rough sketches of the system are outlined here: http://mail.gnome.org/archives/gnome-shell-list/2011-June/msg00283.html Additionally, enable/disable extensions when the 'enabled-extensions' setting changes. Two new DBus methods, "EnableExtension" and "DisableExtension" allow users to manipulate this setting quite easily. Rebased on top of Adel's Screenshot changes in master.
Attachment 193010 [details] pushed as a56cd3c - extensionSystem: Add 'extension-status-changed' signal Attachment 193011 [details] pushed as 465d03a - extensionSystem: Add a DOWNLOADING state Attachment 193012 [details] pushed as 6241a82 - extensionSystem: Start using OUT_OF_DATE Attachment 194126 [details] pushed as 6d3434f - extensionSystem: Remove 'disabled-extensions' blacklist Attachment 194496 [details] pushed as d8a98e5 - extensionSystem: Add install-from-HTTP capability Attachment 194637 [details] pushed as 2d813cb - extensionSystem: Implement new live enable/disable system