Closed Bug 792979 Opened 12 years ago Closed 12 years ago

Attachment reminder is broken due to removal of nsIEditorObserver

Categories

(Thunderbird :: Message Compose Window, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 18.0

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Keywords: regression)

Attachments

(2 files, 5 obsolete files)

nsIEditorObserver got taken out in bug 785091 - we need to update ourselves to use the new interface.
Assignee: nobody → mconley
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.
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?
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Thanks for the review!
Attachment #664059 - Attachment is obsolete: true
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
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
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
Closed: 12 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.