1448808 - Implement overlay loader for Calendar and other add-ons

Status: RESOLVED FIXED

(MailNews Core :: XUL Replacements, enhancement)

Description

[Jorg K (CEST = GMT+2)]

First bug in the new XUL Replacements category ;-)

M-C has removed all its overlays in bug 1426763 and will start crashing on overlay load in bug 1448162 comment #0. It will then remove platform support for overlays entirely.

We're busily removing overlays from C-C code, with only a few to go.

However, to support Calendar and other add-ons it's desirable to re-establish some overlay load mechanism. We'd still have to see whether that's feasible when M-C removes all overlay support from Gecko.

Comment 1

[Jorg K (CEST = GMT+2)]

Any effort here might be futile since non-restartless add-ons just can't be installed any more, see bug 1449149 comment #1.

Comment 2

[Patrick Brunschwig]

We can still provide a generic overlay loader to add-ons. But it needs to be triggered from bootstrap.js:startup().

Comment 3

[Jorg K (CEST = GMT+2)]

Any news here? What are the plans for getting Calendar going again?

Comment 4

[Patrick Brunschwig]

I'm almost done with the loader from my side. The current status is available from here, and already works fine in Enigmail:

https://sourceforge.net/p/enigmail/source/ci/master/tree/package/overlays.jsm

I'm currently on travel, that's why the progress is a bit slow.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

From my side, I am finishing up the WebExtension patches for tabs in Windows APIs. This is a prerequisite to all other web extension APIs, including our custom API that will allow WebExtensions to contain a chrome.manifest loaded using XPCOM. I have a proof of concept running with a simple legacy add-on, but I have not adapted lightning to use it.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: WiP - v1

Here is a start. There are still a few things not working
- mysterious errors about components not registered when calendar.icaljs=true
- Some error about EXPORTED_SYMBOLS not being set


Patrick, I've taken a look at the overlay loader, great work so far! I'd love if it were possible to go by without making any changes to existing xul overlays, so that this would just be a drop-in. Do you think that would be possible? I know toolbars are the exception.

I think we should implement the following remaining features:
- inline scripts (not sure if this is just a stale comment?)
- reading xml-stylesheets and xul-overlay processing instructions (can be done via xpath it seems)
- can we make the load event work as before? It looks like there were maybe some roadblocks
- I would appreciate a little ES6+ magic, mainly use of async/await and the fetch api

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Lightning WebExtension - v1

I'm still sorting out a few things I believe may be Lightning issues, but if you can get the overlays to load with this xpi that would be marvelous. I suspect Lightning not loading its UI may be due to missing support for PIs, but maybe it is something in the backend loader as well.

Comment 8

[Jorg K (CEST = GMT+2)]

Philipp, I was just about to land bug 1450479. Looks like that would rot your patch but also remove functionality you want to maintain?

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Given this is WiP go ahead and land it. We can still resurrect it as part of this patch.

Comment 10

[Jorg K (CEST = GMT+2)]

I think I'll wait a bit. I'm just not sure how M-C will handle non-bootstrapped add-ons in the future. I see
  let enabledAddons = Services.dirsvc.get("XREExtDL", Ci.nsISimpleEnumerator);

That won't continue to work, right?

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Legacy Add-on Converter - v1

Also, here is a thing you can use to turn your favorite legacy add-on into a WX, at least rudimentary. Run it without arguments in the unzipped extension directory.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Jorg K (GMT+1) from comment #10)
> I think I'll wait a bit. I'm just not sure how M-C will handle
> non-bootstrapped add-ons in the future. I see
>   let enabledAddons = Services.dirsvc.get("XREExtDL",
> Ci.nsISimpleEnumerator);
> 
> That won't continue to work, right?

No idea, given your other bug probably not. I don't use that part of the code.

Comment 13

[Patrick Brunschwig]

(In reply to Philipp Kewisch [:Fallen]  from comment #6)
> Patrick, I've taken a look at the overlay loader, great work so far! I'd
> love if it were possible to go by without making any changes to existing xul
> overlays, so that this would just be a drop-in. Do you think that would be
> possible? I know toolbars are the exception.
> 
> I think we should implement the following remaining features:
> - inline scripts (not sure if this is just a stale comment?)

It's not just a stale comment, but I think that should not be difficult.

> - reading xml-stylesheets and xul-overlay processing instructions (can be
> done via xpath it seems)

Which xul-overlay processing instructions do you talk about?

> - can we make the load event work as before? It looks like there were maybe
> some roadblocks

I think that's impossible for two reasons:
1. the "load" event already triggers loading the overlays in bug 1450288. Re-issuing "load" would generate an infinite loop.
2. overlays.jsm does not know which other overlays will be loaded for other addons. We need an event to tell each sepcific addon that its overlays are loaded. Just because the overlays for addon A are loaded doesn't mean that the overlays for addon B are loaded.


> - I would appreciate a little ES6+ magic, mainly use of async/await and the
> fetch api

Well yes ... My available time is quite limited. If you want something working soon, then someone else will have to work on that...

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Patrick Brunschwig from comment #13)
> (In reply to Philipp Kewisch [:Fallen]  from comment #6)
> > Patrick, I've taken a look at the overlay loader, great work so far! I'd
> > love if it were possible to go by without making any changes to existing xul
> > overlays, so that this would just be a drop-in. Do you think that would be
> > possible? I know toolbars are the exception.
> > 
> > I think we should implement the following remaining features:
> > - inline scripts (not sure if this is just a stale comment?)
> 
> It's not just a stale comment, but I think that should not be difficult.
Ah ok, I asked because I misread some of the code that looked like it already handles this. Glad to hear it isn't difficult though!

> 
> > - reading xml-stylesheets and xul-overlay processing instructions (can be
> > done via xpath it seems)
> 
> Which xul-overlay processing instructions do you talk about?

These for example: https://searchfox.org/comm-central/source/calendar/lightning/content/messenger-overlay-sidebar.xul#17,26

> 
> > - can we make the load event work as before? It looks like there were maybe
> > some roadblocks
> 
> I think that's impossible for two reasons:
> 1. the "load" event already triggers loading the overlays in bug 1450288.
> Re-issuing "load" would generate an infinite loop.
Could we load the overlays earlier, e.g. DOMContentLoaded, or beforescriptexecute? I'm not sure what the order is, but maybe thre is some internal event that fires just at the time where in the past overlays are loaded. We could either load the overlays synchronously, or preload overlays into memory to apply them right away.


> 2. overlays.jsm does not know which other overlays will be loaded for other
> addons. We need an event to tell each sepcific addon that its overlays are
> loaded. Just because the overlays for addon A are loaded doesn't mean that
> the overlays for addon B are loaded.
Ok, I see. Just to clarify, does this load event just fire for when the overlay completes loading, or is it a replacement for using addEventListener("load", ...) inside an add-on to wait for the window load to complete? For the former case I'm not overly concerned, the latter case seems quite common and will need some messaging to make sure developers catch up.


> > - I would appreciate a little ES6+ magic, mainly use of async/await and the
> > fetch api
> 
> Well yes ... My available time is quite limited. If you want something
> working soon, then someone else will have to work on that...
Of course, I understand. I might take a stab at it, it shouldn't be too difficult.

Comment 15

[Patrick Brunschwig]

(In reply to Philipp Kewisch [:Fallen]  from comment #14)
> > > - reading xml-stylesheets and xul-overlay processing instructions (can be
> > > done via xpath it seems)
> > 
> > Which xul-overlay processing instructions do you talk about?
> 
> These for example:
> https://searchfox.org/comm-central/source/calendar/lightning/content/
> messenger-overlay-sidebar.xul#17,26

That should be feasible.

> > > - can we make the load event work as before? It looks like there were maybe
> > > some roadblocks
> > 
> > I think that's impossible for two reasons:
> > 1. the "load" event already triggers loading the overlays in bug 1450288.
> > Re-issuing "load" would generate an infinite loop.
> Could we load the overlays earlier, e.g. DOMContentLoaded, or
> beforescriptexecute? I'm not sure what the order is, but maybe thre is some
> internal event that fires just at the time where in the past overlays are
> loaded. We could either load the overlays synchronously, or preload overlays
> into memory to apply them right away.

In theory yes. Can anybody point me to a list of all events that get fired when a window is loaded? In any case, it must be an event that is fired for *any* window.


> > 2. overlays.jsm does not know which other overlays will be loaded for other
> > addons. We need an event to tell each sepcific addon that its overlays are
> > loaded. Just because the overlays for addon A are loaded doesn't mean that
> > the overlays for addon B are loaded.
> Ok, I see. Just to clarify, does this load event just fire for when the
> overlay completes loading, or is it a replacement for using
> addEventListener("load", ...) inside an add-on to wait for the window load
> to complete? For the former case I'm not overly concerned, the latter case
> seems quite common and will need some messaging to make sure developers
> catch up.

This fires after the overlay is completely loaded (incl. scripts & CSS). It must be treated by addons as replacement for addEventListener("load", ...), because at the moment the "load" event is fired, no code of any addon is loaded. Yes, we need to make sure that developers get to know this.

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: WiP - v2

I did some ES6 refactoring, made eslint happy, and did some minor code tweaks. I'm sorry if I mutated it too much! The refactoring worked on the first try, I'm a bit wary of that since I may be missing an edge case and I instead broke something.

I've also implemented the processing instructions code and found a way to fake the load handler without making add-ons change things. The monkey patching doesn't look pretty, but the alternatives don't really work. I tried with Object.create(targetWindow) and that gave some error about the window property not being correct. I tried using a Proxy, but that didn't call the load handler properly.

I was also looking for a list of all DOM events that get fired when the window loads but didn't find anything. I didn't look to extensively though. For the *any* window part, I think there is only the listener on Services.ww, with which you'd then have to add a normal event listener after it notifies.

With this WiP version I get a bit further with Lightning. It loads various scripts and I can open tabs, but there are a few js errors as well and the tab doesn't load right.

My suspicion is that there is a discrepancy in script load order between the xul addon version and the WebExtension. We'd have to do some printf-debugging to figure that out.

Comment 17

[Patrick Brunschwig]

Great, I had a quick look and things look alright.

So for the moment, the main thing we need to make addons happy would be the support of inline-JS (and remove the comments at the top).

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

So, it is much more than what I mentioned. XUL overlays support forward references to nodes that don't exist yet but are loaded by other overlays. It also supports nested overlays. I rewrote the overlay loader to support that, but am stuck on more js errors due to other load order issues. Lightning is really a good test bed for this.

Comment 19

[Patrick Brunschwig]

Attachment: Overlays.jsm - WIP v3

I couln't apply your patch (it tries to modify some non-exisiting files on my comm-central). Nonetheless, I extracted Overlays.jsm and added support for inline scripts.

I used window.eval() to load the script. Do you think that's safe enough in this context, or should I better use Cu.Sandbox (which would cause a lot more overhead)?

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

The patch requires bug 1455471. Hold up for a moment though, I've made some more drastic changes to Overlay.jsm to support xul overlay forward references. I'm down to one js error that is again a race condition, adding a random 100ms timeout fixes it. Also it seems domWindowUtils.loadSheetUsingURIString doesn't record an entry to document.styleSheets which is causing a new js error.

Comment 21

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: WiP - v4

Here is another update. I basically took the xul overlay loader code from XULDocument.cpp and wrote it in Javascript leaving out parts that seemed not useful.


A few kinks:
- I haven't re-added support for toolbars. Do you think you could add this back in? I vaguely recall what the issues were, but they may be more present for you.
- There is an ugly 500ms timeout before running load handlers due to async loading even with the synchronous subscript loader, I suspect something to do with xbl bindings. I haven't found a way to get rid of it, and it will probably break on slow computers, or with a lot of sheets/scripts. It seems to work for Lightning on my computer though. If we can't find a way around this maybe we should just set it based on a pref.
- Alas, I had it fully working, then I did some cleanup and added docs, and now it is broken again. Either the debug logs delayed things so that it happened to work, or I made a mistake on the way. Too tired to fix this now.

Comment 22

[Patrick Brunschwig]

I'm on travel today and tomorrow. Here is the code for the toolbar:



    function getToolbarNthTag(toolbar, tagName, elemIndex) {
      if (elemIndex >= 0) {
        let s = new RegExp(`^${tagName}[0-9]+$`);
        let node = toolbar.firstChild;
        let n = -1;
        while (node) {
          if (node.id.search(s) === 0) n++;
          if (n == elemIndex) return node;
          node = node.nextSibling;
        }
      }

      return null;
    }

    /**
     * get toolbar element for separator, spacer and spring
     */
    function getToolbarElem(toolbar, currentset, index) {
      if (currentset[index] && (currentset[index].search(/^(separator|spacer|spring)$/) === 0)) {
        let target = currentset[index];
        let foundIndex = -1;
        // find the n-th separator/spacer/spring
        for (let i = 0; i < index + 1; i++) {
          if (currentset[i] === target) ++foundIndex;
        }

        return getToolbarNthTag(toolbar, target, foundIndex);
      }
      return null;
    }

    /**
     * Add a button at the correct place on a toolbar.
     * Buttons are always added to the toolbar palette. Whether or not the button is added to the
     * toolbar depends on:
     * 1. if it's in the currentset of the toolbar (i.e. added previously)
     * 2. if it's defined a default button and the button has never been added to the toolbar before
     *
     * @param palette:      Object - the toolbar palette containing all buttons (also invisible ones)
     * @param toolbarButton Object - the button to add
     * @param toolbarId     String - the ID of the toolbar where the button shall be added
     *
     */
    function addToolbarButton(palette, toolbarButton, toolbarId) {
      oconsole.log(`adding button '${toolbarButton.id}' to '${toolbarId}'`);

      let toolbar = $(toolbarId);
      let buttonId = toolbarButton.id;

      let currentset = toolbar.getAttribute("currentset").split(/,/);
      if (toolbar.getAttribute("currentset").length === 0) {
        currentset = toolbar.getAttribute("defaultset").split(/,/);
      }

      toolbarButton.setAttribute("overlay_source", addonID);
      palette.appendChild(toolbarButton);

      let index = currentset.indexOf(buttonId);
      if (index >= 0) {
        // button was added before
        let before = null;

        for (let i = index + 1; i < currentset.length; i++) {
          if (currentset[i].search(/^(separator|spacer|spring)$/) < 0) {
            before = $(currentset[i]);
          }
          else {
            before = getToolbarElem(toolbar, currentset, i);
          }

          if (before) {
            break;
          }
        }

        toolbar.insertItem(buttonId, before);
      }
    }


        if (node.tagName === "toolbarpalette") {
          let toolboxId = node.getAttribute("targetToolbox");
          let toolbarId = node.getAttribute("targetToolbar");
          let defaultSet = node.getAttribute("targetToolbarDefaultset");
          if (!toolboxId) {
            oconsole.log("injectDOM: cannot overlay toolbarpalette: no target toolbox defined");
            continue;
          }
          if (!toolbarId) {
            oconsole.log("injectDOM: cannot overlay toolbarpalette: no target toolbar defined");
            continue;
          }

          if (defaultSet) {
            let toolbar = $(toolbarId);
            if (toolbar) {
              toolbar.setAttribute("defaultset", defaultSet);
            }
          }

          let toolbox = $(toolboxId);
          let palette = toolbox.palette;
          let c = node.children;

          while (c.length > 0) {
            // added toolbar buttons are removed from the palette's children
            if (c[0].tagName && c[0].tagName === "toolbarbutton") {
              addToolbarButton(palette, c[0], toolbarId);
            }
          }
        }

Comment 23

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: WiP - v5

So this mostly works now. The issue I had before was a missing return statement, but in the meanwhile I've made those functions synchronous because this allows to load the scripts before the window load handler fires and have the scripts ready.

Loading without a restart is quite a race, I've bumped the timeout to 1500 in this case which should be good enough for now. If you install Lightning normally and then restart, it will fairly reliably work because at the time the window is not yet loaded and we use the normal load handlers.

I contemplated trying to fool the AOM to consider the add-on requiring a restart, but that was a little more involved. I think in the end it would be best to somehow notify though, and only load such legacy webextensions with a restart required.

I'm starting to run out of time for this patch though, I'd really like to make this ready to land.

Patrick, I kind of have the toolbar code in. I'm now finding the toolbarpalette correctly and merging elements in. I didn't add any code to auto-add buttons into the default set, I don't think the legacy xul overlays did this either?

Anyway, let me know what you think so far and what else you think still needs to be done. If anyone else has feedback please let me know.

Comment 24

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: WiP - v5

Comment 25

[Patrick Brunschwig]

Comment on attachment 8971825 [details] [diff] [review]
WiP - v5

Nice approach indeed. The 1500 ms hack is really ugly. No matter what value you use here, it can still be too small. Asynchronous functions can take _any_ amont of time to complete. 

I checked XULDocument.cpp, they seem to be able to separate compiling scripts from executing them, but I assume that this is possible in JavaScript. 

Maybe you could consider rewriting the Lightning code at this point, such that nothing is executed asynchronously before the "load" event is fired? This would avoid any timeout. I'd say this could be a valid restriction to tell addon authors?

Comment 26

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Patrick Brunschwig from comment #25)
> I checked XULDocument.cpp, they seem to be able to separate compiling
> scripts from executing them, but I assume that this is possible in
> JavaScript. 
I'm not sure it is, nsXULPrototypeCache doesn't have an idl interface.


> Maybe you could consider rewriting the Lightning code at this point, such
> that nothing is executed asynchronously before the "load" event is fired?
> This would avoid any timeout. I'd say this could be a valid restriction to
> tell addon authors?

Yeah, I'll have to debug what exactly was the issue here. I seem to recall what was happening here was that xbl constructors and event handlers were executing in the wrong order w.r.t. the load events. At the time, I wasn't able to get the load order between window scripts, stylesheets, and load handlers right so that it worked without a timeout.

I think the better approach here would be to have ext-legacy not load the chrome.manifest when it is initially installed and notify that a restart is required. In such cases, the load works fine since overlay loading completes before the window load completes. This matches the behavior of legacy xul add-ons much more and doesn't require special casing the load event.

Lightning is of course the extreme case here, a lot of other add-ons will probably "just work" without the timeout. We can certainly make changes to Lightning to accommodate, but of course I'd like it to work as seamless as possible within reasonable effort.

Comment 27

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v6

So, this handles the rest, making sure the add-on is not loaded until restart. There are some edge cases that are not covered, but I think are ok:

* If you "remove" the legacy add-on and close the add-ons manager, it will disappear from the list. The legacy code will still be running, so it really will go away on restart. I couldn't find a way without hacking the add-ons manager code in a way that m-c would not accept the patch.

* If you enable a disabled add-on, the message appears as expected, but if you then disable it again it will not just clear the message until restart. I think this is acceptable.

Together with bug 1457087, Lightning works again in nightly with this patch.

Comment 28

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Change Lightning's install.rdf to a manifest.json - v1

Comment 29

[Jorg K (CEST = GMT+2)]

(In reply to Philipp Kewisch [:Fallen]  from comment #27)
> Together with bug 1457087, Lightning works again in nightly with this patch.
Umm, the patch here doesn't apply, looks like bug 1455471 is also be required.

A try would be nice to see what tests fail. Please note that I disabled MozMill for Calendar in a very crude way, so these need to be backed out (in reverse order):
https://hg.mozilla.org/comm-central/rev/784233e550fd
https://hg.mozilla.org/comm-central/rev/fc93de1b8856

Comment 30

[Geoff Lankow (:darktrojan)]

Comment on attachment 8985867 [details] [diff] [review]
Fix - v6

Review of attachment 8985867 [details] [diff] [review]:
-----------------------------------------------------------------

Overall I'm happy with this. It's a big hack, but I don't think we have a lot of choice.

::: common/src/Overlays.jsm
@@ +401,5 @@
> +                       this.window.location, ex.message);
> +      }
> +    }
> +
> +    this.window.addEventListener = oldAddEventListener;

Does this replacement happen more than once if there are multiple scripts? If so, does it matter?

::: common/src/extensionSupport.jsm
@@ +26,5 @@
>  function extensionDefaults() {
> +  // Fetch enabled non-bootstrapped add-ons.
> +  let enabledAddons = Services.dirsvc.get("XREExtDL", Ci.nsISimpleEnumerator);
> +  for (let addonFile of fixIterator(enabledAddons, Ci.nsIFile)) {
> +    loadAddonPrefs(addonFile);

ExtensionSupport.loadAddonPrefs ?

Comment 31

[:aceman]

Is this for TB60?
As for trunk we are removing the loadAddonPrefs() as non-bootstrapped addons aren't loaded anymore.

Comment 32

[Jorg K (CEST = GMT+2)]

It's for trunk. That's the whole point, they're doing a hack here so non-bootstrapped keep working.

Comment 33

[:aceman]

So this is now not only about loading overlays, but the scope expanded to make non-bootstrapped addons work on trunk?? Where did that come from, it isn't mentioned here. What's the plan with that? Will we try to go against m-c in this?

Comment 34

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to :aceman from comment #33)
> So this is now not only about loading overlays, but the scope expanded to
> make non-bootstrapped addons work on trunk?? Where did that come from, it
> isn't mentioned here. 
It has been that way for the last few patch versions, I'm sorry it was never explicitly mentioned.

What's the plan with that? Will we try to go against
> m-c in this?

This is a stop gap, we have a lot of legacy add-ons and have not done any migration, nor are we ready for WebExtensions yet. I don't want to go too far down this rabbit hole, but it will buy us some time while we get our WX game on.

(In reply to Jorg K (GMT+2) from comment #29)
> (In reply to Philipp Kewisch [:Fallen]  from comment #27)
> > Together with bug 1457087, Lightning works again in nightly with this patch.
> Umm, the patch here doesn't apply, looks like bug 1455471 is also be
> required.
Yes

> 
> A try would be nice to see what tests fail. Please note that I disabled
> MozMill for Calendar in a very crude way, so these need to be backed out (in
> reverse order):
> https://hg.mozilla.org/comm-central/rev/784233e550fd
> https://hg.mozilla.org/comm-central/rev/fc93de1b8856

I agree we need to get tests running, I'm not sure however that this patch will help with that. The unit tests load the lightning components manually, which will not be using this loader.

Comment 35

[Jorg K (CEST = GMT+2)]

(In reply to Philipp Kewisch [:Fallen]  from comment #34)
> > A try would be nice to see what tests fail. Please note that I disabled
> > MozMill for Calendar in a very crude way, so these need to be backed out (in
> > reverse order):
> > https://hg.mozilla.org/comm-central/rev/784233e550fd
> > https://hg.mozilla.org/comm-central/rev/fc93de1b8856
> I agree we need to get tests running, I'm not sure however that this patch
> will help with that. The unit tests load the lightning components manually,
> which will not be using this loader.
I don't follow.

Yes, the patch here doesn't take care of the Xpcshell (unit) tests, that's done in bug 1457087; looks like you also need to unpack the XPI, more reading on this in bug 1466430 comment #8 and bug 1466430 comment #13.

However, the patch here makes "Lightning work again", so it should then be available in TB and MozMill tests should pass. To enable MozMill again, you need the backouts I mentioned.

Comment 36

[Jorg K (CEST = GMT+2)]

Attachment: backout-fc93de1b8856 - Backout patch to bring back Calendar's MozMill tests

Comment 37

[Jorg K (CEST = GMT+2)]

I've applied these patches
summary:     Bug 1455471 - Implement Thunderbird WebExtensions tabs and windows API. r=mkmelin
summary:     [mq]: fix (nsIXULelement bustage)
summary:     Bug 1448808 - Implement legacy overlay loader for WebExtensions. r=darktrojan
summary:     Bug 1448808 - Change Lightning's install.rdf to a manifest.json with legacy. r=darktrojan
summary:     Bug 1448808 - Bring back Calendar MozMill testing. a=backout

Result: Calendar appears to be enabled, I see it in the options, but there is no "Events and Tasks" on the menu bar and there is no calendar and no task icon in the tab bar. Also nothing in the hamburger menu. So somehow I cannot launch the Calendar.

I was tempted to ship this off to a try run, but I'm afraid MozMill will fail since it would used the menu/tab bar items which aren't there.

Am I missing something?

There are also a few JS errors like:
JavaScript error: chrome://calendar/content/calendar-task-view.js, line 273: TypeError: toolbox is null
JavaScript error: chrome://calendar/content/agenda-listbox.js, line 30: TypeError: this.agendaListboxControl is null
JavaScript error: chrome://lightning/content/imip-bar.js, line 107: TypeError: document.getElementById(...) is null
JavaScript error: chrome://calendar/content/calendar-extract.js, line 268: TypeError: contextMenuEvent is null

Comment 38

[Patrick Brunschwig]

Comment on attachment 8985867 [details] [diff] [review]
Fix - v6

I'm sorry, I didn't notice the "feedback?" until now. I think we can live well with the edge cases.

Comment 39

[Jorg K (CEST = GMT+2)]

Geoff pointed out a typo in ext-legacy.js, so with that fixed, it looked more promising ;-)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=345a24a5fed63d4b462595ff407ae7f9ff1a5878

Comment 40

[Geoff Lankow (:darktrojan)]

The overlay provider is going to need to deal with persistence (ie. getting attribute values from xulstore.json). I'm not sure when it's supposed to happen, but it should be possible to replicate with nsIXULStore methods.

Comment 41

[Geoff Lankow (:darktrojan)]

Also, I think the oconsole.log messages should be oconsole.debug messages, as that'll make it easier to filter them out when trying to fix other things.

Comment 42

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #39)
> Geoff pointed out a typo in ext-legacy.js, so with that fixed, it looked
> more promising ;-)
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=345a24a5fed63d4b462595ff407ae7f9ff1a5878
That try run wasn't a complete success :-(
Linux didn't build, perhaps unrelated, Xcpshell seems to show the same errors we tried to fix in bug 1466430, perhaps that now conflicts with the "component loader", and MozMill has a heap of errors like:
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1529500965/build/tests/mozmill/shared-modules/test-calendar-utils.js | test-calendar-utils.js::<TOP_LEVEL>
That <TOP_LEVEL> indicates some JS problem.

I don't want to interfere with assignee and reviewer, so I won't do anything further here.

Comment 43

[Geoff Lankow (:darktrojan)]

Through much trial and error we've established that removing the install.rdf file causes the packaging to totally break. Fixing mozilla-central to look for something else instead causes other things to break.

If we keep install.rdf (but change the contents to explain why we're keeping it - an empty file causes yet more things to break) an acceptable solution?

Comment 44

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Can you provide more info on what is breaking for the packaging? I guess it would be a viable hackaround, but I'd much rather see the packaging code be fixed.

Comment 45

[Geoff Lankow (:darktrojan)]

https://searchfox.org/mozilla-central/source/python/mozbuild/mozpack/packager/__init__.py#256

If this function comes across a file called install.rdf it decides that it is packing an addon and responds appropriately. I changed it to also check for manifest.json files, and a lot of tests broke. (Mostly not ours.)

Now that I've had another look at, I have a bad feeling this part of the packager will go away altogether in future, given the direction the Firefox team is moving. We might need to find an alternative.

Comment 46

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I'm pretty sure it will go away. How though does Firefox handle system add-ons shipped with Firefox? I suspect we need to go down the same path. 

I've been leaning the other way in the past, but possibly we could also just ship Lightning as a system add-on. I have been wanting Lightning to present itself in a way that when there are no calendars configured, it would be visually disabled, e.g. the UI doesn't appear. Once the user receives invitations or makes use of some other select features, calendaring would be enabled. But I guess that is a discussion for another bug.

Comment 47

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Oh I guess the packager code failing is mostly for l10n stuff? Firefox WebExtensions will of course have different localized files as per WebExtension format. I bet glandium might be a good person to ask, I think it was him that I talked with on the last set of mozpack changes required for Lightning.

Comment 48

[Geoff Lankow (:darktrojan)]

Can we revisit comment 43, given the carnage when I tried to fix the packager in bug 1472123?

Comment 49

[[:MakeMyDay]]

Comment on attachment 8985868 [details] [diff] [review]
Change Lightning's install.rdf to a manifest.json - v1

Review of attachment 8985868 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/lightning/manifest.json
@@ +18,5 @@
> +    }
> +  },
> +  "icons": {
> +    "32": "chrome/skin/linux/calendar/cal-icon32.png",
> +    "48": "chrome/skin/linux/calendar/cal-icon32.png"

We currently only have cal-icon24 and cal-icon32 icons in the tree. If you wanted to go for 48 here, can you please file a follow-up bug to introduce a native icon here and not scale up?

Comment 50

[[:MakeMyDay]]

Comment on attachment 8985867 [details] [diff] [review]
Fix - v6

Review of attachment 8985867 [details] [diff] [review]:
-----------------------------------------------------------------

When test-driving this, I observed an issue with loading of styles from chrome manifest - that doesn't seem to work as designed in this patch. E.g. the datetimepickers for events and tasks in the event dialog/tab, which should appear mutually exclusive are displayed side-by-side in the event dialog/tab for me. Their appearence is controlled by one of the css file included from chrome manifest. If I included the mentioned css file directly into calendar-event-dialog.xul likewise in bug 1450757, it works as expected again.

This behaviour is probably expected due to bug 1450753 and following the approsach of bug 1450757 might be an option. However, for Lightning we're injecting styles also to TB files (or to Lightning from gdata provider) in jar.mn [1], so such an approach wouldn't be sufficient and we would have to inject it otherwise.

[1] https://dxr.mozilla.org/comm-central/search?q=%22%25+style%22+path%3Acalendar&redirect=false

::: mail/components/extensions/parent/ext-legacy.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +ChromeUtils.defineModuleGetter(this, "ChromeManifest", "resource:///modules/ChromeManifest.jsm");
> +ChromeUtils.defineModuleGetter(this, "ExtensionSupport", "resource:///modules/ExtensionSupport.jsm");

This must be extensionSupport.jsm instead of ExtensionSupport.jsm to work.

Comment 51

[Geoff Lankow (:darktrojan)]

(In reply to [:MakeMyDay] from comment #50)
> This behaviour is probably expected due to bug 1450753 and following the
> approsach of bug 1450757 might be an option. However, for Lightning we're
> injecting styles also to TB files (or to Lightning from gdata provider) in
> jar.mn [1], so such an approach wouldn't be sufficient and we would have to
> inject it otherwise.

I've got a bunch of stuff including that and things like it to fix after this lands.

> This must be extensionSupport.jsm instead of ExtensionSupport.jsm to work.

Yes.

Comment 52

[Geoff Lankow (:darktrojan)]

(In reply to Geoff Lankow (:darktrojan) from comment #51)
> I've got a bunch of stuff including that and things like it to fix after
> this lands.

I should say, I've got fixes for a bunch of stuff including that.

Comment 53

[[:MakeMyDay]]

Can you please provide the bug references here? Or is there a meta bug to follow for requiered changes limited to Lightning to follow-up if any?

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7287778fe124
Implement legacy overlay loader for WebExtensions. r=darktrojan

Comment 55

[Geoff Lankow (:darktrojan)]

I should've set ni? with comment 48.

Comment 56

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I'm fine with keeping install.rdf as a hack. Can you make sure and test that it loads as the right kind of add-on when installed? I'm not sure if the presence of install.rdf will cause toolkit code to think it is a legacy add-on.

Comment 57

[Frank-Rainer Grahl (:frg)]

This seems to cause a compile error in SeaMonkey:

42:47.36 nsCommonModule.cpp
42:47.36 d:/seamonkey/comm-central/comm/common/src/nsCommonModule.cpp(2): fatal error C1083: Cannot open include file: 'nsCommonBase
CID.h': No such file or directory
42:47.36 mozmake.EXE[4]: *** [d:/seamonkey/comm-central/config/rules.mk:1034: nsCommonModule.obj] Error 2
42:47.39 mozmake.EXE[3]: *** [d:/seamonkey/comm-central/config/recurse.mk:74: comm/common/src/target] Error 2
42:47.39 mozmake.EXE[3]: *** Waiting for unfinished jobs....
42:47.74 sslspec.c

Does it work for Thunderbird with current default tip for c-c and m-c?

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/509c442a3700
Fix missing include file nsCommonBaseCID.h in suite compile. rs=bustage-fix

Comment 59

[Frank-Rainer Grahl (:frg)]

Found the error

Comment 60

[Jorg K (CEST = GMT+2)]

Comment on attachment 8985867 [details] [diff] [review]
Fix - v6

We will need to ship a TB 62 beta at some stage with working Calendar.

Comment 61

[Jorg K (CEST = GMT+2)]

Beta (TB 62):
https://hg.mozilla.org/releases/comm-beta/rev/74bfdeb9f25f
https://hg.mozilla.org/releases/comm-beta/rev/eccd4a2309f5

Geoff, please file a *Calendar* bug for the outstanding Calendar work.

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e81f4b59a00a
Change Lightning's install.rdf to a manifest.json with legacy. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/07addfefa1b6
Bring back Calendar MozMill testing. a=backout

Comment 63

[Jorg K (CEST = GMT+2)]

Beta (TB 62, Cal 6.4):
https://hg.mozilla.org/releases/comm-beta/rev/38233158f3e05f98f2bb8f13beab64ad0388613d - Calendar patch
https://hg.mozilla.org/releases/comm-beta/rev/8f1ad5daf40c72c413daf4caa2db41949b21ca79 - Calendar patch