Closed Bug 1472883 Opened Last year Closed Last year

Fixes to make WebExt Lightning run better

Categories

(Calendar :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(1 file, 8 obsolete files)

Attached patch fixups (obsolete) β€” β€” Splinter Review
This is a collection of changes to Lightning to help it deal with being a WebExtension better. Some things that used to work reliably now happen in the wrong order or not at all.

There's several things going on here (in the order they appear in the patch):

* Command controllers aren't set up properly. I'm not sure how this worked before, but they seem to need appending to the commandDispatcher.
* The string bundles aren't added to the document because the overlay loader can't find #calendar_stringbundles to add them to. I think probably the loader should be changed, but I haven't looked at that yet.
* Changed the overlay loader to deal with "style" lines in the manifest, and moved one into the document it applies to.
* Have tabs that can't be restored because the extension isn't ready, restored once the extension is ready.

I'm sure there are more things I'll add to this, and maybe I'll split off the overlay loader stuff into another bug.
Comment on attachment 8989311 [details] [diff] [review]
fixups

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

You didn't ask, but I have some comments to keep you busy :)

::: calendar/base/content/calendar-common-sets.js
@@ +648,5 @@
>          return false;
>      }
>  };
>  
> +document.commandDispatcher.getControllers().appendController(calendarController);

https://searchfox.org/comm-central/source/calendar/base/content/calendar-common-sets.js#788 should be responsible for injecting the controller. It needs to be inserted before the mail controller so that it catches some of the commands common to mail and lightning.

::: calendar/base/content/calendar-common-sets.xul
@@ -10,5 @@
>  ]>
>  
>  <overlay id="calendar-common-sets-overlay"
>           xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> -  <stringbundleset id="calendar_stringbundles">

I came across this in the overlay loader as well. What should be happening here is that the loader doesn't find calendar_stringbundles until the very end, and then resorts to just inserting the element into the window it is being overlaid onto. At least I think. There may be some kinks.

See https://searchfox.org/comm-central/source/mozilla/dom/xul/XULDocument.cpp#2166 for how the loading is done originally, this is what the js code should be doing.

::: calendar/base/content/calendar-views.js
@@ +176,5 @@
>   *
>   * @param aViewType     The type of view to select.
>   */
>  function switchToView(aViewType) {
> +    if (!aViewType) {

I remember this as well. If the view type is null here, something else is wrong. Best to figure out where this is called and make sure it does not pass null.
Attachment #8989311 - Flags: feedback+
Blocks: 1473244
Attached patch 1472883-fixups-1.diff (obsolete) β€” β€” Splinter Review
Today's iteration. I have this "passing" on tryserver, but I'm not totally happy with it yet. The biggest problem I've been dealing with is that we're trying to do some things before the XBL bindings are ready to do them (especially when running mozmill tests as the overheads are slowing things down). I've made the Today Pane initialisation promise-based, and that has solved a lot of issues.

Also there are a few hacks in this patch to avoid the fact that overlays are still being applied by the original code.
Attachment #8989311 - Attachment is obsolete: true
Comment on attachment 8989890 [details] [diff] [review]
1472883-fixups-1.diff

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

::: calendar/base/content/agenda-listbox.js
@@ +13,5 @@
>      this.duration = aDuration;
>      this.multiday = aMultiday;
>  }
>  
> +function XBLReady(testFunc) {

Maybe it would make sense for the binding itself to emit a bindingattached event that you could wait on, along with a flag to say it was constructed.

N.B. I believe this event was in some version of the xbl spec, but isn't implemented. We have some code in calendar that assumes it fires, but I suspect it doesn't.

I'm sure when you say you are unhappy you mean you despise timeouts as much as I do and only resort to them when you are desparate :D
Good idea. I thought of doing something like that and then forgot about it.
Attached patch 1472883-fixups-2.diff (obsolete) β€” β€” Splinter Review
With this patch I eliminate the 1500ms timeout!

I think this is almost ready for use, but I seem to have an intermittent mozmill test (that is, sometimes it passes) on Linux.
Attachment #8989890 - Attachment is obsolete: true
Blocks: 1474123
I've got this (with some changes) passing on everything except Linux debug builds. I'm really hoping the problems there are caused by the old overlay loader still existing and how I counteracted it. Running tests now without either.

If that isn't the issue, then I'm just about out of ideas and may be tempted to land with a perma-orange.
Fixed one problem, revealed another. Good times!
Attached patch 1472883-fixups-3.diff (obsolete) β€” β€” Splinter Review
The removal of overlays from m-c has revealed a few things:

* We've been ignoring xul-overlay processing instructions in top-level documents. Why we even have these is a topic for another day.
* Items added to toolbox palettes aren't being added to toolbars where they should be.
* We're only listening for top level windows opening. Since about:preferences opens in a tab, our overlay provider doesn't see it. (Also, because its address is about:preferences, there are other issues.) I know how it can be fixed, but I'd like to land what I've got so far and come back to it, so I've disabled the test. This does mean the preferences will be broken until then.

Also still to do:

* Adding rules to stylesheets sometimes fails. I'm not sure why, as they should be ready by the time our script runs. This appears to only happen in tests.
Attachment #8990478 - Attachment is obsolete: true
Thanks for bringing this forward.

When fixing Lightning to work with the new loader, I would expect you to also come accross this bugs:

 * bug 758493 (blocks some UI control updates; required for trunk, desired for 6.2; chek it out, wip patch available)
 * bug 1447830 (blocks adding attendees to an event, advise how to fix it in the bug)
 * bug 1101381 (blocks printing; also required for 6.2)

I haven't yet marked them blocking bug 1473244 yet, because they technically are not blocking to build Lighnting again - but to work as before, and that's probably you're reference for this bug.
Attached patch 1472883-fixups-4.diff (obsolete) β€” β€” Splinter Review
I believe I finally have this passing all the CI mozmill tests, except the one I've disabled due to the preferences tab being very busted. I'll file a follow-up for that.

This is just the changes in calendar. I need to spend some more time tidying up the other changes and I think I'll put them in another bug too.
Attachment #8990582 - Attachment is obsolete: true
Attachment #8990667 - Flags: review?(philipp)
Attached patch 1472883-fixups-5.diff (obsolete) β€” β€” Splinter Review
Oops, unintentional whitespace changes.
Attachment #8990667 - Attachment is obsolete: true
Attachment #8990667 - Flags: review?(philipp)
Attachment #8990668 - Flags: review?(philipp)
Attached patch 1472883-fixups-6.diff (obsolete) β€” β€” Splinter Review
… and a piece I forgot about because it's in mail not calendar. I am doing well today. Sorry about the bugspam!
Attachment #8990668 - Attachment is obsolete: true
Attachment #8990668 - Flags: review?(philipp)
Attachment #8990670 - Flags: review?(philipp)
Depends on: 1474248
Comment on attachment 8990670 [details] [diff] [review]
1472883-fixups-6.diff

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

r+wc for the calendar parts. The mail parts look fine to me as well, not sure if someone else wants to review them.

::: calendar/base/content/agenda-listbox.js
@@ +25,5 @@
>  /**
>   * Initialize the agenda listbox, used on window load.
>   */
>  agendaListbox.init = function() {
> +    return new Promise(resolve => {

This could be agendaListbox.init = async function() {
  if (...) {
    ...
  } else {
    await new Promise(resolve => header.addEventListener("bindingattached", resolve, { once: true }));
  }
  ...
}

I'm not quite sure why you need to resolve the promise in the next tick, can you exlplain? Promises should be doing this automatically.

@@ +67,5 @@
> +        window.addEventListener("unload", () => {
> +            Services.prefs.removeObserver("calendar.agendaListbox", prefObserver);
> +            this.uninit();
> +        });
> +    }).catch(Cu.reportError);

This catch would obviously not fit in well with an async function, but then again the calling function should either handle the promise rejection or throw it further up.

@@ +697,5 @@
>  /**
>   * Sets up the calendar for the agenda listbox.
>   */
>  agendaListbox.setupCalendar = function() {
> +    return this.init().then(() => {

Prefer making this an async function and then |await this.init();|

::: calendar/base/content/agenda-listbox.xml
@@ +46,5 @@
>      </content>
>      <implementation>
>        <field name="kCheckbox">null</field>;
>        <constructor><![CDATA[
> +          this.dispatchEvent(new CustomEvent("bindingattached", { bubbles: false }));

Out of curiosity, why not bubble?

::: calendar/base/content/calendar-unifinder-todo.js
@@ +20,5 @@
>   * @param aFilter        The filter name to set.
>   */
>  function updateCalendarToDoUnifinder(aFilter) {
> +    let tree = document.getElementById("unifinder-todo-tree");
> +    if (!tree.mFilter) { // We're not ready for this.

Not the best to wait on an internal variable. Can we expose tree.ready or something that checks mFilter internally?

::: calendar/base/content/today-pane.js
@@ +24,5 @@
>      /**
>       * Load Handler, sets up the today pane controls.
>       */
>      onLoad: function() {
> +        new Promise(resolve => {

Make this an async function and you can use await later on.
Attachment #8990670 - Flags: review?(philipp) → review+
Oh yeah, we don't use promises for stuff any more. Old habits die hard.

(In reply to Philipp Kewisch [:Fallen] from comment #13)
> I'm not quite sure why you need to resolve the promise in the next tick, can
> you exlplain? Promises should be doing this automatically.

At the time, I was trying anything I could think of, and it seemed to help. IIRC the thread firing the event would stop if I tried to resolve from it - probably just XBL weirdness. I don't know if that's actually the case, so I'm trying without.

> @@ +67,5 @@
> > +        window.addEventListener("unload", () => {
> > +            Services.prefs.removeObserver("calendar.agendaListbox", prefObserver);
> > +            this.uninit();
> > +        });
> > +    }).catch(Cu.reportError);
> 
> This catch would obviously not fit in well with an async function, but then
> again the calling function should either handle the promise rejection or
> throw it further up.

Yeah, oops, that wasn't meant to be there still.

> ::: calendar/base/content/agenda-listbox.xml
> @@ +46,5 @@
> >      </content>
> >      <implementation>
> >        <field name="kCheckbox">null</field>;
> >        <constructor><![CDATA[
> > +          this.dispatchEvent(new CustomEvent("bindingattached", { bubbles: false }));
> 
> Out of curiosity, why not bubble?

Matching the "load" event, which doesn't bubble, and also to prevent a listener from accidentally getting the event from the wrong element.

> ::: calendar/base/content/calendar-unifinder-todo.js
> @@ +20,5 @@
> >   * @param aFilter        The filter name to set.
> >   */
> >  function updateCalendarToDoUnifinder(aFilter) {
> > +    let tree = document.getElementById("unifinder-todo-tree");
> > +    if (!tree.mFilter) { // We're not ready for this.
> 
> Not the best to wait on an internal variable. Can we expose tree.ready or
> something that checks mFilter internally?

I think with the timing changes I'm making in bug 1474248 this change is unnecessary now. Removed it.
Attached patch 1472883-fixups-7.diff (obsolete) β€” β€” Splinter Review
Attachment #8990670 - Attachment is obsolete: true
Attached patch 1472883-fixups-8.diff β€” β€” Splinter Review
I think this has changed enough that it needs re-reviewing. The change is almost all due to XBL bindings that aren't ready on time. I can't wait to be rid of XBL.

I've finally got this passing everything on Try now so I really don't want to make more changes.
Attachment #8991249 - Attachment is obsolete: true
Attachment #8992223 - Flags: review?(philipp)
Comment on attachment 8992223 [details] [diff] [review]
1472883-fixups-8.diff

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

Lgtm, I'll keep the nits to myself :) r=philipp
Attachment #8992223 - Flags: review?(philipp) → review+
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/011b4f845383
Fixes to make WebExt Lightning run better; r=philipp
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → 6.5
Comment on attachment 8992223 [details] [diff] [review]
1472883-fixups-8.diff

I'm going to make myself unpopular by whole-sale approving all patches needed to get a TB 62 with calendar out one day.
Attachment #8992223 - Flags: approval-calendar-beta+
You need to log in before you can comment on or make changes to this bug.