462820 - RFE: Add listeners for attachment processing events in msgHdrViewOverlay.js

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Gerry Healy]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.0.3) Gecko/2008092414 Firefox/3.0.3
Build Identifier: 3.0a3

Add listeners (e.g. onStartAttachments, onEndAttachments) for the start and end of attachment processing methods in msgHdrViewOverlay.js.

A listener can be registered for onStartHeaders and onEndHeaders events, but there is no support for attachment processing. This leads to a race condition when an extension tries to handle attachments after headers have been processed but before attachments. Adding support for attachment processing listeners would allow extensions to handle attachments more consistently i.e. Lookout.


Reproducible: Always

Comment 1

[Gerry Healy]

Attachment: bug 462820 patch

Adds support for onStartAttachments and onEndAttachments listener to messageHeaderSink

Comment 2

[Gerry Healy]

This is my first time submitting a patch so please let me know if there is anything to do differently.

The patch submitted above (id=348399) was made to revision 1102, Thu Nov 13 19:12:10 2008 -0800.

Comment 3

[Gerry Healy]

Comment on attachment 348399 [details] [diff] [review]
bug 462820 patch

I haven't had a reply in a week so changing review requestee to standard8

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 348399 [details] [diff] [review]
bug 462820 patch

Sorry for not looking at this earlier. David knows slightly more about this area than I do, so passing to him.

Comment 5

[Gerry Healy]

If an updated patch is needed let me know.

One question. Should I be patching both or only one of:

- mail/base/content/msgHdrViewOverlay.js
- mailnews/base/resources/content/msgHdrViewOverlay.js

It is the mail version which ends up in chrome/messenger.jar in TB. I assume the other one is SeaMonkey.

Comment 6

[Magnus Melin [:mkmelin]]

Yes that's correct. It's good to patch both (so you help seamonkey keep up) - but not a strict requirement.

Comment 7

[David :Bienvenu]

Comment on attachment 348399 [details] [diff] [review]
bug 462820 patch

since this affects SM as well, you need sr as well. 

I would get rid of the space after and before ( )

+        if ( typeof(gMessageListeners[index].onStartAttachments) == 'function' )

Comment 8

[[e-mail]]

Comment on attachment 348399 [details] [diff] [review]
bug 462820 patch

>     onEndAllAttachments: function()
>     {
>+      for (index in gMessageListeners) {
>+        if ( typeof(gMessageListeners[index].onStartAttachments) == 'function' )
>+          gMessageListeners[index].onStartAttachments();
>+      }
>+
>       // AddSaveAllAttachmentsMenu();
>       if (gCollapsedHeaderViewMode)
>         displayAttachmentsForCollapsedView();
>       else
>         displayAttachmentsForExpandedView();
>+
>+      for (index in gMessageListeners) {
>+        if ( typeof(gMessageListeners[index].onEndAttachments) == 'function' )
>+          gMessageListeners[index].onEndAttachments();
>+      }
>     },
I don't quite understand why you need two notifications here, I'd just use onEndAllAttachments. I also wouldn't bother with the typeof check, just use
if ("onEndAllAttachments" in gMessageListeners[index])

Comment 9

[Gerry Healy]

re: comment 7
No problem spaces are a result of an enforced coding style that I find hard to shake

re: comment8
I added the typeof check to make sure I don't break old listeners, but I believe calling a non-existent function *does not* results in a fatal error so it can removed. Is there a protocol to follow when changing an existing interface?

I added onStartAttachments() for completeness, but it is not needed unless significantly more processing takes place before onEndAttachments(). Will remove it.

I'm in a bit of an internet black hole for a few days, will post updated patch in two or three days.

Comment 10

[David :Bienvenu]

Comment on attachment 348399 [details] [diff] [review]
bug 462820 patch

great, thx, clearing requests on this patch, then.

Comment 11

[[e-mail]]

(In reply to comment #9)
> (In reply to comment #8)
> I added the typeof check to make sure I don't break old listeners
Sure, but I'd prefer it if you used the in check.

Comment 12

[Gerry Healy]

Attachment: patch-bug462820_v2

My mistake msgHdrViewSMIMEOverlay.js is broken by changing the listener interface. I'd forgotten, but that was why I added the check for the existence of the onEndAttachments() listener function, without it javascript dies.

This version of the patch retains the check, but uses the syntax as suggested in comment #8.

Comment 13

[Gerry Healy]

Attachment: patch-bug462820_v3

This version of the patch doesn't contain checks for existence of the onEndAttachments() listener function, but requires modifications to msgHdrViewSMIMEOverlay.js to comply with the new interface.

I prefer patch v2 because it won't break other code that I don't know about.

NOTE: I don't have any SMIME messages handy to test the SMIME code, but I could test that the changes fix the error caused by changing the listener interface.

Comment 14

[David :Bienvenu]

Comment on attachment 358316 [details] [diff] [review]
patch-bug462820_v2

thx, Gerry, sorry for the delay.

Comment 15

[David :Bienvenu]

fix checked in, v2 patch.