GNOME Bugzilla – Bug 677586
Other prerequisite patches for the extension updating system
Last modified: 2012-06-14 02:31:32 UTC
Oh boy. Large patch stack here. I'm not sure some of these should go in. The SweetTooth CHANGELOG part hasn't landed yet either, and hopefully I can get this integrated with other System Updates for 3.6 as well. I think the installation DBus process has to be redesigned, maybe using async DBus, so we can report errors/cancellations properly instead of faking async APIs with a special signal.
Created attachment 215778 [details] [review] extensionSystem: Split off the extension downloader into a separate file This keeps all UI out of the extension system, leaving it strictly for loading and unloading extensions.
Created attachment 215779 [details] [review] extensionUtils: Don't write to the filesystem on start Create the potentially empty directory when we need to, not when we don't.
Created attachment 215780 [details] [review] extensionUtils: Remove version check for js-version This is seldomly used, and isn't checked in SweetTooth. Just remove this inconsistency here rather than adding infrastructure to manage and check it elsewhere.
Created attachment 215781 [details] [review] extensionUtils: Remove userExtensionsDir Make this less stateful
Created attachment 215782 [details] [review] extensionUtils: Use signals rather than callbacks for finding extensions This allows us to move to a file-monitor based approach in the future.
Created attachment 215783 [details] [review] extensionUtils: Create and load the extension object when scanning This reduces some duplicate code when loading extensions.
Created attachment 215784 [details] [review] extensionSystem: Load/unload stylesheets on enable/disable I'm not entirely sure why this wasn't caught earlier.
Created attachment 215785 [details] [review] extensionSystem: Make the init function optional A large amount of extensions have something like this in them: function init() {} Since we have encouraged extension authors to try and not make any changes in init, it feels weird and strange to have to create an initialization function that does nothing. From now on, don't require it.
Created attachment 215786 [details] [review] extensionSystem: Remove the two-step initialize Initially, extensions were loaded after they shell had fully created the session and all objects, but this didn't allow extensions easy ways to monkey patch prototypes, as most functions had already been bound. Remove the historical vestigal function, and just merge the two together.
Created attachment 215787 [details] [review] extensionSystem: Add a method to unload an extension
Created attachment 215788 [details] [review] runDialog: Add a 're' command to reload extensions A common request from extension developers has been to reload their extensions without restarting the Shell.
Created attachment 215789 [details] [review] extensionDownloader: Add dumb support for updating a single extension This is implemented as an uninstall + install. We'll need to rework the error-handling code so that we can't accidentally wipe a user's extension if a bogus update comes out or the HTTP download fails for some reason.
Created attachment 215790 [details] [review] extensionUtils: Create and load the extension object when scanning This reduces some duplicate code when loading extensions. Whoops, there are some errors in this one.
Created attachment 215791 [details] [review] extensionSystem: Add a method to unload an extension This one too.
Review of attachment 215778 [details] [review]: ::: js/ui/extensionDownloader.js @@ +21,3 @@ +const REPOSITORY_URL_INFO = REPOSITORY_URL_BASE + '/extension-info/'; + +const _httpSession = new Soup.SessionAsync({ ssl_use_system_ca_file: true }); I'm really not a fan of creating the soup session while we're parsing the JS files. That's just asking for pain. Make it a dynamically-initialized singleton with the: if (_httpSession == null) _httpSession = ... ? @@ +74,3 @@ + let dir = ExtensionUtils.userExtensionsDir.get_child(uuid); + let contents = message.response_body.flatten().as_bytes(); + stream.output_stream.write_bytes(contents, null); Can't you splice_async() here? (What you really want is GSubprocess...) ::: js/ui/shellDBus.js @@ -280,3 @@ }, - ApiVersion: ExtensionSystem.API_VERSION, This seems unrelated?
Review of attachment 215779 [details] [review]: Sure, though this slightly penalizes people who want to install extensions "by hand".
Review of attachment 215780 [details] [review]: Sure. ::: js/Makefile.am @@ -10,2 @@ -e "s|[@]HAVE_BLUETOOTH@|$(HAVE_BLUETOOTH)|g" \ - -e "s|[@]SHELL_SYSTEM_CA_FILE@|$(SHELL_SYSTEM_CA_FILE)|g" \ Unrelated, but...I don't really care if you keep it in this patch.
Review of attachment 215781 [details] [review]: You could also keep ExtensionUtils.getUserExtensionsDir() to avoid duplicating the path (but make a new Gio.File object each time). ::: js/misc/extensionUtils.js @@ +173,2 @@ function scanExtensions(callback) { + let userExtensionsDir = Gio.File.new_for_path(GLib.build_filenamev([global.userdatadir, 'extensions'])); I should really get g_file_new_build_path() into glib...
Review of attachment 215782 [details] [review]: ::: js/misc/extensionUtils.js @@ +25,3 @@ + +const connect = Lang.bind(_signals, _signals.connect); +const disconnect = Lang.bind(_signals, _signals.disconnect); Er...this seems really weird. Probably less confusing to add the extra 10 lines of code to make it a "real" object.
Review of attachment 215784 [details] [review]: OK
Review of attachment 215785 [details] [review]: Makes sense.
Review of attachment 215786 [details] [review]: Makes sense; good commit message.
Review of attachment 215788 [details] [review]: I worry we're proliferiating magical run dialog entries a bit too much...but oh well.
Review of attachment 215789 [details] [review]: I guess...this is starting to feel like gnome-tweak-tool or something territory.
Review of attachment 215790 [details] [review]: Does seem nicer.
Review of attachment 215791 [details] [review]: ::: js/ui/extensionDownloader.js @@ +46,2 @@ return false; Aren't we losing the ability to disable system extensions? (Is that an intentional feature? Firefox has it, mainly because Windows installers often drop browser toolbars too...)
Review of attachment 215782 [details] [review]: ::: js/misc/extensionUtils.js @@ +25,3 @@ + +const connect = Lang.bind(_signals, _signals.connect); +const disconnect = Lang.bind(_signals, _signals.disconnect); We do the same hack in ExtensionSystem, and I knew somebody was going to call me out on it. I briefly attempted to make it a real object, but decided it wasn't really worth it. We already rely on a singleton API for ExtensionUtils, with ExtensionUtils.getCurrentExtension(), so that would mean that we just do: const extensionUtils = new ExtensionUtils(); // Don't break API const getCurrentExtension = extensionUtils.getCurrentExtension; at the bottom of the module, and I'm not sure if that's much better.
Review of attachment 215789 [details] [review]: Right, this isn't really intended to land right now. As said, this is just a dummy updater. We want native extension update integration for 3.6
Review of attachment 215778 [details] [review]: Keep in mind that most of this code has just been peeled from extensionSystem.js. ::: js/ui/extensionDownloader.js @@ +21,3 @@ +const REPOSITORY_URL_INFO = REPOSITORY_URL_BASE + '/extension-info/'; + +const _httpSession = new Soup.SessionAsync({ ssl_use_system_ca_file: true }); This is how it has always been done. What in here is painful? @@ +74,3 @@ + let dir = ExtensionUtils.userExtensionsDir.get_child(uuid); + let contents = message.response_body.flatten().as_bytes(); + stream.output_stream.write_bytes(contents, null); I don't know how I could splice. Soup doesn't allow me to get a GOutputStream. This is the entire reason I landed the write_bytes patches, by the way. ::: js/ui/shellDBus.js @@ -280,3 @@ }, - ApiVersion: ExtensionSystem.API_VERSION, It's just cruft that we never removed. Even after the DBus API version changed several times, we never bumped the number, because we never read it.
Review of attachment 215791 [details] [review]: ::: js/ui/extensionDownloader.js @@ +45,3 @@ if (!extension) return false; uninstall != disable We still have the ability to disable system extensions, just not delete the directory we live in, because, well, we can't delete that directory. If you want to put the unload above the type check so that uninstallation tries to disable before it magically fails, sure, but I'm not sure that's a good idea.
(In reply to comment #29) > Review of attachment 215778 [details] [review]: > > Keep in mind that most of this code has just been peeled from > extensionSystem.js. > > ::: js/ui/extensionDownloader.js > @@ +21,3 @@ > +const REPOSITORY_URL_INFO = REPOSITORY_URL_BASE + '/extension-info/'; > + > +const _httpSession = new Soup.SessionAsync({ ssl_use_system_ca_file: true }); > > This is how it has always been done. What in here is painful? Because we're going to be initializing proxy loaders and all sorts of other stuff while we're in the middle of parsing the JS code, which happens very early. For most GObject creation we've factored things out so that it happens inside an initialization method, which means it's more predictable and we can control ordering better. > @@ +74,3 @@ > + let dir = ExtensionUtils.userExtensionsDir.get_child(uuid); > + let contents = message.response_body.flatten().as_bytes(); > + stream.output_stream.write_bytes(contents, null); > > I don't know how I could splice. Soup doesn't allow me to get a GOutputStream. > This is the entire reason I landed the write_bytes patches, by the way. You should be able to do it with the SoupRequester. > ::: js/ui/shellDBus.js > @@ -280,3 @@ > }, > > - ApiVersion: ExtensionSystem.API_VERSION, > > It's just cruft that we never removed. Even after the DBus API version changed > several times, we never bumped the number, because we never read it. Ok. I understand there are a lot of patches, this one seems like it could go in a "remove obsolete/unused extension system stuff" patch though.
(In reply to comment #31) > Because we're going to be initializing proxy loaders and all sorts of other > stuff while we're in the middle of parsing the JS code, which happens very > early. For most GObject creation we've factored things out so that it happens > inside an initialization method, which means it's more predictable and we can > control ordering better. OK then. I'll fix it tomorrow. > You should be able to do it with the SoupRequester. Not a stable API, as Dan Winship has said many times over.
(In reply to comment #16) > Review of attachment 215779 [details] [review]: > > Sure, though this slightly penalizes people who want to install extensions "by > hand". mkdir is hard to do by hand?
Created attachment 215895 [details] [review] extensionSystem: Initialize the HTTP session after parse time We really shouldn't be creating GObjects while we're still parsing other files for performance and clarity reasons.
Created attachment 215896 [details] [review] extensionSystem: Split off the extension downloader into a separate file This keeps all UI out of the extension system, leaving it strictly for loading and unloading extensions.
Created attachment 215897 [details] [review] extensionUtils: Don't write to the filesystem on start Create the potentially empty directory when we need to, not when we don't.
Created attachment 215898 [details] [review] extensionUtils: Remove version check for js-version This is seldomly used, and isn't checked in SweetTooth. Just remove this inconsistency here rather than adding infrastructure to manage and check it elsewhere.
Created attachment 215899 [details] [review] extensionUtils: Remove userExtensionsDir Make this less stateful
Created attachment 215900 [details] [review] extensionUtils: Use signals rather than callbacks for finding extensions This allows us to move to a file-monitor based approach in the future. Since we need signals, we convert the current set of functions to an object we attach signals too, leading to the new ExtensionFinder object.
Created attachment 215901 [details] [review] extensionUtils: Create and load the extension object when scanning This reduces some duplicate code when loading extensions.
Created attachment 215902 [details] [review] extensionSystem: Load/unload stylesheets on enable/disable I'm not entirely sure why this wasn't caught earlier.
Created attachment 215903 [details] [review] extensionSystem: Make the init function optional A large amount of extensions have something like this in them: function init() {} Since we have encouraged extension authors to try and not make any changes in init, it feels weird and strange to have to create an initialization function that does nothing. From now on, don't require it.
Created attachment 215904 [details] [review] extensionSystem: Remove the two-step initialize Initially, extensions were loaded after they shell had fully created the session and all objects, but this didn't allow extensions easy ways to monkey patch prototypes, as most functions had already been bound. Remove the historical vestigal function, and just merge the two together.
Created attachment 215905 [details] [review] extensionSystem: Add a method to unload an extension
Created attachment 215906 [details] [review] shellDBus: Add a method to reload extensions A common request from extension developers has been to reload their extensions without restarting the Shell.
Created attachment 215907 [details] [review] extensionDownloader: Add dumb support for updating a single extension This is implemented as an uninstall + install. We'll need to rework the error-handling code so that we can't accidentally wipe a user's extension if a bogus update comes out or the HTTP download fails for some reason. THIS IS NOT INTENDED TO LAND
(In reply to comment #24) > Review of attachment 215789 [details] [review]: > > I guess...this is starting to feel like gnome-tweak-tool or something > territory. Yeah... I am happy to add extension downloading to gnome-tweak-tool. It is kinda already there (https://github.com/nzjrs/gnome-tweak-tool/tree/ego). I wanted to land support for complete extension management in tweak-tool this cycle, so please let me know if you want to collaborate or if you are going to change the http API.
(In reply to comment #47) > I wanted to land support for complete extension management in tweak-tool this > cycle, so please let me know if you want to collaborate or if you are going to > change the http API. I'm happy to collaborate. I'm not going to change the HTTP API that much, hopefully, and if you want me to add anything, just ping me. The last patch is not intended to land, but just a demo of how to use the client-side APIs until we get system update integration done.
(In reply to comment #48) > (In reply to comment #47) > > I wanted to land support for complete extension management in tweak-tool this > > cycle, so please let me know if you want to collaborate or if you are going to > > change the http API. > > I'm happy to collaborate. I'm not going to change the HTTP API that much, > hopefully, and if you want me to add anything, just ping me. How about a non-paginated version of extension-query/?shell_version=x.y.z Otherwise I need to call this multiple times with page=n to get all the compatible extensions. > > The last patch is not intended to land, but just a demo of how to use the > client-side APIs until we get system update integration done. OK, that makes sense.
Review of attachment 215895 [details] [review]: Thanks! I'm happier with that.
Comment on attachment 215895 [details] [review] extensionSystem: Initialize the HTTP session after parse time Attachment 215895 [details] pushed as c7b4022 - extensionSystem: Initialize the HTTP session after parse time
Review of attachment 215788 [details] [review]: (This has been replaced by a DBus method)
These new patches all look good to me.
Attachment 215896 [details] pushed as 3ff51da - extensionSystem: Split off the extension downloader into a separate file Attachment 215897 [details] pushed as feef35a - extensionUtils: Don't write to the filesystem on start Attachment 215898 [details] pushed as cdbe0bb - extensionUtils: Remove version check for js-version Attachment 215899 [details] pushed as 5265884 - extensionUtils: Remove userExtensionsDir Attachment 215900 [details] pushed as 498b023 - extensionUtils: Use signals rather than callbacks for finding extensions Attachment 215901 [details] pushed as 86de6f5 - extensionUtils: Create and load the extension object when scanning Attachment 215902 [details] pushed as 11278a0 - extensionSystem: Load/unload stylesheets on enable/disable Attachment 215903 [details] pushed as 0805d7a - extensionSystem: Make the init function optional Attachment 215904 [details] pushed as 3290bfa - extensionSystem: Remove the two-step initialize Attachment 215905 [details] pushed as 75570b7 - extensionSystem: Add a method to unload an extension Attachment 215906 [details] pushed as 19ef6b0 - shellDBus: Add a method to reload extensions OK, then.
(In reply to comment #49) > How about a non-paginated version of extension-query/?shell_version=x.y.z > > Otherwise I need to call this multiple times with page=n to get all the > compatible extensions. https://extensions.gnome.org/extension-query/?shell_version=3.4.1&n_per_page=-1