1477956 - Further overlay loader improvements

Status: RESOLVED FIXED

(MailNews Core :: XUL Replacements, enhancement)

Description

[Geoff Lankow (:darktrojan)]

Attachment: overlay-loader-1.diff

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.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: overlay-loader-2.diff

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.

Comment 2

[Philipp Kewisch [:Fallen][📱📆]]

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?

Comment 3

[Geoff Lankow (:darktrojan)]

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 4

[[:MakeMyDay]]

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.

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: overlay-loader-3.diff

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.

Comment 6

[Magnus Melin [:mkmelin]]

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)

Comment 7

[Magnus Melin [:mkmelin]]

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)

Comment 8

[Jorg K (CEST = GMT+2)]

(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 9

[Philipp Kewisch [:Fallen][📱📆]]

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

Comment 10

[Geoff Lankow (:darktrojan)]

(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.

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: 1477956-overlay-loader-logs-1.diff

Follow-up #1: make the logging quieter.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1477956-overlay-loader-sheet-undefined-1.diff

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.

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: 1477956-overlay-loader-4.diff

Review comments fixed.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9005916 [details] [diff] [review]
1477956-overlay-loader-logs-1.diff

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

LGTM, r=mkmelin

Comment 15

[Magnus Melin [:mkmelin]]

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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/653f80572dac
Follow-up: Fix linting issue (dangling comma). rs=bustage-fix DONTBUILD

Comment 18

[Helge Skjeveland]

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.

Comment 19

[Joachim Herb]

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

Comment 20

[Helge Skjeveland]

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!

Comment 21

[Geoff Lankow (:darktrojan)]

(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.)

Comment 22

[Joachim Herb]

(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?

Comment 23

[Worcester12345]

(In reply to Joachim Herb from comment #22)

(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?

Comment 24

[Geoff Lankow (:darktrojan)]

It really is a bit late now, but we fixed this same problem in bug 1493034, and you could do the same thing we did.

Comment 25

[Worcester12345]

(In reply to Geoff Lankow (:darktrojan) from comment #24)

It really is a bit late now, but we fixed this same problem in bug 1493034, and you could do the same thing we did.

Just seeing if this needs to be added here still.

Comment 26

[Joachim Herb]

(In reply to Worcester12345 from comment #25)

Just seeing if this needs to be added here still.

I am not sure, what you mean. I actually now use the flag added with this patch in bug 1493034.
headerViewElement.loaded = true;
https://hg.mozilla.org/comm-central/rev/e1b29b3607c4#l2.12

(by the way: is comm-central on https://dxr.mozilla.org/comm-central/ not updated?)

Now I have no idea, if other load events need such a flag also.