The default bug view has changed. See this FAQ.

Attachment reminder is broken due to removal of nsIEditorObserver

RESOLVED FIXED in Thunderbird 18.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

({regression})

Trunk
Thunderbird 18.0
x86
All
regression

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

nsIEditorObserver got taken out in bug 785091 - we need to update ourselves to use the new interface.
Assignee: nobody → mconley

Comment 1

5 years ago
So that people can find this bug, this is the error in Error console upon just opening the compose window:

Error: TypeError: editor.addEditorObserver is not a function
Source file: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Sorry.. I thought I checked c-c for usages of this interface... Probably not hard enough...
(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> Sorry.. I thought I checked c-c for usages of this interface... Probably not
> hard enough...

So maybe there should be automatic tests of patches to mozilla-central against comm-central?
(In reply to Jens Müller from comment #3)
> (In reply to Ehsan Akhgari [:ehsan] from comment #2)
> > Sorry.. I thought I checked c-c for usages of this interface... Probably not
> > hard enough...
> 
> So maybe there should be automatic tests of patches to mozilla-central
> against comm-central?

I believe that's part of why ehsan was campaigning for bug 787208. :)
(In reply to comment #4)
> (In reply to Jens Müller from comment #3)
> > (In reply to Ehsan Akhgari [:ehsan] from comment #2)
> > > Sorry.. I thought I checked c-c for usages of this interface... Probably not
> > > hard enough...
> > 
> > So maybe there should be automatic tests of patches to mozilla-central
> > against comm-central?
> 
> I believe that's part of why ehsan was campaigning for bug 787208. :)

Indeed!
Welp, setEditorObserver is not accessible from JavaScript. So it's not as easy as switching over.

Ehsan and I are investigating other options.
So, I tried using a MutationObserver instead, but mutation notifications don't appear to get fired when things change under the editor's document. :/ (Though it certainly works if I add node under editor...and then a node under that node...etc.)

Other options before we resort to backing 785091 out?
Ok, getting the MutationObserver to listen to the editor's contentDocument did the trick!

I'm writing / fixing up our regression tests for the attachment reminder, since those should have caught this in the first place.
Created attachment 663501 [details] [diff] [review]
Patch to switch us to using MutationObserver.
Created attachment 663502 [details] [diff] [review]
Re-enable and modernize attachment reminder tests.

Updated

5 years ago
Attachment #663501 - Flags: review+
Thanks for the surprise r+, Ehsan. :D

I've pushed the patches to try - the old tests were apparently disabled due to frequent unexplained timeouts. Just going to make sure those are gone before r?'ing it.

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=92a66b5d685d
Comment on attachment 663501 [details] [diff] [review]
Patch to switch us to using MutationObserver.

>+    this._obs = new MutationObserver(gAttachmentNotifier.onMutation
>+                                                        .bind(this));
If you name the method handleMutations, then you just simply pass this itself as the observer, and you don't have to mess around with binding anything. (But why use gAttachmentNotifier.onMutation instead of this.onMutation?)

>-  }
>+  },
Isn't this a strict JS warning?

>-  editor.addEditorObserver(gAttachmentNotifier);
>+  let editorDocument = document.getElementById("content-frame")
>+                               .contentDocument;
>+  gAttachmentNotifier.init(editorDocument);
Why not editor.document?
Created attachment 664059 [details] [diff] [review]
Patch v2
Attachment #663501 - Attachment is obsolete: true
Attachment #664059 - Flags: review?(neil)
Comment on attachment 663502 [details] [diff] [review]
Re-enable and modernize attachment reminder tests.

These tests seemed to pass OK on try.
Attachment #663502 - Flags: review?(bwinton)
Keywords: regression
Comment on attachment 664059 [details] [diff] [review]
Patch v2

[I can't give r+ to mail/ code of course.]
Attachment #664059 - Flags: review?(neil) → feedback+
Comment on attachment 664059 [details] [diff] [review]
Patch v2

Ah, right - sorry about that, Neil. :) Thanks for the drive-by!
Attachment #664059 - Flags: review?(bwinton)
Comment on attachment 663502 [details] [diff] [review]
Re-enable and modernize attachment reminder tests.

>+++ b/mail/test/mozmill/composition/test-attachment-reminder.js
>@@ -1,178 +1,175 @@
>+/**
>+ * Test that the alert appears normally, but now after closing the
>+ * notification.

That seems like a typo.  Should it be "but not" instead?

Other than that, they seem good, so r=me.

Thanks,
Blake.
Attachment #663502 - Flags: review?(bwinton) → review+
Comment on attachment 664059 [details] [diff] [review]
Patch v2

>+++ b/mail/components/compose/content/MsgComposeCommands.js
>@@ -4581,37 +4582,56 @@ function AutoSave()
> const gAttachmentNotifier =
> {
>+  init: function gAN_init(aDocument) {

Should we check that there isn't an existing this._obs, and if there is, disconnect it or throw an error or just skip this method?
(Otherwise the next line will cause us to lose that observer, and leak stuff I think.)

>+    this._obs = new MutationObserver(this);
[snip…]
>+  uninit: function gAN_uninit() {

I think I prefer "shutdown", but it's a weak preference, so if you don't feel like changing it, don't bother.  ;)

Other than that, I think I like it.  r=me, based on a successful test run or two.

Thanks,
Blake.
Attachment #664059 - Flags: review?(bwinton) → review+
Hm. So I'm seeing the timeout failures on our Windows machines. Investigating...
Strange. I've got persistent timeouts on our Windows Mozmill testers.

I don't think I've got the cycles to dig in to why, so I'm going to modify these tests so that we skip the Windows platform. It's better than what we've currently got, anyhow.
Created attachment 664941 [details] [diff] [review]
Patch v3 (r+ by bwinton, feedback+ by Neil)

Thanks for the review!
Attachment #664059 - Attachment is obsolete: true
Created attachment 664942 [details] [diff] [review]
Tests v3 - disabled for Windows. (r+ by bwinton)

Our Windows test machines don't seem to like these tests, even though locally they run fine. I'm not really interested in diving in and figuring out why at this point. I think we're in a better position to detect regressions than we were before seeing as how the tests are enabled on OSX and Linux, so let's roll with that.

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=291b2cc6a154
Attachment #663502 - Attachment is obsolete: true
Created attachment 664947 [details] [diff] [review]
Patch v4 (r+ by bwinton, feedback+ by Neil)

There was a fragment in Patch v3 that should have been in the tests patch. Not a big deal unless we decide to back only one out at some point, and I'm sufficiently paranoid enough to consider that an actual possibility.
Attachment #664941 - Attachment is obsolete: true
Created attachment 664948 [details] [diff] [review]
Tests v4 - disabled for Windows (r+ by bwinton)

Corresponding test patch.
Attachment #664942 - Attachment is obsolete: true
comm-central:

https://hg.mozilla.org/comm-central/rev/afb0368abfe2
https://hg.mozilla.org/comm-central/rev/a5a09c169cee
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
You need to log in before you can comment on or make changes to this bug.