Closed Bug 302387 Opened 16 years ago Closed 13 years ago

rdf_FormatDate() generates incorrect timezone information

Categories

(Core Graveyard :: RDF, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: toddsf, Assigned: ian)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6

Under Windows XP, rdf_FormatDate() generates non-abbreviated timzone
specifications which rdf_ParseDate() cannot read. For example, rdf_FormatDate()
generates dates that look like this:

Fri Jul 15 11:07:05 Pacific Daylight Time 2005 +000000

instead of 

Fri Jul 15 11:07:05 PDT 2005 +000000

Reproducible: Always

Steps to Reproduce:
Use XMLSerializer to generate XML for an RDF datastore containing date
information, such as rdf:Bookmarks.


Actual Results:  
See "Details"

Expected Results:  
See "Details"
I'm seeing this too. It's causing problems in Flock.

The problem seems to be that Windows XP can't provide the abreviated timezone. One solution might be to always save RDF dates in GMT.
This should resolve this bug and not introduce any issues on any other platforms. It does change the behaviour subtly - dates are always stored in their GMT form rather than in locale-specific form, but this will actually fix potential other bugs since the date parsing for RDF only understands half a dozen timezones anyway.
Attachment #223882 - Flags: review?
This should fix bug 274567, too.

Do we really need to explicitly specify GMT in the format string?
Assignee: nobody → ian
Blocks: 274567
Status: UNCONFIRMED → NEW
Component: OS Integration → RDF
Ever confirmed: true
Flags: review?
Product: Firefox → Core
Version: unspecified → Trunk
QA Contact: os.integration → rdf
IMHO, we should not use nsIRDFDate anywhere in any product for a while.
For example, fx's download manager is using that interface.
So my firefox claims I've downloaded a nightly build
the day after tomorrow. (Right click -> "Properties")

This is because the browser selializes local date and parsing it as GMT.
(In reply to comment #3)
> This should fix bug 274567, too.

Ahh yes - looks like it will. It'll also fix behaviour on non-windows platforms in non American/European/Japanese locales, since PR_ParseTimeString doesn't support their timezones: http://lxr.mozilla.org/mozilla1.8/source/nsprpub/pr/src/misc/prtime.c#899
> 
> Do we really need to explicitly specify GMT in the format string?
> 
Hmm, perhaps not - rdf_ParseDate asks PR_ParseTimeString to default to GMT. I will make another patch that omits the GMT string.

Status: NEW → ASSIGNED
(In reply to comment #4)
> IMHO, we should not use nsIRDFDate anywhere in any product for a while.
> For example, fx's download manager is using that interface.
> So my firefox claims I've downloaded a nightly build
> the day after tomorrow. (Right click -> "Properties")
> 
> This is because the browser selializes local date and parsing it as GMT.
> 
RDF dates are important for display in XUL (I think) but there's at least this bug in their storage, perhaps others.
(In reply to comment #6) 
> RDF dates are important for display in XUL (I think) but there's at least this
> bug in their storage, perhaps others.

Isn't it risky to change the behavior of an exisiting interface?
It's already widely used all over the gecko-based products.
As a date recorder, you can use RDF int instead and make the node
remember Unix Time value in seconds/miliseconds, though it's ugly
in terms of object-oriented programings...
(In reply to comment #8)
> (In reply to comment #6) 
>
> Isn't it risky to change the behavior of an exisiting interface?

I don't think this changes the behaviour. This corrects platform-specific and locale-specific bugs in the current implementation. In fact it makes the code match the documentation about the function in comments. The behaviour is backwards-compatible.

> It's already widely used all over the gecko-based products.

It doesn't seem to be used too widely. This bug and change only affect RDF that's serialized to XML, and I think only the download manager (and perhaps the update system) use this inside mozilla products. We use it in Flock for the storage for our favorites (bookmarks) system. This change doesn't affect rdf date literals used by the firefox global history or bookmarks system as they are backed by mork and bookmarks.html respectively, rather than by RDFXML.

> As a date recorder, you can use RDF int instead and make the node
> remember Unix Time value in seconds/miliseconds, though it's ugly
> in terms of object-oriented programings...

Thats actually what we do in other parts of Flock and I'm sure in other parts of the Mozilla products, but for favorites we want to display the RDF in a tree and for that we need to use the date literals (to provide correct display and sorting).
Axel, Torisugari. Any decisions related to this bug? It would make our life easier if we could push it.
Ping? I'm trying to get the ball rolling on getting some of our fixes upstream, and this is one of them.
Attachment #223965 - Flags: review?(l10n)
Comment on attachment 223965 [details] [diff] [review]
This version of the patch doesn't store the string "GMT" in the date string and updates the comment to reflect the format of the string

l10n@mozilla.com isn't doing RDF reviews. Please rerequest review from axel@pike.org
Attachment #223965 - Flags: review?(l10n) → review?
Attachment #223965 - Flags: review? → review?(axel)
Comment on attachment 223965 [details] [diff] [review]
This version of the patch doesn't store the string "GMT" in the date string and updates the comment to reflect the format of the string

r=me, looks good.

I don't have a good tip for a sr.
Attachment #223965 - Flags: review?(axel) → review+
Comment on attachment 223965 [details] [diff] [review]
This version of the patch doesn't store the string "GMT" in the date string and updates the comment to reflect the format of the string

sr=shaver; sorry this sat so long for such a trivial patch.  In general if you don't know who to ask for super-review, you should grab one of the "catch all" cases listed in http://www.mozilla.org/hacking/reviewers.html -- if they're not the right people, they'll bounce to someone better, but then you don't have to track such things yourself.  I'm sorry nobody gave you that advice earlier.
Attachment #223965 - Flags: superreview+
Attachment #223965 - Flags: approval1.9?
Todd you have any unit tests to cover this kind of thing?
Flags: in-testsuite?
Comment on attachment 223965 [details] [diff] [review]
This version of the patch doesn't store the string "GMT" in the date string and updates the comment to reflect the format of the string

a1.9+=damons
Attachment #223965 - Flags: approval1.9? → approval1.9+
Checking in rdfutil.cpp;
/cvsroot/mozilla/rdf/base/src/rdfutil.cpp,v  <--  rdfutil.cpp
new revision: 1.62; previous revision: 1.61

Wohoo! Upstream completed ;)
Thanks ian, axel, shaver, damon and everyone involved :))
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9 M11
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.