Closed Bug 447842 Opened 16 years ago Closed 15 years ago

Provide support for building a JavaScript message representation

Categories

(MailNews Core :: MIME, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b2

People

(Reporter: asuth, Assigned: asuth)

References

Details

Attachments

(5 files, 3 obsolete files)

It would be useful for JavaScript code to be able to easily get at all of a message's headers, the message body, and its attachments.  Most specifically, gloda (the global database extension) can greatly benefit from this.

As a starting point, I have tried to make the minimal number of C++ changes required to allow such a JS representation to be populated.  The attached patch allows a JavaScript XPCOM nsIMimeEmitter implementation to exist and receive the desired information to build JS object representations.  The idea is that we can change the mechanism to something cleaner / more reliable / more accurate while maintaining the same JS interface.
I've attached the JS side of the equation.  The plan in my mind would be to first commit the required C++ hooks and let the javascript implementation/interface grow/stabilize a little bit as gloda and gloda plugins use it.  Then we migrate it into the core, preferably integrating it into STEEL, which I believe does not yet provide this functionality.
Status: NEW → ASSIGNED
Attachment #331163 - Flags: superreview?(bienvenu)
Attachment #331163 - Flags: review?(bienvenu)
Comment on attachment 331165 [details]
example gloda js emitter

* Processing occurs in two passes.  During the first pass, libmime is parsing
 

What's the second pass?
Whoops, I did rather forget to conclude that thought.  Additional documentation now added:

During the second pass, the libmime object model is traversed, generating
attachment notifications for all leaf nodes.  From our perspective, this
means file attachments and embedded messages (message/rfc822).  We use this
pass to create the attachment objects and properly structure the MIME part
hierarchy.  We extract the 'part name' (ex: 1.2.2.1) from the URL provided
with the attacment and rely on the fact that the attachment notifications
are generated as the result of an in-order traversal of the hierarchy.  We
generate MimeUnknown instances for apparent leaf nodes (nodes for whom
we did not hear about and do not know of any of their children), and
MimeContainer instances for apparent container nodes (nodes for whom we
know about one or more children).
Comment on attachment 331163 [details] [diff] [review]
Provide hooks to allow a javascript mime emitter to be used

very cool, couple nits:

this:

+    const char *remainder;
+    if ((remainder = SkipPrefix(emitter, "js")) &&
+        (*remainder == '\0' || *remainder == '&'))
+    {
+      mOverrideFormat = "application/x-js-mime-message";
+    }

might be more readable like this:

    const char *remainder = SkipPrefix(emitter, "js");
    if (remainder && (!*remainder  || *remainder == '&'))
      mOverrideFormat = "application/x-js-mime-message";

"nested_bodies" isn't quite descriptive enough - maybe notify_nested_bodies?
Attachment #331163 - Flags: superreview?(bienvenu)
Attachment #331163 - Flags: superreview+
Attachment #331163 - Flags: review?(bienvenu)
Attachment #331163 - Flags: review+
Nits fixed.  Carrying forward r=bienvenu,sr=bienvenu.
Attachment #331163 - Attachment is obsolete: true
Attachment #331201 - Flags: superreview+
Attachment #331201 - Flags: review+
Assignee: nobody → bugmail
Status: ASSIGNED → NEW
Keywords: checkin-needed
Checked in: changeset 23:3955ec5709a6
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.1a2
Attached patch make this work for news messages (obsolete) — Splinter Review
This didn't work for news.  The header=blah stuff ended up in the originalspec, which was neither used by the news code (to determine whether to use a converter), or the stream converter code (to determine what control directives were issued).  The news code also failed to propagate the converter reliably to the downstream code.  (Although I may have changed things that did sorta work to just be more explicit in the process of getting things to work.)
Product: Core → MailNews Core
Slightly cleaned up version of the news support bug.  As per the previous comment, this modifies the code that is looking for information to be conveyed by the 'spec' on the URL to check in the originalspec first.  This is required because the news code completely regenerates the spec.  Additionally, the news code was not very diligent about passing the potentially-revised listener around (which makes sense since it was unable to actually create a streamconverter object), and this resolves that.
Attachment #333703 - Flags: superreview?
Attachment #333703 - Flags: review?
I'd like the indexing code to use this.
Blocks: 430614
Given Comment #11 and the fact that gloda needs it, I think we want this in by 3.0b1
Flags: blocking-thunderbird3.0b1+
Flags: blocking-thunderbird3+
So, it strikes me that having both gloda and Vista/OS indexing fetching the same body within the same (likely) short period of time is probably silly.  Some options that come to mind:

1) Have Vista/OS indexing be driven by gloda for message addition purposes when gloda is active.  For everything else, it takes care of itself.
2) Have gloda completely drive the Vista/OS indexing when enabled (Vista/OS indexer doesn't listen for its own events).
3) Have the MIME retrieval guy do some form of caching.
4) Rely on the network layer to cache recently accessed messages?  Not sure this is actually possible/happens.
5) Just rely on offline message storage being enabled to make everything okay.

Option 2 is mentioned because the trick with option 1 is that gloda's indexing queue means that someone only receiving message adds from gloda but moves/deletions from the real source could see the move/delete before the add happens.

Option 3 and 4 can quickly become useless in cases where more than a few new messages show up.

Option 5 is actually somewhat appealing, although option 2 is probably the best choice.

Thoughts?  Also, this functionality will come in with gloda.
Status: NEW → ASSIGNED
(In reply to comment #10)
> Created an attachment (id=333703) [details]
> v2 make this work for news messages

Andrew, this patch has no reviewers assigned (although reviews requested from the wind) is that intentional?
Comment on attachment 333703 [details] [diff] [review]
v2 make this work for news messages

Whoops.  Thanks for catching that!  I was wondering why it was taking Bienvenu so long... ;)
Attachment #333703 - Flags: superreview?(bienvenu)
Attachment #333703 - Flags: superreview?
Attachment #333703 - Flags: review?(bienvenu)
Attachment #333703 - Flags: review?
Comment on attachment 333703 [details] [diff] [review]
v2 make this work for news messages

technically, non-negative rv's are fine, so this should be something like:

+    if (msgUrl && NS_SUCCEEDED(rv = msgUrl->GetOriginalSpec(getter_Copies(originalSpec))))) 

changing all the channel stuff for news worries me a little - do things like opening/saving attachments still work?
Attachment #333703 - Flags: superreview?(bienvenu)
Attachment #333703 - Flags: superreview+
Attachment #333703 - Flags: review?(bienvenu)
Attachment #333703 - Flags: review+
(In reply to comment #16)
> changing all the channel stuff for news worries me a little - do things like
> opening/saving attachments still work?

Since I'm in unit test town, I'll add some unit tests to those ends in news.  (I think we already have some elsewhere, or at least along the same lines...)
Target Milestone: mozilla1.9.1a2 → Thunderbird 3.0b1
Whiteboard: [has reviewed patch, needs unit tests]
Priority: -- → P2
Attachment #331201 - Attachment description: v2 Provide hooks to allow a javascript mime emitter to be used → [checked in] v2 Provide hooks to allow a javascript mime emitter to be used
Attachment #331634 - Attachment is obsolete: true
Whiteboard: [has reviewed patch, needs unit tests] → [patch checked in, need news fix, needs unit tests]
3.0b1 flag is going away in favour of 3.0 flag and milestone combination.
Flags: blocking-thunderbird3.0b1+
The remaining work on this (news and unit tests) is being moved out to beta 2. The work already done here is enough for what we need for beta 1.
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
moving out to beta 2 - what we have is enough for beta 1
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
This patch is friends with this gloda changeset:
https://hg.mozilla.org/users/bugmail_asutherland.org/gloda/rev/942330424dd7

Together, they allow us have things like:
 * mimeMessage.bodyPartsPlain, mimeMessage.bodyPartsHTML: The list of parts with text/plain and text/html bodies, respectively.  (Each part having a .body that is appropriately chock full'o'content.)
 * mimeMessage.bodyPlain, mimeMessage.bodyHTML: Just the concatenation of those bodies.

This also allows it to know the existence and content type of every part; perviously it just inferred the existence of some parts by the existence of attachment leaf nodes.

The patch itself does not make me particularly happy, but you will note that it is scoped to only do things for the emitter=js case, so it at least won't break others.  Also, the reality of the situation is that something along these lines is required if we want to avoid streaming a message twice or making libmime buffer all the content.  libmime was simply not designed for the use-case, and implementing something "cleaner" like a visitor pattern would require extensive and dangerous changes.

The only potential issue in my mind is that this will probably not do the right thing for encapsulated content.  TNEF, encrypted sub-parts, etc.  Of course, I don't think we've figured out how to integrate TNEF yet, and I rather suspect people using encrypted e-mail don't want gloda automatically indexing it anyways.

I think with this change we're basically where we want to be functionality-wise with the JS representation...
Attachment #344860 - Flags: superreview?(bienvenu)
Attachment #344860 - Flags: review?(bienvenu)
Comment on attachment 344860 [details] [diff] [review]
v1 emit additional data so body parts can be delineated

I definitely appreciate the approach of not breaking anyone else :-)

style guidelines say that you need braces around the else clause if you have them around the if clause, so we need braces here:

+            // no content type means text/plain.
+            else if (obj->options->notify_nested_bodies)
+              mimeEmitterAddHeaderField(obj->options, HEADER_CONTENT_TYPE,
+                                        "text/plain");

It looks like most code uses PR_Free to free the value from mime_part_address, so we should do it here as well.

r/sr=me with those nits fixed.
Attachment #344860 - Flags: superreview?(bienvenu)
Attachment #344860 - Flags: superreview+
Attachment #344860 - Flags: review?(bienvenu)
Attachment #344860 - Flags: review+
Nits fixed.  I also realized that I was not consistent with brace usage for the context and fixed that.  Carrying forward r=bienvenu,sr=bienvenu.
Attachment #344860 - Attachment is obsolete: true
Attachment #345088 - Flags: superreview+
Attachment #345088 - Flags: review+
Keywords: checkin-needed
Comment on attachment 345088 [details] [diff] [review]
[checked in] v1.1 emit additional data so body parts can be delineated

http://hg.mozilla.org/comm-central/rev/e104cc413c8e
Attachment #345088 - Attachment description: [needs check-in] v1.1 emit additional data so body parts can be delineated → [checked in] v1.1 emit additional data so body parts can be delineated
anything remaining that we would block b2 for?
Whiteboard: [patch checked in, need news fix, needs unit tests] → [patch checked in, need news fix, needs unit tests][no l10n impact]
needs unit test and newsgroup patch.  That should be spun off into another bug
Whiteboard: [patch checked in, need news fix, needs unit tests][no l10n impact] → [patch checked in, need news fix, needs unit tests][no l10n impact][target slushy]
(In reply to comment #26)
> needs unit test and newsgroup patch.  That should be spun off into another bug

Bug 478167 created to spin the newsgroup patch (and its desired unit test) off.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [patch checked in, need news fix, needs unit tests][no l10n impact][target slushy]
You need to log in before you can comment on or make changes to this bug.