Last Comment Bug 792979 - Attachment reminder is broken due to removal of nsIEditorObserver
: Attachment reminder is broken due to removal of nsIEditorObserver
Status: RESOLVED FIXED
: regression
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Thunderbird 18.0
Assigned To: Mike Conley (:mconley) - (Needinfo me!)
:
Mentors:
Depends on:
Blocks: 785091
  Show dependency treegraph
 
Reported: 2012-09-20 13:44 PDT by Mike Conley (:mconley) - (Needinfo me!)
Modified: 2012-09-27 10:59 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch to switch us to using MutationObserver. (3.50 KB, patch)
2012-09-21 12:04 PDT, Mike Conley (:mconley) - (Needinfo me!)
ehsan: review+
Details | Diff | Splinter Review
Re-enable and modernize attachment reminder tests. (12.69 KB, patch)
2012-09-21 12:04 PDT, Mike Conley (:mconley) - (Needinfo me!)
bwinton: review+
Details | Diff | Splinter Review
Patch v2 (3.30 KB, patch)
2012-09-24 07:10 PDT, Mike Conley (:mconley) - (Needinfo me!)
bwinton: review+
neil: feedback+
Details | Diff | Splinter Review
Patch v3 (r+ by bwinton, feedback+ by Neil) (4.35 KB, patch)
2012-09-26 07:04 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Tests v3 - disabled for Windows. (r+ by bwinton) (13.00 KB, patch)
2012-09-26 07:07 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Patch v4 (r+ by bwinton, feedback+ by Neil) (13.14 KB, patch)
2012-09-26 07:11 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Tests v4 - disabled for Windows (r+ by bwinton) (3.35 KB, patch)
2012-09-26 07:12 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review

Description Mike Conley (:mconley) - (Needinfo me!) 2012-09-20 13:44:31 PDT
nsIEditorObserver got taken out in bug 785091 - we need to update ourselves to use the new interface.
Comment 1 :aceman 2012-09-20 13:59:55 PDT
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
Comment 2 :Ehsan Akhgari 2012-09-20 16:02:54 PDT
Sorry.. I thought I checked c-c for usages of this interface... Probably not hard enough...
Comment 3 Jens Müller (:tessarakt) 2012-09-20 22:50:24 PDT
(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?
Comment 4 Mike Conley (:mconley) - (Needinfo me!) 2012-09-21 07:27:46 PDT
(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. :)
Comment 5 :Ehsan Akhgari 2012-09-21 07:52:10 PDT
(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!
Comment 6 Mike Conley (:mconley) - (Needinfo me!) 2012-09-21 09:03:39 PDT
Welp, setEditorObserver is not accessible from JavaScript. So it's not as easy as switching over.

Ehsan and I are investigating other options.
Comment 7 Mike Conley (:mconley) - (Needinfo me!) 2012-09-21 10:15:18 PDT
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?
Comment 8 Mike Conley (:mconley) - (Needinfo me!) 2012-09-21 11:30:40 PDT
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.
Comment 9 Mike Conley (:mconley) - (Needinfo me!) 2012-09-21 12:04:15 PDT
Created attachment 663501 [details] [diff] [review]
Patch to switch us to using MutationObserver.
Comment 10 Mike Conley (:mconley) - (Needinfo me!) 2012-09-21 12:04:51 PDT
Created attachment 663502 [details] [diff] [review]
Re-enable and modernize attachment reminder tests.
Comment 11 Mike Conley (:mconley) - (Needinfo me!) 2012-09-21 12:08:56 PDT
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 12 neil@parkwaycc.co.uk 2012-09-21 12:57:12 PDT
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?
Comment 13 Mike Conley (:mconley) - (Needinfo me!) 2012-09-24 07:10:10 PDT
Created attachment 664059 [details] [diff] [review]
Patch v2
Comment 14 Mike Conley (:mconley) - (Needinfo me!) 2012-09-24 07:11:09 PDT
Comment on attachment 663502 [details] [diff] [review]
Re-enable and modernize attachment reminder tests.

These tests seemed to pass OK on try.
Comment 15 neil@parkwaycc.co.uk 2012-09-24 07:19:28 PDT
Comment on attachment 664059 [details] [diff] [review]
Patch v2

[I can't give r+ to mail/ code of course.]
Comment 16 Mike Conley (:mconley) - (Needinfo me!) 2012-09-24 08:05:37 PDT
Comment on attachment 664059 [details] [diff] [review]
Patch v2

Ah, right - sorry about that, Neil. :) Thanks for the drive-by!
Comment 17 Blake Winton (:bwinton) (:☕️) 2012-09-24 11:07:30 PDT
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.
Comment 18 Blake Winton (:bwinton) (:☕️) 2012-09-24 11:12:45 PDT
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.
Comment 19 Mike Conley (:mconley) - (Needinfo me!) 2012-09-24 12:25:10 PDT
Hm. So I'm seeing the timeout failures on our Windows machines. Investigating...
Comment 20 Mike Conley (:mconley) - (Needinfo me!) 2012-09-25 08:31:23 PDT
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.
Comment 21 Mike Conley (:mconley) - (Needinfo me!) 2012-09-26 07:04:44 PDT
Created attachment 664941 [details] [diff] [review]
Patch v3 (r+ by bwinton, feedback+ by Neil)

Thanks for the review!
Comment 22 Mike Conley (:mconley) - (Needinfo me!) 2012-09-26 07:07:03 PDT
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
Comment 23 Mike Conley (:mconley) - (Needinfo me!) 2012-09-26 07:11:34 PDT
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.
Comment 24 Mike Conley (:mconley) - (Needinfo me!) 2012-09-26 07:12:12 PDT
Created attachment 664948 [details] [diff] [review]
Tests v4 - disabled for Windows (r+ by bwinton)

Corresponding test patch.
Comment 25 Mike Conley (:mconley) - (Needinfo me!) 2012-09-27 06:28:47 PDT
comm-central:

https://hg.mozilla.org/comm-central/rev/afb0368abfe2
https://hg.mozilla.org/comm-central/rev/a5a09c169cee

Note You need to log in before you can comment on or make changes to this bug.