Last Comment Bug 695345 - Bring back ISO8601DateUtils.jsm
: Bring back ISO8601DateUtils.jsm
Status: RESOLVED FIXED
[qa!]
: addon-compat, dev-doc-complete, verified-aurora, verified-beta
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- major (vote)
: mozilla8
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on: 698334
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-18 08:58 PDT by Jorge Villalobos [:jorgev]
Modified: 2011-12-30 11:54 PST (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
+
verified


Attachments
Shim (1.98 KB, text/plain)
2011-10-18 09:08 PDT, Mike Kaply [:mkaply]
no flags Details

Description Jorge Villalobos [:jorgev] 2011-10-18 08:58:31 PDT
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.
Comment 1 Mike Kaply [:mkaply] 2011-10-18 09:08:37 PDT
Created attachment 567768 [details]
Shim

Here's the file with functions replaced. Everything else (including license header) left the same.
Comment 2 Jorge Villalobos [:jorgev] 2011-10-18 12:15:23 PDT
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.
Comment 3 Phil Ringnalda (:philor) 2011-10-18 12:29:17 PDT
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.
Comment 4 Mike Kaply [:mkaply] 2011-10-21 06:08:50 PDT
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.
Comment 5 Mike Kaply [:mkaply] 2011-10-21 06:11:02 PDT
Jorge, my bad, flags were set but it got rejected. I just put a comment in for Asa.
Comment 6 Ben Bucksch (:BenB) 2011-10-22 03:32:00 PDT
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.
Comment 7 Phil Ringnalda (:philor) 2011-10-24 08:16:12 PDT
(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.
Comment 8 Ben Bucksch (:BenB) 2011-10-24 08:31:08 PDT
You could have just fixed it instead of removing it and having this whole fruitless debate.
Comment 9 Phil Ringnalda (:philor) 2011-10-24 08:40:19 PDT
As could you.
Comment 10 Ben Bucksch (:BenB) 2011-10-24 08:47:47 PDT
I did. I paid the fix here, which you just opposed.
Comment 11 Mike Kaply [:mkaply] 2011-10-24 11:36:48 PDT
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).
Comment 12 Ben Bucksch (:BenB) 2011-10-24 11:39:20 PDT
> 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.
Comment 13 christian 2011-10-25 20:01:29 PDT
---------------------------------[ 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.
Comment 14 Phil Ringnalda (:philor) 2011-10-25 20:48:54 PDT
Backed out on beta in https://hg.mozilla.org/releases/mozilla-beta/rev/b3528a0ab1f7

(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.)
Comment 15 christian 2011-10-25 20:59:07 PDT
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.
Comment 16 Phil Ringnalda (:philor) 2011-10-25 21:05:55 PDT
http://jlebar.com/2011/5/20/Faster_and_smaller_clones_of_branches.html (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)
Comment 17 Marco Bonardo [::mak] 2011-10-26 02:05:23 PDT
fwiw, http://ftp.mozilla.org/pub/mozilla.org/firefox/bundles/ has a mozilla-beta bundle that can be used to speed up cloning.
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-26 09:35:03 PDT
Is there something QA can do to verify this fix?
Comment 19 Phil Ringnalda (:philor) 2011-10-27 23:55:57 PDT
(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.
Comment 20 Phil Ringnalda (:philor) 2011-10-30 23:25:50 PDT
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.
Comment 21 Mike Kaply [:mkaply] 2011-10-31 07:09:23 PDT
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.
Comment 22 christian 2011-11-01 14:37:59 PDT
Backed out in aurora9 as well:

http://hg.mozilla.org/releases/mozilla-aurora/rev/40aad4c14fbd

We think we'll want to shim for mozilla-central but will defer to philor.
Comment 23 juan becerra [:juanb] 2011-11-03 09:32:31 PDT
Changing to [qa+] for bug verification. Please look at comment #19 for bug verification.
Comment 24 Ioana (away) 2011-11-07 07:06:01 PST
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

Aurora:
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.
Comment 25 Alex Keybl [:akeybl] 2011-12-04 11:45:40 PST
[Triage Comment]
Let's perform the same back out we did in https://hg.mozilla.org/releases/mozilla-beta/rev/b3528a0ab1f7 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).
Comment 27 Virgil Dicu [:virgil] [QA] 2011-12-19 02:22:47 PST
Adding [qa!:8] and [qa!:9] based on comment 24. This remains to be verified on Firefox 10 and 11.
Comment 28 Paul Silaghi, QA [:pauly] 2011-12-28 02:19:00 PST
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
Comment 29 Florian Scholz [:fscholz] (MDN) 2011-12-30 11:54:58 PST
Documentation has been updated and no longer says that ISO8601DateUtils.jsm is gone.

Note You need to log in before you can comment on or make changes to this bug.