In this bug I attempt to fix a number of flaws in the overlay loader landed in bug 1448808: * Removing the 1500ms timeout before firing "load" events. I've done this by adding an element and a stylesheet to the end of the document, waiting for the binding to apply, and then removing both. The stylesheet should (in theory) not be applied until all others in the document have been. It adds an XBL binding to the element, which fires a "bindingattached" event, which triggers the whole thing to be cleaned up and the "load" event to be fired. * "xul-overlay" processing instructions in the top-level document are not handled at all. I've added some code to handle this at least in the case of documents where the overlay loader runs. It should actually happen for all chrome documents, but doesn't, yet. * Stylesheets are not added for documents where there's a "sheet" directive in the manifest. * If a button is added to a toolbox palette, we set the toolbox's toolbars' "currentSet" attribute to add them to the toolbar. I don't think this yet handles the case where the toolbar has been modified from the default. * I've commented the code which prevents running in all cases except APP_STARTUP, because ADDON_INSTALL and ADDON_UPGRADE(/DOWNGRADE?) are also valid if the app is starting up. Need to find a replacement for this. * And finally, a bustage fix from mozilla-central refactoring.

I think it might be possible to observe "chrome-document-interactive" instead of "chrome-document-loaded" but I haven't looked into it yet, as I'm keen to get this stuff landing ASAP. As it currently is, at least we know what state the document is in when the overlays are applied.

Comment on attachment 8991248 [details] [diff] [review] 1474248-overlay-loader-1.diff Review of attachment 8991248 [details] [diff] [review]: ----------------------------------------------------------------- A few more comments to consider: ::: common/src/Overlays.jsm @@ +120,5 @@ > } > } > > // Prepare loading further nested xul overlays from the overlay > + Array.prototype.push.apply(unloadedOverlays, this._collectOverlays(doc)); unloadedOverlays.push(...this._collectOverlays(doc)); @@ +183,1 @@ > setTimeout(() => { Do we still need this setTimeout now that we have the overlay trigger? @@ +210,5 @@ > } > + }, 0); > + }, { once: true }); > + this.document.documentElement.appendChild(overlayTrigger); > + let sheet = this.loadCSS("chrome://messenger/content/overlayBindings.css"); I know why this works, but it feels so wrong to have a variable defined on the last line :) Can we load the sheet a bit earlier to improve readability or would that break things? @@ +261,5 @@ > if (node.id && node.localName == "toolbarpalette") { > + // These vanish from the document but still exist via the palette property > + let boxes = [...this.document.getElementsByTagName("toolbox")]; > + let box = boxes.find(box => box.palette && box.palette.id == node.id); > + let palette = box ? box.palette : null; Can we move this to later for perf? e.g. do all the processing, then once we are through find all the toolboxes once, turn them into a Map() for lookup, then resolve all palettes? @@ +270,5 @@ > } > > this._mergeElement(palette, node); > + > + for (let bar of [...box.querySelectorAll("toolbar")]) { No need to spread when using for..of, unless you are expecting the amount of toolbars to change while you are iterating.

Should be good to go now, I reckon.

Comment on attachment 8992100 [details] [diff] [review] 1474248-overlay-loader-2.diff Review of attachment 8992100 [details] [diff] [review]: ----------------------------------------------------------------- Did you check if the final setTimeout is still needed now that we wait for a binding to attach? Would be great if we can get rid of it. r+ with that considered.

I should've said. Yes, I tested that, and yes it's still necessary. Why? Dunno.

A few more changes here too. To keep OS X happy I've had to push the application of overlayBindings.css back to the end of the function. I don't understand why this is necessary, (it goes against what I thought I knew about the event loop), but it is.

Comment on attachment 8992224 [details] [diff] [review] 1474248-overlay-loader-3.diff Review of attachment 8992224 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/extensions/parent/ext-legacy.js @@ +95,5 @@ > } > > + let documentObserver = { > + observe(document) { > + if (document.toString() == "[object XULDocument]") { Can we use (document instanceof XULDocument) here? Another thing that came to mind, does remote XUL still work, and if so, can we limit this to local XUL documents?

We're already limited to system principal documents by the fact we're observing "chrome-document-loaded", and we refuse to load overlays that aren't chrome or resource protocol, is that limited enough?

Yes, that is limited enough for me. Thanks!

Not landing this right now due to bug 1474862 comment #5. I'd like to see the entire lot green first.

Okay, I've broken it by getting ahead of myself and adding bug 1470307 as well. The rest can land without it.

Sorry, I don't follow. You have two try runs https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=833519c1ee7c38435b0827a584645eaf9be8f6c4 https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3e06302c7db40861ee8e0f7e78af69fcd089b835 In the first one, the font tests fail, in the second, they pass, although both runs appear to include the same patches. If you bring back Mozmill testing, then you will need some form of bug 1470307 to address the fact that open_pref_window() doesn't exist any more. So what do you want landed? IRC: "you can land all this, assuming nothing goes badly". "All" being what exactly?

Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/f4b9ecb02a3b Improvements to legacy extension handling and the overlay loader; r=philipp

Beta (TB 62): https://hg.mozilla.org/releases/comm-beta/rev/8d96840851f322e476d94a6db4d6999a30cfef5d