Closed Bug 695345 Opened 8 years ago Closed 8 years ago

Bring back ISO8601DateUtils.jsm


(Core :: XPConnect, defect, major)

Not set



Tracking Status
firefox8 + verified
firefox9 + verified
firefox10 + verified


(Reporter: jorgev, Unassigned)



(4 keywords, Whiteboard: [qa!])


(1 file)

We need to revert bug 543535 in Firefox 8 and keep this module for at least one more release. According to new information I've received from add-on developers, this change breaks add-ons that potentially reach millions of users. Since this particular add-on is hosted off of AMO, I didn't get this information until today.

We should follow a similar procedure as we did for bug 672063 and bug 675075, where we keep the old interface as a compatibility shim.
Attached file Shim
Here's the file with functions replaced. Everything else (including license header) left the same.
I think we should add warnings in the Error Console that indicate that those calls are deprecated and the Date functions should be used instead.

Also, given bug 693077, I'm not sure if we want the reinstated module to point to the Date functions instead of using its previous quirky behavior.
Nobody who felt that bug 682754 ought to land on mozilla-aurora or mozilla-beta has ever flagged it for approval, so you don't have the option of using Date.parse() until that happens first.

At least a sketchy minimalist test, the first ever for ISO8601DateUtils, which would show that that patch does not actually currently work where it's supposed to work, would be a nice touch.
Surprised noone caught this with a first glance:

  parse: function ISO8601_parse(aDateString) {
    return Date.parse(aDateString);

Should be

  parse: function ISO8601_parse(aDateString) {
    return new Date(Date.parse(aDateString));

Having those both named parse is confusing.

As far as the other comments in here, someone needs to figure 693077 vs. 682754 because I thought 682754 fixed 693077

Jorge, you commented that you wanted 682754 in aurora and beta, but you didn't actually set any flags.

Phil: as far as test goes, if there are unit tests for Date.parse and Date.toISOString, they would cover these cases. I'm unclear as to why we would need additional tests. This is a shim to an exissting API.
Jorge, my bad, flags were set but it got rejected. I just put a comment in for Asa.
Just FYI, the extension I talked about wasn't marked compatible with FF8, and we'll ship a fix at the same time as FF8, so it shouldn't actually break FF8 beta users as I thought.
(In reply to Michael Kaply (mkaply) from comment #4)
>     return new Date(Date.parse(aDateString));

The easier way to spell that is new Date(aDateString).

But, that's the wrong thing to do. If this API was so important, we shouldn't be silently changing it, dropping support for the pastiche of Basic and Extended "20111024T08:08:00+01:00" that it supported for half the planet, the half which was in a + timezone, nor should we be dropping its belief that "+0800" means "whatever the user's timezone is."

But, to reinterate, you _need_ tests. The reason I removed that essential support for returning some datetime, probably not the one the author intended but some datetime, for "-0800" and "-08" was because there were no tests saying that those were intentional parts of the API rather than unknown accidents of the implementation.
You could have just fixed it instead of removing it and having this whole fruitless debate.
As could you.
I did. I paid the fix here, which you just opposed.
I honestly don't care whether ISO8601DateUtils.jsm works exactly as it did before. That's not the point of this bug.

If you remove the API, add-ons have to be fixed, if we map the old API to the new API and a few edge cases break, add-ons have to be fixed (but less add-ons)

So leaving the old JSM and shimming it to the new API with a message saying "Old API doesn't work, use new API and test it" puts us better off than we are now which is completely breaking add-ons that used ISO8601DateUtils.jsm.

All I'm asking for is a shim to map the old API to the new API. Whether it works 100% like the old, I don't care. But if we found some edge cases that we could fix (682754).
> completely breaking add-ons that used ISO8601DateUtils.jsm.

Just FYI, as it stands, these addons break right at the top of the JS file at the "Components.import", which breaks a lot more than just a removed function.
---------------------------------[ Triage Comment ]---------------------------------

We'll want to fix this bug (which is basically backing out bug 543535) for Firefox 8. That means there is no action for bug 682754 for Firefox 8/Beta (though there will likely be action for Firefox 9/Aurora).

We are not willing to take the add-on compat hit for Firefox 8 at this time so we would like to go back to the behavior of Firefox 7 / Release.

Please create a backout patch and land ASAP.

Tracking this for the various versions so it doesn't get lost.
Backed out on beta in

(Pay no attention to the man behind the curtain backing it out and relanding it on aurora - while mentioning a certain amount of dissatisfaction with orders to back out on a tree that I don't have cloned, and would have to clone over a capped and crappy connection, pushing me into the neighborhood of spending cash money to back this out, I said the wrong tree name to my angel, so he backed it out of aurora first, then relanded it there and backed it out of beta.)
Thanks so much philor! I *think* there is some way to create mozilla-beta from mozilla-central and only download the additional delta but I don't have the directions handy. (which I've used several times, and told people to use for cedar and inbound innumerable times, but for some reason it didn't occur to me to use for going down to beta)
fwiw, has a mozilla-beta bundle that can be used to speed up cloning.
Is there something QA can do to verify this fix?
Whiteboard: [qa?]
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #18)
> Is there something QA can do to verify this fix?

Sure: paste

Components.utils.import("resource://gre/modules/ISO8601DateUtils.jsm"); ISO8601DateUtils.parse("20111024T08:08:00+01:00").toUTCString();

into the error console's "Code:" textbox and click "Evaluate" - it should print "Mon Oct 24 2011 07:08:00 GMT" on the Messages tab. Failure would be an "(NS_ERROR_FILE_NOT_FOUND)[nsIXPCComponents_Utils.import]" message on the Errors tab.
Also, "why tests?" - it only took me seven tests intended for this bug to discover that I had the timezone weirdness backward in bug 693077, and that date_parseISOString doesn't actually comply with ES5.
Depends on: 698334
Honestly, I simply don't care anymore. Do whatever you want with this bug.

Your (Phil's) attitude in this bug and others related to it have simply been offensive.

I suggested a simple fix for the problem. What started out as a very simple problem (leave the jsm and simply map it to the new API that developers are being told to use anyway), has turned into a joke.

Do whatever you want. It's your project.
Attachment #567768 - Flags: review+
Attachment #567768 - Flags: review+
Backed out in aurora9 as well:

We think we'll want to shim for mozilla-central but will defer to philor.
Changing to [qa+] for bug verification. Please look at comment #19 for bug verification.
Whiteboard: [qa?] → [qa+]
Verified as fixed on:
Fx 8.0 - RC:
Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Windows NT 6.1; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20100101 Firefox/8.0

Mozilla/5.0 (Windows NT 5.1; rv:9.0a2) Gecko/20111106 Firefox/9.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:9.0a2) Gecko/20111106 Firefox/9.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a2) Gecko/20111106 Firefox/9.0a2

The fix was verified as specified in comment #19.
[Triage Comment]
Let's perform the same back out we did in on mozilla-aurora 10 and m-c. I'm now reopening 543535, requesting a deprecation warning for at least 2 releases, at which point we can move forward with the removal.

Whoever performs the backout, please be warned that the previous backouts may not have applied cleanly because some xpconnect source moved locations (which may affect m-c and mozilla-aurora).
Adding [qa!:8] and [qa!:9] based on comment 24. This remains to be verified on Firefox 10 and 11.
Whiteboard: [qa+] → [qa+], [qa!:9], [qa!:8]
Based on comment 19, this is verified fixed on Firefox 10 Beta1:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0) Gecko/20100101 Firefox/10.0
Whiteboard: [qa+], [qa!:9], [qa!:8] → [qa+], [qa!:9], [qa!:8][qa!:10]
Whiteboard: [qa+], [qa!:9], [qa!:8][qa!:10] → [qa!]
Documentation has been updated and no longer says that ISO8601DateUtils.jsm is gone.
Depends on: 1351079
No longer depends on: 1351079
You need to log in before you can comment on or make changes to this bug.