We now have the same boilerplate code in three bootstrapped extensions: Mozmill, JS Bridge and mail/test/mozmill/folder-tree-modes/test-extension/bootstrap.js. The same boilerplate will be necessary in *all* bootstrapped extension that want do attach something to a window. I propose to use extensionSupport.jsm to implement the following function: ExtensionSupport.registerExtension(aParam) and .unregisterExtension(aParam). Extensions can call this in their startup function. aParam is an object with the following properties: name: Typically the add-on name or some unique key. filter: A list of strings containing window hrefs that the add-on is interested in, typically "chrome://messenger/content/messenger.xul" or "chrome://messenger/content/messengercompose/messengercompose.xul". callback: A callback that will be called for every existing or new window. So the code we have for example in JS Brigde would be reduced to: ExtensionSupport.registerExtension({ name: "Thunderbird-internal-JS-Bridge", filter: [ "chrome://messenger/content/messenger.xul" ], callback: setupServer }); The callback will be passed the DOM window. Yes, unregistering will then just stop the notifications. Aceman, can I get you interested?

I'd suggest that in addition to this, I provide an "overlay.jsm" module, which can be used by addons from within setupServer() to re-use their existing XUL and load the elements into the UI DOM tree.

Yes, I'd like this. But this just helps with one removed feature, while many others still need to be solved in addons. So what was nicely shared in the C++ code will now be re-implemented in JS in addons in differing ways. I don't see the speed benefit of that. Enough ranting, I see if I can do this.

This seems to work for me. Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b264b95ed35a8a90204c0eb8cff5d471a04886af I also added possibility to run callback on window unload. Unregistering the listener is also there. The current 3 addons run it only on shutdown() but calling it anytime should work too. Open questions: 1. is there a better way to get the addonID that is in the install.rdf file? I don't want to hardcode it in the bootstrap.js file again. 2. the current architecture in the patch is to create a new listener for every addon. Would it be nicer/simpler to have only one listener that would at each new window just go through the array of what addons have registered and run the callbacks ? 3. each addon can only register one listener. Is it enough? E.g. in jsbridge where we do one thing for each window, but start the server only once, there could be 2 listeners of which one would be unregistered after the first run. Please comment on variable/function names too. I will then also add documentation to the functions in the final patch.

(In reply to Patrick Brunschwig from comment #1) > I'd suggest that in addition to this, I provide an "overlay.jsm" module, > which can be used by addons from within setupServer() to re-use their > existing XUL and load the elements into the UI DOM tree. Isn't that bug 1448808? What is setupServer() in this context? That's the function we ultimately want to run in our JS Bridge add-on. It just served as an example here.

Comment on attachment 8964128 [details] [diff] [review] 1450288.patch WIP Review of attachment 8964128 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to :aceman from comment #3) > This seems to work for me. Try run: > https://treeherder.mozilla.org/#/jobs?repo=try-comm- > central&revision=b264b95ed35a8a90204c0eb8cff5d471a04886af Lot's of linting issues :-( > Open questions: > 1. is there a better way to get the addonID that is in the install.rdf file? > I don't want to hardcode it in the bootstrap.js file again. RDF will be removed and replaced with JSON in the near future. I think hard-coding the string is fine for now. > 2. the current architecture in the patch is to create a new listener for > every addon. Would it be nicer/simpler to have only one listener that would > at each new window just go through the array of what addons have registered > and run the callbacks ? Yes. That module should administer a list of windows, and when an add-on registers, give it those windows as well as notify of new windows. That's of course a little trickier since it would have to watch onCloseWindow() as well. > 3. each addon can only register one listener. Is it enough? E.g. in jsbridge > where we do one thing for each window, but start the server only once, there > could be 2 listeners of which one would be unregistered after the first run. Yes, JS Bridge attaches the "overlay" to every windows, but only starts the server once. I still think one listener is enough, besides, you can start two if you pass in two different add-on IDs ;-) > I will then also add documentation to the functions in the final patch. Good ;-) Clearing feedback. It's going the right way, thanks :-) ::: common/src/extensionSupport.jsm @@ +18,5 @@ > + * Reads preferences from addon provided locations (defaults/preferences/*.js) > + * and stores them in the default preferences branch. > + * This is only run once by the application at startup, not by each addon. > + */ > + extensionDefaults: function() { We will need to remove that, but that's for bug 1450479. We should stick with the spelling "add-on", however, I don't think it makes sense to add more comments to something that will be removed. Or we could always use "extension". @@ +143,5 @@ > + setupWindow(domWindow, aAddonFilter); > + } > + > + function checkWindowURL(aURL, aFilterURLs) { > + return aFilterURLs.some(url => (url == "*" || url == aURL)); I thought the add-on would not pass any filter if it wants all URLs. I forgot to mentions that in comment #0. Anyway, this will not work if the add-on doesn't pass that property. If we start using wildcards, then people will think that wildcards will really work. @@ +152,5 @@ > + // A window is available, but it's not yet fully loaded. > + // Add an event listener to fire when the window is completely loaded. > + if (aAddonFilter.onOpenWindow) { > + aWindow.addEventListener("load", function() { > + if (checkWindowURL(aWindow.document.location.href, aAddonFilter.chromeURLs)) aAddonFilter.chromeURLs could be undefined.

Comment on attachment 8964128 [details] [diff] [review] 1450288.patch WIP Review of attachment 8964128 [details] [diff] [review]: ----------------------------------------------------------------- > Open questions: > 1. is there a better way to get the addonID that is in the install.rdf file? > I don't want to hardcode it in the bootstrap.js file again. You don't need to hardcode the addonID. The addonID is part of the "data" object passed in startup(data, reason); i.e.: addonId = data.id; See: https://developer.mozilla.org/en-US/docs/Archive/Add-ons/Bootstrapped_extensions#Bootstrap_data(In reply to :aceman from comment #3).

This fixes the comments so far. It adds even some tests, which did uncover problems in the first patch. Try run at https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f12c10f7b21a764a6939cab5f2a1013214d3b1cf .

Comment on attachment 8966059 [details] [diff] [review] 1450288.patch WIP2 Review of attachment 8966059 [details] [diff] [review]: ----------------------------------------------------------------- Looking good to me. I suggest to eslint the code -- I found at least one occurrence where you assign a value to an undeclared variable (addonID = data.id).

Try server does eslint too, but yet it may be the last version on try doesn't contain the latest code. But I do not see the undeclared addonID in the attached patch. Also I'd think undeclared variable is a JS error these days. But I can add 'use "strict"' to be sure ;)

Comment on attachment 8966059 [details] [diff] [review] 1450288.patch WIP2 Review of attachment 8966059 [details] [diff] [review]: ----------------------------------------------------------------- ::: common/src/extensionSupport.jsm @@ +175,5 @@ > + > + onCloseWindow(xulWindow) {} > + }, > + > + _setupWindow(aWindow, aID, aFirstListener) { Merge aID, aFirstListener into a "shouldAttachListener = true".

Comment on attachment 8966059 [details] [diff] [review] 1450288.patch WIP2 Review of attachment 8966059 [details] [diff] [review]: ----------------------------------------------------------------- ::: common/src/extensionSupport.jsm @@ +175,5 @@ > + > + onCloseWindow(xulWindow) {} > + }, > + > + _setupWindow(aWindow, aID, aFirstListener) { Sorry, you use aID below. In this case I suggest: _setupWindow(aWindow, aID, aShouldAttachListener = true)

Added more tests, documentation and storing the list of currently open windows.

Comment on attachment 8968004 [details] [diff] [review] 1450288.patch v3 Review of attachment 8968004 [details] [diff] [review]: ----------------------------------------------------------------- ::: common/src/extensionSupport.jsm @@ +155,5 @@ > + Cu.reportError("Window listener for extension + '" + aID + "' already registered"); > + return false; > + } > + > + if (!("onLoadWindow" in aExtensionHook) && !("onLoadWindow" in aExtensionHook)) { I'm sure you meant something else.

Comment on attachment 8968004 [details] [diff] [review] 1450288.patch v3 Review of attachment 8968004 [details] [diff] [review]: ----------------------------------------------------------------- Comments from just looking at the code. Let me run it now. ::: common/src/extensionSupport.jsm @@ +129,5 @@ > } > + > +var ExtensionSupport = { > + /** > + * Register listening for window opens that will run the specified callback function windows being opened. "the open" is maybe a noun in the sense of "out in the open". @@ +134,5 @@ > + * when a matching window is loaded. > + * > + * @param aID {String} Some identification of the caller, usually the extension ID. > + * @param aExtensionHook {Object} The object describing the hook the caller wants to register. > + * Members of the object can be (all optional): Hmm, you return 'false' if no callback is given. @@ +142,5 @@ > + * onLoadWindow {function} The callback function to run when window loads > + * the matching document. > + * onUnloadWindow {function} The callback function to run when window > + * unloads the matching document. > + * Both callbacks receive the matching window object as argument. Indentation. Return value? @@ +155,5 @@ > + Cu.reportError("Window listener for extension + '" + aID + "' already registered"); > + return false; > + } > + > + if (!("onLoadWindow" in aExtensionHook) && !("onLoadWindow" in aExtensionHook)) { Yes, repeated condition. @@ +174,5 @@ > + // Get the list of windows already open. > + let windows = Services.wm.getEnumerator(null); > + while (windows.hasMoreElements()) { > + let domWindow = windows.getNext().QueryInterface(Ci.nsIDOMWindow); > + openWindowList.add(domWindow); Here you add the window to the list. But it could be one that hasn't fully loaded yet. Why not only collect "good" windows? @@ +178,5 @@ > + openWindowList.add(domWindow); > + } > + } > + openWindowList.forEach(domWindow => > + this._setupWindow(domWindow, extensionHooks.size == 1, aID)); Please add comment here. @@ +202,5 @@ > + } > + > + extensionHooks.delete(aID); > + // Remove our global listener if there are no callers registered anymore. > + if (extensionHooks.size == 0) { Why? You can leave it for the next registration. Then you'd need to have a global to tell you whether you already attached the listener. You can't guarantee that add-ons will unregister anyway. @@ +216,5 @@ > + // nsIWindowMediatorListener functions > + onOpenWindow(xulWindow) { > + // A new window has opened. > + let domWindow = xulWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindow); > + openWindowList.add(domWindow); Same here, new window comes in, in the list it goes, although it might still be an incomplete one. @@ +233,5 @@ > + * > + * @param aWindow {nsIDOMWindow} The window to set up. > + * @param aShouldAttachListener {boolean} Set to true when the load/unload > + * listeners should be attached (the window > + * doesn't already have them. Closing parenthesis missing.

In the test the comment line as to how to run it is missing: mozmake SOLO_TEST=utils/test-extensionSupport.js mozmill-one

Comment on attachment 8968004 [details] [diff] [review] 1450288.patch v3 Review of attachment 8968004 [details] [diff] [review]: ----------------------------------------------------------------- ::: common/src/extensionSupport.jsm @@ +248,5 @@ > + aWindow.addEventListener("load", function listener1() { > + if (aWindow.document.location.href !== "about:blank") { > + aWindow.removeEventListener("load", listener1); > + ExtensionSupport._checkAndRunMatchingExtensions(aWindow, "load"); > + } else { dump("ACE: ignored:"+aWindow.document.location.href+"\n"); } Change this to "ignored(1)" and you will see that this never gets printed, the one below does get printed. Therefore the inner check !== "about:blank" is not necessary. When the load notification runs, the href *is* final. I have and add-on that listens for compose windows and it works without this inner test. @@ +261,5 @@ > + aWindow.addEventListener("unload", function listener2() { > + if (aWindow.document.location.href !== "about:blank") { > + aWindow.removeEventListener("unload", listener2); > + ExtensionSupport._checkAndRunMatchingExtensions(aWindow, "unload"); > + } else { dump("ACE: ignored:"+aWindow.document.location.href+"\n"); } You should change the debug to "ignored(2)". That's the one that's printed.

I don't want to attach a full patch. I reduced the code to: if (aWindow.document.location.href === "about:blank") { // A window is available, but it's not yet fully loaded. // Add an event listener to fire when the window is completely loaded. if (aShouldAttachListener) { dump("ACE: set up load="+aWindow.document.location.href+"\n"); aWindow.addEventListener("load", function listener1() { ExtensionSupport._checkAndRunMatchingExtensions(aWindow, "load"); }, { once: true }); } } else { // Window fully loaded in the past, run the extension code. ExtensionSupport._checkAndRunMatchingExtensions(aWindow, "load", aID); } according to my previous comment and the test still passes.

This does the job just as well: _setupWindow(aWindow, aShouldAttachListener, aID) { // Many windows first load about:blank, then unload it and then load // the real .xul file. We ignore these dummy transitions and wait for the real URL. if (aWindow.document.location.href === "about:blank") { // A window is available, but it's not yet fully loaded. // Add an event listener to fire when the window is completely loaded. if (aShouldAttachListener) { dump("ACE: set up load="+aWindow.document.location.href+"\n"); aWindow.addEventListener("load", function listener1() { aWindow.addEventListener("unload", function listener2() { ExtensionSupport._checkAndRunMatchingExtensions(aWindow, "unload"); }, { once: true }); ExtensionSupport._checkAndRunMatchingExtensions(aWindow, "load"); }, { once: true }); } } else { // Window fully loaded in the past, run the extension code. ExtensionSupport._checkAndRunMatchingExtensions(aWindow, "load", aID); } }, I'm not sure this is 100% correct since I don't know what happens to attaching the "unload" in the 'else' branch. That said, you might want to refactor the whole thing and only put "good" windows into your list.

Thanks for the checks and findings. (In reply to Jorg K (GMT+1) from comment #14) > @@ +142,5 @@ > > + * onLoadWindow {function} The callback function to run when window loads > > + * the matching document. > > + * onUnloadWindow {function} The callback function to run when window > > + * unloads the matching document. > > + * Both callbacks receive the matching window object as argument. > > Indentation. Return value? OK, I added description of return value. But this line applies to both of the arguments above so how should I indent it? > @@ +155,5 @@ > > + if (!("onLoadWindow" in aExtensionHook) && !("onLoadWindow" in aExtensionHook)) { > > Yes, repeated condition. Yes, the other should be "onUnloadWindow". > @@ +174,5 @@ > > + // Get the list of windows already open. > > + let windows = Services.wm.getEnumerator(null); > > + while (windows.hasMoreElements()) { > > + let domWindow = windows.getNext().QueryInterface(Ci.nsIDOMWindow); > > + openWindowList.add(domWindow); > > Here you add the window to the list. But it could be one that hasn't fully > loaded yet. Why not only collect "good" windows? Why only "good" ones? We have code to skip about:blank URLs. And then, how would we get notification when a window has become good and we can added? Looks like more complication. > @@ +202,5 @@ > > + } > > + > > + extensionHooks.delete(aID); > > + // Remove our global listener if there are no callers registered anymore. > > + if (extensionHooks.size == 0) { > > Why? You can leave it for the next registration. In the common case there will be no next registration. Either all the addons unregister on shutdown and we can remove the listener (as it won't ever be needed next), or they don't, as you say. Then we will never unregister the listener. > Then you'd need to have a > global to tell you whether you already attached the listener. You can't > guarantee that add-ons will unregister anyway. So why adding a new global var when the number of registered addons can be used? > @@ +216,5 @@ > > + // nsIWindowMediatorListener functions > > + onOpenWindow(xulWindow) { > > + // A new window has opened. > > + let domWindow = xulWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindow); > > + openWindowList.add(domWindow); > > Same here, new window comes in, in the list it goes, although it might still > be an incomplete one. That doesn't seem to be a problem (we handle about:blank). (In reply to Jorg K (GMT+1) from comment #15) > In the test the comment line as to how to run it is missing: > mozmake SOLO_TEST=utils/test-extensionSupport.js mozmill-one I'm not a fan of these lines, there are very few of them in our tests and it is even invalid on Linux/Mac as the command is different. (In reply to Jorg K (GMT+1) from comment #16) > ::: common/src/extensionSupport.jsm > @@ +248,5 @@ > > + aWindow.addEventListener("load", function listener1() { > > + if (aWindow.document.location.href !== "about:blank") { > > + aWindow.removeEventListener("load", listener1); > > + ExtensionSupport._checkAndRunMatchingExtensions(aWindow, "load"); > > + } else { dump("ACE: ignored:"+aWindow.document.location.href+"\n"); } > > Change this to "ignored(1)" and you will see that this never gets printed, > the one below does get printed. > > Therefore the inner check !== "about:blank" is not necessary. When the load > notification runs, the href *is* final. > You should change the debug to "ignored(2)". That's the one that's printed. Thanks, good catch. We do not get to see the "load" event of about:blank, but we see the unload event of it. (In reply to Jorg K (GMT+1) from comment #18) > This does the job just as well: > _setupWindow(aWindow, aShouldAttachListener, aID) { > // Many windows first load about:blank, then unload it and then load > // the real .xul file. We ignore these dummy transitions and wait for > the real URL. > if (aWindow.document.location.href === "about:blank") { > // A window is available, but it's not yet fully loaded. > // Add an event listener to fire when the window is completely loaded. > if (aShouldAttachListener) { > dump("ACE: set up load="+aWindow.document.location.href+"\n"); > aWindow.addEventListener("load", function listener1() { > aWindow.addEventListener("unload", function listener2() { > ExtensionSupport._checkAndRunMatchingExtensions(aWindow, > "unload"); > }, { once: true }); > ExtensionSupport._checkAndRunMatchingExtensions(aWindow, "load"); > }, { once: true }); > } > } else { > // Window fully loaded in the past, run the extension code. > ExtensionSupport._checkAndRunMatchingExtensions(aWindow, "load", aID); > } > }, > > I'm not sure this is 100% correct since I don't know what happens to > attaching the "unload" in the 'else' branch. I think this breaks the patch as it will never register the "unload" listener on windows that are already open. This just can't be seen in the tests because every window will already have the listeners added for the mozmill support addons. > That said, you might want to refactor the whole thing and only put "good" > windows into your list. I think this would complicate things.

(In reply to :aceman from comment #19) > > > + * onUnloadWindow {function} The callback function to run when window > > > + * unloads the matching document. > > > + * Both callbacks receive the matching window object as argument. > But this line applies to both of > the arguments above so how should I indent it? Like so? > Why only "good" ones? We have code to skip about:blank URLs. And then, how > would we get notification when a window has become good and we can added? > Looks like more complication. I think the logic would actually be simpler. Loop through the existing windows: if not about:blank: call LoadWorker. else call your setup function. LoadWorker: Add to your list, attach the unload listener. Call all add-ons which were interested. Setup function: Attach load listener, once: true, calling LoadWorker. onOpenWindow: call your setup function.

This is how I'd do it. Still needs cleaning up.

I did the clean-up and addressed the remaining review issues (I hope).

Comment on attachment 8968448 [details] [diff] [review] 1450288.patch - JK v5 Review of attachment 8968448 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for playing with this and re-architecting it. I do not find it simpler, but if you do and it still works, then I can accept it. I have fixed your nits from the review in parallel and also improved the test cases so I will now merge the patches. ::: common/src/extensionSupport.jsm @@ +170,5 @@ > + Services.wm.addListener(this._windowListener); > + } > + > + if (openWindowList) { > + // We already have a list of open windonws, notify the add-on about them. Typo :)

This should be the merge of the patches.

Comment on attachment 8968671 [details] [diff] [review] 1450288-jk.patch v6 Review of attachment 8968671 [details] [diff] [review]: ----------------------------------------------------------------- Looking good. I can fix the nits at checkin. I'm not sure about the "make" vs. "mozmake". ::: common/src/extensionSupport.jsm @@ +246,5 @@ > + /** > + * Set up listeners to run the callbacks on the given window. > + * > + * @param aWindow {nsIDOMWindow} The window to set up. > + * @param aID {String} Optional. Id of the new caller that has registered right now. ID. @@ +262,5 @@ > + * add it to our list, attach the "unload" listener to it and notify interested > + * callers. > + * > + * @param aWindow {nsIDOMWindow} The window to process. > + * @param aID {String} Optional. Id of the new caller that has registered right now. ID. ::: mail/test/mozmill/utils/test-extensionSupport.js @@ +5,5 @@ > +/** > + * Tests extensionSupport.jsm functions. > + */ > + > +// mozmake SOLO_TEST=utils/test-extensionSupport.js mozmill-one We usually write "make" as I had it. Is "mozmake" more correct?

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/8d636ad5810b introduce ExtensionSupport.registerWindowListener() so add-ons don't need a boilerplate. r=jorgk

Landed with a few comment tweaks and "chrome://messenger/content/messengercompose/messengercompose.xul" removed from the list of URLs JS Bridge cares about. If we want to run Mozmill on the compose window only, we'll have to add this again.