GNOME Bugzilla – Bug 661815
More and more improvements to the extension system
Last modified: 2011-10-20 21:39:11 UTC
A few things here.
Created attachment 199041 [details] [review] extensionSystem: Load user extensions after system ones
Created attachment 199042 [details] [review] extensionSystem: Only load importers for enabled extensions
Created attachment 199043 [details] [review] extensionSystem: Rebase the extension order list to help prevent conflicts When two extensions monkey-patch the same area, enable() and disable() may behave badly and completely wreck things. To solve this, when disabling an extension, "rebase" the extension list so that monkey patches should be added and removed in order.
(In reply to comment #0) > A few things here. I did not mean to ^D so soon. We have two main improvements here: * The second patch makes it so that we only load importers (and execute code) when an extension is enabled. This is so that nobody can try to sneak stuff past us. * The third is more iff-y, and tries to "rebase" extensions so that conflicts won't happen as often. If two extensions modify the same signal handler in the userMenu for example, they could step on each other's toes. Concrete example: * Extensions A and B monkey patch a function by saving the initial value, and restoring it on disable. * User enables A, then B. * B has saved A's handler. * User disables A, then B. * B restores the initial value: A's handler. * A is still running unbeknownst to the user. This solves it so that when disabling A, it will first disable B, then disable A, then enable B again. B should "re-grab" the initial value.
Review of attachment 199041 [details] [review]: Makes sense.
Created attachment 199138 [details] [review] gnome-shell-extension-tool: Add facilities to enable/disable extensions Requested on IRC by Moc.
Comment on attachment 199041 [details] [review] extensionSystem: Load user extensions after system ones Attachment 199041 [details] pushed as 82ed80c - extensionSystem: Load user extensions after system ones
Review of attachment 199043 [details] [review]: Looks good, has some style issues and one (probably c&p caused) bug. ::: js/ui/extensionSystem.js @@ +227,3 @@ + + let orderIdx = extensionOrder.indexOf(uuid); + let order = extensionOrder.slice(orderIdx+1); Missing whitespace around '+'. @@ +230,3 @@ + let orderReversed = order.slice().reverse(); + + for (let i = 0; i < orderReversed.length; i ++) { Wrong whitespace "i ++". @@ +246,3 @@ } + for (let i = 0; i < order.length; i ++) { Wrong whitespace "i ++". @@ +247,3 @@ + for (let i = 0; i < order.length; i ++) { + let uuid = orderReversed[i]; You want order here not orderRevered.
Review of attachment 199138 [details] [review]: Looks good.
Review of attachment 199042 [details] [review]: LGTM
Created attachment 199586 [details] [review] extensionSystem: Rebase the extension order list to help prevent conflicts When two extensions monkey-patch the same area, enable() and disable() may behave badly and completely wreck things. To solve this, when disabling an extension, "rebase" the extension list so that monkey patches should be added and removed in order. Fixed basic whitespace comments. Anything else?
Review of attachment 199586 [details] [review]: Fine then.
Attachment 199042 [details] pushed as 4ae2a0b - extensionSystem: Only load importers for enabled extensions Attachment 199138 [details] pushed as 44e2f7f - gnome-shell-extension-tool: Add facilities to enable/disable extensions Attachment 199586 [details] pushed as fd1b3b4 - extensionSystem: Rebase the extension order list to help prevent conflicts