304362 - parse W3C-DTF dates correctly

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

feedview's W3C-DTF date parser doesn't always parse dates correctly.  For
example, mozilla.org's news feed's dates show up as "Invalid Dat @ NaN:NaN".

http://www.mozilla.org/news.rdf

feedview should use the W3CToIETFDate conversion function per bug 302121,
comment 44.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: switches to W3CToIETFDate function

Unfortunately because feedview.js is included in content it has no access to
chrome JavaScript, so we can't factor out W3CToIETFDate from FeedItem.js and
use it both here and in Thunderbird's newsblog extension.

So I've just copied the function into feedview.js.  Note that this almost
certainly conflicts with the patch on bug 303848.  Perhaps it makes sense for
Ben to land that (much larger and more complex) patch first.

Comment 2

[Phil Ringnalda (:philor)]

*** Bug 303052 has been marked as a duplicate of this bug. ***

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 192416 [details] [diff] [review]
patch v1: switches to W3CToIETFDate function

Going to attach a new patch.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: unrotted from bug 303848 landing

This patch updates the code for the bug 303848 landing, which changed a lot of
things.  In addition to implementing a much better W3C-DTF to IETF date
conversion function, it also fixes some problems with the code from bug 303848
that cause it not to work, namely:

* feeds don't get initialized because Feed.init() is commented out;
* even if feeds did get initialized, dates wouldn't appear because
initializeDates() calls xmlDate() instead of this._xmlDate() (but of course
with this patch it now calls this._W3CToIETFDate());
* the sidebar disappears as soon as it appears because it looks for a
mixed-case "showMenu" attribute, but the XSLT processor converted it to the
lower-case "showmenu", so init() thinks the user doesn't want to see the menu.

It also fixes two JavaScript strict warnings about invalid commas.

I can supply these as separate patches if you would prefer that.

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 192491 [details] [diff] [review]
patch v1: unrotted from bug 303848 landing

>Index: browser/base/content/feedview.js
>     // Set up the auto-reload timer    
>     setTimeout("RSSPrettyPrint.refresh()", 
>-               this._getIntegerPref("reloadInterval") * 1000);
>-  },
>+               this._getIntegerPref("reloadinterval") * 1000);
>+  }

One thing I noticed - I didn't update the variable name here... that should be
"FeedView.refresh()" not "RSSPrettyPrint.refresh()"

-Ben

Comment 6

[Myk Melez [:myk] [@mykmelez]]

> One thing I noticed - I didn't update the variable name here... that should be
> "FeedView.refresh()" not "RSSPrettyPrint.refresh()"

Indeed, but that's a good thing, since over in browser.js, line 6214 you
hardcoded reloadInterval to "30", so if this code worked it would make all RSS
feeds refresh every 30 seconds, causing many RSS providers to block Firefox users.

http://lxr.mozilla.org/mozilla/source/browser/base/content/browser.js#6214

I'll update the patch to use the browser.feedview.reloadInterval preference (set
to "0", a.k.a. disabled, by default).

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: fixes additional issues

This patch adds back the "showMenu" and "reloadInterval" preferences, removes
the others, sets the "reloadInterval" preference to "0" (disabled) by default,
makes the "reloadInterval" preference be minutes rather than seconds, and makes
reloading work when that preference isn't disabled.

But I see that over in bug 303848 Ben is about to back out his fixes, so
perhaps this is all for naught.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 192570 [details] [diff] [review]
patch v2: fixes additional issues

sr=[e-mail]

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Thanks Ben and Mike!  Fix checked in to the trunk; still waiting for approval to
check it in to the branch.

Checking in browser/base/content/feedview.js;
/cvsroot/mozilla/browser/base/content/feedview.js,v  <--  feedview.js
new revision: 1.2; previous revision: 1.1
done
Checking in browser/base/content/feedview.xsl;
/cvsroot/mozilla/browser/base/content/feedview.xsl,v  <--  feedview.xsl
new revision: 1.2; previous revision: 1.1
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.482; previous revision: 1.481
done
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.72; previous revision: 1.71
done

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Patch checked in to 1.8 branch:

Checking in browser/base/content/feedview.js;
/cvsroot/mozilla/browser/base/content/feedview.js,v  <--  feedview.js
new revision: 1.1.2.1; previous revision: 1.1
done
Checking in browser/base/content/feedview.xsl;
/cvsroot/mozilla/browser/base/content/feedview.xsl,v  <--  feedview.xsl
new revision: 1.1.2.1; previous revision: 1.1
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.479.2.4; previous revision: 1.479.2.3
done
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.71.2.1; previous revision: 1.71
done

Comment 11

[Tanner M. Young [:tmyoung]]

Resetting QA Contact to default.