934241 - Feeds should support the received date, ie store a Received trace header

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Comment 1

[alta88]

Attachment: receivedDate.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 826553 [details] [diff] [review]
receivedDate.patch

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

::: mailnews/extensions/newsblog/content/FeedItem.js
@@ +305,5 @@
>        openingLine +
>        'X-Mozilla-Status: 0000\n' +
>        'X-Mozilla-Status2: 00000000\n' +
>        'X-Mozilla-Keys: ' + " ".repeat(80) + '\n' +
> +      'Received: by Localhost; ' + (new Date().toString()) + '\n' +

This won't get you a rfc2822 datestamp (just one fairly similar, that we maybe happen to parse). also maybe lowercase localhost

Comment 3

[alta88]

2822 has been obsoleted by 5322, so i don't know if you mean that strictly.  are you saying Date() is not 5322 compliant?  the code defaults to this elsewhere if there's no valid date given:
http://mxr.mozilla.org/comm-central/source/mailnews/extensions/newsblog/content/feed-parser.js#786

alternatively, what is your suggestion?

Comment 4

[Magnus Melin [:mkmelin]]

new Date().toString() outputs something like 
Wed Nov 06 2013 09:45:27 GMT+0200 (FLE Standard Time)

.. while the rfc2822/rfc5322 date is something like 
Wed, 06 Nov 2013 09:45:27 +0200

I don't know what the easiest way to generate an rfc2822 is, but please fix the dateRescue too.

Comment 5

[alta88]

Attachment: receivedDate.patch

looks like a roll your own necessity.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 828113 [details] [diff] [review]
receivedDate.patch

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

::: mailnews/extensions/newsblog/content/feed-parser.js
@@ +773,2 @@
>  
> +    return FeedUtils.getValidRFC5322Date(ds);

so, any point even bother testing if it's w3c or not?

::: mailnews/extensions/newsblog/content/utils.js
@@ +743,5 @@
> +  getValidRFC5322Date: function(aDateString)
> +  {
> +    let ds = aDateString ? new Date(aDateString) : null;
> +    ds = ds == "Invalid Date" || ds == null ? null : aDateString;
> +    let utcDate = (ds ? new Date(ds) : new Date()).toUTCString();

this is all very hard to read. i'd suggest just something like this (untested)

let d = new Date(aDateString || new Date().getTime());
if (isNaN((ds.getTime())) // invalid date
  d = new Date();

Comment 7

[alta88]

(In reply to Magnus Melin from comment #6)
> Comment on attachment 828113 [details] [diff] [review]
> receivedDate.patch
> 
> Review of attachment 828113 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/extensions/newsblog/content/feed-parser.js
> @@ +773,2 @@
> >  
> > +    return FeedUtils.getValidRFC5322Date(ds);
> 
> so, any point even bother testing if it's w3c or not?
> 

sure, an iso date can be converted by Date() and retain the publishers intent.

> ::: mailnews/extensions/newsblog/content/utils.js
> @@ +743,5 @@
> > +  getValidRFC5322Date: function(aDateString)
> > +  {
> > +    let ds = aDateString ? new Date(aDateString) : null;
> > +    ds = ds == "Invalid Date" || ds == null ? null : aDateString;
> > +    let utcDate = (ds ? new Date(ds) : new Date()).toUTCString();
> 
> this is all very hard to read. i'd suggest just something like this
> (untested)
> 
> let d = new Date(aDateString || new Date().getTime());
> if (isNaN((ds.getTime())) // invalid date
>   d = new Date();

i'll see if i can't name that tune it in fewer notes..

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to alta88 from comment #7)
> sure, an iso date can be converted by Date() and retain the publishers
> intent.

Yes, but if it happens not to be anything date parsing can't handle, we'd still handle that later the same thing as with null, no?

Comment 9

[alta88]

(In reply to Magnus Melin from comment #8)
> (In reply to alta88 from comment #7)
> > sure, an iso date can be converted by Date() and retain the publishers
> > intent.
> 
> Yes, but if it happens not to be anything date parsing can't handle, we'd
> still handle that later the same thing as with null, no?

oh, i see.  yes.

Comment 10

[alta88]

Attachment: receivedDate.patch

updated for comments.

Comment 11

[alta88]

Attachment: receivedDate.patch

updated for comments.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/31b2b8a22a7a

Comment 13

[alta88]

Attachment: receivedDate.patch

argh, date needs to be utc but time needs to be local.

Comment 14

[alta88]

checkin-needed for followup fix, Attachment #829527 [details] [diff].

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/976e600de099