GNOME Bugzilla – Bug 676837
Cleanups and improvements to the extension system
Last modified: 2012-05-29 18:36:36 UTC
This is a quick clean up base that I'd like to have before I work on extension updating for 3.6.
Created attachment 214984 [details] [review] browser-plugin: Set the 'enabled-extensions' GSettings key directly This way, we won't have to shuttle over DBus if we want to set an extension's status outside of a GNOME Shell session.
Created attachment 214985 [details] [review] extensionSystem: Remove now-unused version_tag, clean up other code as well
Created attachment 214986 [details] [review] extensionSystem: Remove custom CA file handling libsoup (well, glib-networking) now handles this automatically
Created attachment 214987 [details] [review] extensionSystem: Clean up
Created attachment 214988 [details] [review] extensionSystem: Use Gio.File.new_tmp
Created attachment 214989 [details] [review] extensionSystem: Replace Shell.write_soup_message_to_stream with GBytes usage
Created attachment 214990 [details] [review] extensionSystem: Show an icon in the install dialog
Review of attachment 214984 [details] [review]: I think the removal of DBus interfaces deserves a commit on its own. It is questionable, as there could be other consumers of it. ::: browser-plugin/browser-plugin.c @@ +517,3 @@ + new_uuids = g_new (const gchar *, length + 2); /* New key, NULL */ + for (i = 0; i < length; i ++) + new_uuids[i] = g_strdup (uuids[i]); You can avoid the copy if you g_free(new_uuids) instead of g_strfreev.
Review of attachment 214985 [details] [review]: Looks good.
Review of attachment 214986 [details] [review]: LG
Review of attachment 214987 [details] [review]: Yep
Review of attachment 214988 [details] [review]: LG
Review of attachment 214989 [details] [review]: Are we ok with the extra copy caused by conversion to GBytes? (If so, acn)
Review of attachment 214990 [details] [review]: LG
Review of attachment 214989 [details] [review]: There's no extra copy. soup_buffer_get_as_bytes will take a ref on the buffer and hand the same buffer data contents that the SoupBuffer has internally: http://git.gnome.org/browse/libsoup/tree/libsoup/soup-message-body.c#n311
(In reply to comment #15) > Review of attachment 214989 [details] [review]: > > There's no extra copy. soup_buffer_get_as_bytes will take a ref on the buffer > and hand the same buffer data contents that the SoupBuffer has internally: > > http://git.gnome.org/browse/libsoup/tree/libsoup/soup-message-body.c#n311 Yes, my bad, I misread it.
Created attachment 215007 [details] [review] browser-plugin: Set the 'enabled-extensions' GSettings key directly This way, we won't have to shuttle over DBus if we want to set an extension's status outside of a GNOME Shell session.
Created attachment 215008 [details] [review] shellDBus: Remove now unused DisableExtension/EnableExtension These methods were initially introduced when I was planning on having an explicit DisableExtension/EnableExtension, instead of hooking up a gsettings notify. This behavior was changed at the last minute, but the methods were kept to avoid having to change the browser-plugin. Consumers of this API should just set the GSettings key directly instead now.
Review of attachment 215007 [details] [review]: This goes in for sure.
Review of attachment 215008 [details] [review]: This one I'm not sure... but after all it's not even 3.5.2, and that API has never been part of any stability promise. Go on and kill it.
Attachment 214985 [details] pushed as 031206c - extensionSystem: Remove now-unused version_tag, clean up other code as well Attachment 214986 [details] pushed as d57658d - extensionSystem: Remove custom CA file handling Attachment 214987 [details] pushed as 33ad9d1 - extensionSystem: Clean up Attachment 214988 [details] pushed as 0c736c4 - extensionSystem: Use Gio.File.new_tmp Attachment 214989 [details] pushed as 9639616 - extensionSystem: Replace Shell.write_soup_message_to_stream with GBytes usage Attachment 214990 [details] pushed as 3f94230 - extensionSystem: Show an icon in the install dialog Attachment 215007 [details] pushed as 9d3750b - browser-plugin: Set the 'enabled-extensions' GSettings key directly Attachment 215008 [details] pushed as 12c7cc2 - shellDBus: Remove now unused DisableExtension/EnableExtension
Created attachment 215157 [details] [review] browser-plugin: Fix enable/disable This code was broken at the last minute, and an accidentally stale plugin copy kept it untested.
Review of attachment 215157 [details] [review]: Ooops... Good to go, integrate the comment if you want ::: browser-plugin/browser-plugin.c @@ +516,3 @@ { new_uuids = g_new (const gchar *, length + 2); /* New key, NULL */ + memcpy (new_uuids, uuids, length * sizeof (*new_uuids)); sizeof(const char*) is clearer to me, but ok either way
Attachment 215157 [details] pushed as ad6d986 - browser-plugin: Fix enable/disable