Further overlay loader improvements

RESOLVED FIXED in Thunderbird 63.0

Status

enhancement
RESOLVED FIXED
Last year
10 months ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

(Blocks 2 bugs)

Trunk
Thunderbird 63.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Posted patch overlay-loader-1.diff (obsolete) — Splinter Review
I've managed to make some good performance improvements by observing chrome-document-interactive instead of chrome-document-loaded. I thought this might cause problems if the document finished loading while the overlay loader was doing things, but this has never happened to me, so I think it must wait.

I had a few issues with Lightning because of this change, but I'll fix them in another bug.
Attachment #8994455 - Flags: review?(philipp)
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Component: General → XUL Replacements
Product: Thunderbird → MailNews Core
Blocks: 1478578
Depends on: 1477958
Posted patch overlay-loader-2.diff (obsolete) — Splinter Review
I tried to avoid making this extra special case for deck elements, but I don't think I have a choice. Setting selectedIndex too early just causes odd behaviour.
Attachment #8994455 - Attachment is obsolete: true
Attachment #8994455 - Flags: review?(philipp)
Attachment #8996590 - Flags: review?(philipp)
No longer depends on: 1477958
Comment on attachment 8996590 [details] [diff] [review]
overlay-loader-2.diff

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

Did you remove common/content/overlayBindings.* and the jar.mn on purpose? It also seems like there are some unrelated changes about script load order?
Yes, they're not used any more, because we can use the document's real load event instead of trying to find the point at which we think our added stuff is loaded. Same applies to the script loading stuff.

…

But, now that you've made me think about it, I think there might be a case where we want all that stuff back. I need to investigate something I saw yesterday. Damn.
Comment on attachment 8996590 [details] [diff] [review]
overlay-loader-2.diff

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

::: common/src/Overlays.jsm
@@ +168,5 @@
>        this.loadCSS(sheet);
>      }
>  
> +    let decksToResolve = new Map();
> +    let ids = xulStore.getIDsEnumerator(this.location);

You're redeclaring ids here - it is already defoined in line 147.
Posted patch overlay-loader-3.diff (obsolete) — Splinter Review
I've changed this into a hybrid of what it was and what I wanted it to be, since I noticed that in some situations (such as terribly slow machines, like my Windows VM) the extension isn't loaded until after the first window appears. So we can now load overlays before OR after the window loads.

Bonus side effect: legacy extensions can be installed without a restart, although extension writers will probably need to be careful checking that their extension works properly in both cases. This can be done by disabling the extension in the Add-On Manager, restarting, then enabling the extension.
Attachment #8996590 - Attachment is obsolete: true
Attachment #8996590 - Flags: review?(philipp)
Attachment #8997265 - Flags: review?(philipp)
Depends on: 1483087
Blocks: 1476259
Blocks: 1477506
Comment on attachment 8997265 [details] [diff] [review]
overlay-loader-3.diff

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

Some drive-by comments. 
With the patch lightning seems to load consistently, though I didn't test in depth.

::: common/src/Overlays.jsm
@@ +176,5 @@
>  
> +    this._decksToResolve = new Map();
> +    for (let id of this.persistedIDs.values()) {
> +      let element = this.document.getElementById(id);
> +      if (element) {

Personally I prefer bailing early for cases like this (early continue). Less indention and less to keep in your head.

@@ +223,2 @@
>            }
> +        }, 0);

0 is already the default so you don't need to specify it

@@ +446,1 @@
>     * @return {Object[]}                             An object with listener and useCapture,

objects?
Or is it only one object in the array? In that case, why is it returning an array of objects?

@@ +447,5 @@
>     *                                                  describing load handlers the script creates
>     *                                                  when first run.
>     */
>    loadScript(node) {
>      let deferredLoad = [];

deferredLoads?

::: mail/components/extensions/parent/ext-legacy.js
@@ +113,5 @@
> +
> +function getAllWindows() {
> +  let domWindows = [];
> +
> +  function getChildDocShells(parentDocShell) {

I think you should do |let getChildDocShells = function......|

... not to get a complaint for javascript strict mode (about declaring sub functions need to be first thing)
Attachment #8997265 - Flags: feedback+
Any update on reviewing this? Would be good to get it in before next merge. 


(Also, can we perhaps make it bit less chatty? Overlays.jsm spews quite a lot of debug info into the console)
Flags: needinfo?(philipp)
(In reply to Magnus Melin from comment #7)
> Any update on reviewing this? Would be good to get it in before next merge. 
Together with bug 1473178 and bug 1483087.
Comment on attachment 8997265 [details] [diff] [review]
overlay-loader-3.diff

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

I'm a little wary about overlaying into the window after it is loaded. I tried this at first too, but two thing spoke against it:

1) (some) legacy add-ons are not set up for this. Lightning had a bunch of issues, and the only way I could resolve them was to limit the load to before the window load completed.
2) I'd rather not have developers considering this a new feature and investing time in making use of it. If the add-on was legacy, it shouldn't be able to do more than before. If they need more, they need to migrate to WX.


By the way, I found this, which sounds like we can block parsing of the document with a promise. Maybe we can make use of this for the overlay loader: https://searchfox.org/comm-central/source/mozilla/toolkit/components/extensions/ext-browser-content.js#136

I'm going to r+ since you already put in a lot of work, but maybe you want to reconsider. It could be a footgun if allowing this prompts a bunch of other changes in Lightning to accomodate.

::: mail/components/extensions/parent/ext-legacy.js
@@ +113,5 @@
> +
> +function getAllWindows() {
> +  let domWindows = [];
> +
> +  function getChildDocShells(parentDocShell) {

+1 on Magnus comment here, functions need to either go to the top or be defined with let.

@@ +120,5 @@
> +      Ci.nsIDocShell.ENUMERATE_FORWARDS
> +    );
> +
> +    while (docShellEnum.hasMoreElements()) {
> +      let docShell = docShellEnum.getNext();

I think we can do for (let docShell of docShellEnum) now since Kris did those enum changes?

@@ +130,5 @@
> +  }
> +
> +  let windowEnum = Services.ww.getWindowEnumerator();
> +  while (windowEnum.hasMoreElements()) {
> +      let win = windowEnum.getNext();

Same
Attachment #8997265 - Flags: review?(philipp) → review+
Flags: needinfo?(philipp)
(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #9)
> I'm a little wary about overlaying into the window after it is loaded.

We haven't really got a choice. In many cases the extension isn't ready until after the first window is loaded. It did take a bit of fixing for Lightning but 1, I imagine most extensions are nowhere near as complex, and 2, a lot of things I changed in Lightning to make it work needed changing anyway.
Follow-up #1: make the logging quieter.
Attachment #9005916 - Flags: review?(philipp)
Follow-up #2: Fix the "sheet is undefined" error. This is caused by a second extension overlaying while the sheet from the first is still in the document, triggering the bindingattached event early.
Attachment #9005918 - Flags: review?(philipp)
Review comments fixed.
Attachment #8997265 - Attachment is obsolete: true
Comment on attachment 9005916 [details] [diff] [review]
1477956-overlay-loader-logs-1.diff

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

LGTM, r=mkmelin
Attachment #9005916 - Flags: review?(philipp) → review+
Comment on attachment 9005920 [details] [diff] [review]
1477956-overlay-loader-4.diff

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

Remember to carry forward the r+ for the patch so we recognize it's reviewed already.
I think we want this, to at least allow lightning to load consistently.
Attachment #9005920 - Flags: review+
Attachment #9005918 - Flags: review?(philipp) → review+
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/4dfbf61d868b
Change overlay loader so it can run before document load; stop requiring restart to start legacy extension; r=Fallen
https://hg.mozilla.org/comm-central/rev/b67c87c8e4fe
Turn down overlay loader log verbosity by default; r=mkmelin
https://hg.mozilla.org/comm-central/rev/4da4607269c1
Fix "sheet is undefined" message when multiple overlay extensions are enabled; r=Fallen DONTBUILD
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 63.0
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/653f80572dac
Follow-up: Fix linting issue (dangling comma). rs=bustage-fix DONTBUILD
I'm using self-updated TB 63.0a1 x64 (2018-5-23) Daily now, and the included Lightning 65.a1 works fine, except for one problem: If I close and re-start TB, the  Lightning icons on the menu bar have disappeared.  The AddOns Manager shows it installed, though.  In order to get the icons to re-appear for use of the add-on, I have to disable Lightning from TB's AddOns Manager, close TB, re-start it, and then Enable Lightning from the AddOns manager, whereupon TB says it has to re-start, and then the Lightning icons are there again. Since Lightning itself works fine now, is there a procedure I can use that will bring up TB with the Lightning menu-bar icons active? Looks to me like TB is only partially actuating the add-on, not displaying it in the menu bar, on a fresh startup. I'd be glad to help test fixes. Thank you.
In my addon, it seems like the following line has no effect:
addEventListener('messagepane-loaded', org_mozdev_compactHeader.pane.coheOnLoadMsgHeaderPane, true);

Perhaps this is also the reason, why Lightning has still bugs, because it is also used:
https://dxr.mozilla.org/comm-central/source/calendar/lightning/content/imip-bar.js#511
Just auto-updated my TB x64 to 64.0a1; now Lightning's menu-bar icons survive restart, and the add-on works fine.  Great job!
(In reply to Joachim Herb from comment #19)
> In my addon, it seems like the following line has no effect:
> addEventListener('messagepane-loaded',
> org_mozdev_compactHeader.pane.coheOnLoadMsgHeaderPane, true);
> 
> Perhaps this is also the reason, why Lightning has still bugs, because it is
> also used:
> https://dxr.mozilla.org/comm-central/source/calendar/lightning/content/imip-
> bar.js#511

Sorry Joachim, I hadn't seen your message until now. Yes it's possible your listener is added *after* the event fires, so you should check for windows where that's already happened. (Don't remove the listener, it's also possible for it to be added before the event fires.)
(In reply to Geoff Lankow (:darktrojan) from comment #21)
> 
> Sorry Joachim, I hadn't seen your message until now. Yes it's possible your
> listener is added *after* the event fires, so you should check for windows
> where that's already happened. (Don't remove the listener, it's also
> possible for it to be added before the event fires.)
@:darktrojan Thank you for your answer: Could you please point me to an example how I can check if this event has been fired already?
You need to log in before you can comment on or make changes to this bug.