761656 - support Atom Threading Extensions - RFC 4685

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, enhancement)

Description

[Glenn Knickerbocker]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0.4) Gecko/20100101 Firefox/10.0.4
Build ID: 20120420145309

Steps to reproduce:

Some Atom servers such as IBM (Lotus) Connections provide <thr:in-reply-to> elements to specify the threading of entries, per RFC 4685.


Actual results:

Thunderbird does not parse these elements into headers for use in threading entries.


Expected results:

The REF attribute should be parsed into a References: header so that entries can be threaded properly.

Comment 1

[alta88]

Attachment: atomThread.patch

threading can be tested by subcribing to any random blogger feed and also its comments feed, eg:
http://blbooks.blogspot.com/feeds/posts/default
http://blbooks.blogspot.com/feeds/comments/default

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 811729 [details] [diff] [review]
atomThread.patch

Review of attachment 811729 [details] [diff] [review]:
-----------------------------------------------------------------

Nice feature! I'd like to test it when nits below are fixed

::: mailnews/extensions/newsblog/content/FeedItem.js
@@ +25,5 @@
>    enclosures: [],
>    // TO DO: this needs to be localized.
>    title: "(no subject)",
>    author: "anonymous",
> +  inreplyto: "",

camel case plase, inReplyTo

@@ +42,5 @@
>      '    %CONTENT%\n' +
>      '  </body>\n' +
>      '</html>\n',
>  
> +  get MOZKEYS_PAD()

just inline this function, it's small and only used once

@@ +91,5 @@
>      FeedUtils.log.trace("FeedItem.messageID: id - " + this.id);
>      FeedUtils.log.trace("FeedItem.messageID: mURL - " + this.mURL);
>      FeedUtils.log.trace("FeedItem.messageID: title - " + this.title);
>  
> +    return this.normalizeMessageID(messageID);

I don't like how this adds the <> to the messageID. The messageId is the id, the arrows are not part of it, and you can call normalizeMessageID when you need the arrows.

@@ +340,5 @@
> +    // If there is an inreplyto value, create the headers.
> +    let inreplytoHdrsStr;
> +    if (this.inreplyto)
> +      inreplytoHdrsStr = "References: " + this.inreplyto + "\n" +
> +                         "In-Reply-To: " + this.inreplyto + "\n";

let inReplyToHdrsStr = (this.inReplyTo) ? the headers : "" here directly maybe?

The In-Reply-To header is always only one id, not all the ids in the thread. (The message it's a direct reply to.)

Comment 3

[alta88]

(In reply to Magnus Melin from comment #2)
> Comment on attachment 811729 [details] [diff] [review]
> atomThread.patch
> 
> Review of attachment 811729 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice feature! I'd like to test it when nits below are fixed
> 
> ::: mailnews/extensions/newsblog/content/FeedItem.js
> @@ +25,5 @@
> >    enclosures: [],
> >    // TO DO: this needs to be localized.
> >    title: "(no subject)",
> >    author: "anonymous",
> > +  inreplyto: "",
> 
> camel case plase, inReplyTo
> 

sure.

> @@ +42,5 @@
> >      '    %CONTENT%\n' +
> >      '  </body>\n' +
> >      '</html>\n',
> >  
> > +  get MOZKEYS_PAD()
> 
> just inline this function, it's small and only used once
> 

fine.

> @@ +91,5 @@
> >      FeedUtils.log.trace("FeedItem.messageID: id - " + this.id);
> >      FeedUtils.log.trace("FeedItem.messageID: mURL - " + this.mURL);
> >      FeedUtils.log.trace("FeedItem.messageID: title - " + this.title);
> >  
> > +    return this.normalizeMessageID(messageID);
> 
> I don't like how this adds the <> to the messageID. The messageId is the id,
> the arrows are not part of it, and you can call normalizeMessageID when you
> need the arrows.
> 
> @@ +340,5 @@
> > +    // If there is an inreplyto value, create the headers.
> > +    let inreplytoHdrsStr;
> > +    if (this.inreplyto)
> > +      inreplytoHdrsStr = "References: " + this.inreplyto + "\n" +
> > +                         "In-Reply-To: " + this.inreplyto + "\n";
> 
> let inReplyToHdrsStr = (this.inReplyTo) ? the headers : "" here directly
> maybe?
> 

no, normalizeMessageID() with <> is split out like this because it's also used in the parser, which must create the exact same messageId string as the messageID getter for this to work.  and angle brackets are part of the RFC5322 message id syntax.

> The In-Reply-To header is always only one id, not all the ids in the thread.
> (The message it's a direct reply to.)

no, not per https://tools.ietf.org/html/rfc5322.  a message may have 1 or more parents and In-Reply-To is to reflect that.  RFC 4685 provides for multiple <inreplyto> tags which give the parent messagIds.

Comment 4

[alta88]

Attachment: atomThread.patch

address nits.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 813552 [details] [diff] [review]
atomThread.patch

Review of attachment 813552 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! r=mkmelin with the below fixed

::: mailnews/extensions/newsblog/content/FeedItem.js
@@ +85,5 @@
>      FeedUtils.log.trace("FeedItem.messageID: id - " + this.id);
>      FeedUtils.log.trace("FeedItem.messageID: mURL - " + this.mURL);
>      FeedUtils.log.trace("FeedItem.messageID: title - " + this.title);
>  
> +    return this.normalizeMessageID(messageID);

please don't normalize it in the getter. The angel brackets aren't semantically part of the message id. Just add them when needed

Comment 6

[alta88]

Attachment: atomThread.patch

updated, for checkin.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/0f6cf88829f2