Add support for IANA time zone names to internationalization API

RESOLVED FIXED in mozilla52

Status

()

Core
JavaScript: Internationalization API
--
enhancement
RESOLVED FIXED
5 years ago
7 months ago

People

(Reporter: Norbert Lindenberg, Assigned: André Bargull)

Tracking

(Blocks: 3 bugs, {dev-doc-complete})

Trunk
mozilla52
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

5 years ago
Edition 1.0 of ECMA-402, ECMAScript Internationalization API Specification, doesn't require support for the IANA time zone names, but many applications do, and the spec allows it in its Conformance clause.
(Reporter)

Updated

5 years ago
Blocks: 837963
(Reporter)

Updated

5 years ago
Depends on: 843748
(Reporter)

Updated

4 years ago
Assignee: mozillabugs → general
Assignee: general → nobody
(Assignee)

Updated

2 years ago
Component: JavaScript Engine → JavaScript: Internationalization API
(Assignee)

Updated

11 months ago
(Assignee)

Comment 1

11 months ago
Created attachment 8786022 [details] [diff] [review]
intl-dtf-timezone.patch

Open questions:
- Do we really need/want to work around http://bugs.icu-project.org/trac/ticket/12044 ?
- timeZoneCache.timeZones (in builtin/Intl.js) is a list of all time zones, that means we keep a list of about ~600 strings alive for the whole runtime. Do you think this will affect memory usage and that we should try to minimize the list size? It's definitely not an option to call intl_availableTimeZones() every time we need the available time zones, because enumerating all time zones is just too slow in ICU. You can measure the time needed in a standalone cpp program, or just use JavaScriptCore [1]. ;-)
```
// Note: ICU enumerates times zones alphabetically.

// JSC: ~1950ms, SM: ~200ms
var d = Date.now();
for (var i = 0; i < 10000; ++i) { Intl.DateTimeFormat(void 0, {timeZone: "Africa/Abidjan"}); }
print(Date.now() - d);

// JSC: ~4950ms, SM: ~250ms
var d = Date.now();
for (var i = 0; i < 10000; ++i) { Intl.DateTimeFormat(void 0, {timeZone: "UTC"}); }
print(Date.now() - d);
```

[1] https://github.com/WebKit/webkit/blob/465398d15bfcb92b9368d9310bd22df0aba59d33/Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp#L132


And I've just noticed that I still need to add "usage" comments to the new functions in builtin/Intl.h.
Assignee: nobody → andrebargull
Attachment #8786022 - Flags: feedback?(jwalden+bmo)
Just out of curiosity, what is the story (or bug number) for updates to the timezone database? The database is updated multiple times a year (2014 went to "j", 2015 to "f") and sometimes new timezone names are added or aliases are created. Will ICU be updated as often as they update timezone definitions? Will the updates be available in new releases only, or will intermediate updates be pushed?

Comment 3

11 months ago
(In reply to Philipp Kewisch [:Fallen] from comment #2)
> Just out of curiosity, what is the story (or bug number) for updates to the
> timezone database? The database is updated multiple times a year (2014 went
> to "j", 2015 to "f") and sometimes new timezone names are added or aliases
> are created. Will ICU be updated as often as they update timezone
> definitions? Will the updates be available in new releases only, or will
> intermediate updates be pushed?

Good question! See http://userguide.icu-project.org/datetime/timezone#TOC-Updating-the-Time-Zone-Data and http://source.icu-project.org/repos/icu/data/trunk/tzdata/icunew/

Comment 4

11 months ago
(In reply to Steven R. Loomis from comment #3)
> (In reply to Philipp Kewisch [:Fallen] from comment #2)
> > Just out of curiosity, what is the story (or bug number) for updates to the
> > timezone database? The database is updated multiple times a year (2014 went
> > to "j", 2015 to "f") and sometimes new timezone names are added or aliases
> > are created. Will ICU be updated as often as they update timezone
> > definitions? Will the updates be available in new releases only, or will
> > intermediate updates be pushed?
> 
> Good question! See
> http://userguide.icu-project.org/datetime/timezone#TOC-Updating-the-Time-
> Zone-Data and
> http://source.icu-project.org/repos/icu/data/trunk/tzdata/icunew/

That explains how to update ICU, but what is Mozilla's process for doing so?  Hopefully shortly after any tzdata release, Mozilla would update accordingly.
The process historically has been to update ICU overall, and not to update the CLDR subcomponent of it.  Given we've kind of decided to strand ourselves on ICU that supports WinXP, that quasi-choice may have to be reevaluated at some point.

Comment 6

11 months ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> The process historically has been to update ICU overall, and not to update
> the CLDR subcomponent of it.  Given we've kind of decided to strand
> ourselves on ICU that supports WinXP, that quasi-choice may have to be
> reevaluated at some point.

It's not clear what's meant by 'the CLDR subcomponent' here. CLDR is a source of data, not really a subcomponent. TZ data (besides localization and metazone mapping) comes from the TZ database, not from CLDR.
Erm.  Huh, I thought time zone info *was* in CLDR.  If it's not, comment 5 is nonsensical.  :-)

I don't know how often we would update tzdata we had to import into our tree for use.  It would depend how much manual labor was involved.  Also on just how much interest there was in its always being 100% up to date.  Also on whether it would at any particular instant be the best use of resources to work on keeping it up-to-date or not -- there's a lot of things we *could* do, after all, but have to make tradeoffs.

Which is a long way of saying I don't really know at this point.

Comment 8

11 months ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> Erm.  Huh, I thought time zone info *was* in CLDR.  If it's not, comment 5
> is nonsensical.  :-)

CLDR has TZ *dependent* data. But ICU pulls tzdata from IANA's tzdata as in the links I gave.

> I don't know how often we would update tzdata we had to import into our tree
> for use.  It would depend how much manual labor was involved.  Also on just
> how much interest there was in its always being 100% up to date.  Also on
> whether it would at any particular instant be the best use of resources to
> work on keeping it up-to-date or not -- there's a lot of things we *could*
> do, after all, but have to make tradeoffs.
> 
> Which is a long way of saying I don't really know at this point.

Not clear. For Node.js for example there are ways to update tzdata without rebuilding the world - https://github.com/nodejs/node/wiki/Intl#updating-timezone-data - but nothing further at this point.
Given my questions have struck some discussion I'm sure it would be better placed in a separate bug, but I think this is something that should definitely be planned for and there should be a responsible team. While just supporting timezone names might not be much of an issue, if I read the Intl spec correctly, there are operations that depend on up to date timezone information. If timezone information is not up to date, this may cause calculated dates to be wrong for periods of time. We've had this situation before with Lightning, I believe it was Russia that changed their DST rules, so various dates were one hour off for a period of time. One possibility would be to make use of kinto to deliver timezone updates, as this could be outside of the release schedule and independent of versions.
(Assignee)

Comment 10

11 months ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> It would depend how much manual labor was involved.

Executing a single shell script (bug 1299189). :-)
(Assignee)

Comment 11

11 months ago
(In reply to Philipp Kewisch [:Fallen] from comment #9)
> While just supporting timezone names might not be much of an issue, if
> I read the Intl spec correctly, there are operations that depend on up
> to date timezone information. If timezone information is not up to date,
> this may cause calculated dates to be wrong for periods of time. 

And it can also lead to user confusion when similar methods (Date.prototype.toString and Date.prototype.toLocaleString) return different results (bug 1246930 comment #0, bug 1147872 comment #0), because the tzdata shipped with Firefox's ICU version has gone stale.
(Assignee)

Comment 12

11 months ago
(In reply to André Bargull from comment #11)
> And it can also lead to user confusion when similar methods
> (Date.prototype.toString and Date.prototype.toLocaleString) return different
> results [...]

This is currently an issue for Asia/Novosibirsk, Asia/Magadan, Asia/Chita, America/Caracas, and others:

~$ TZ=Asia/Novosibirsk mozjs
js> new Date(Date.UTC(2016, 7, 31)).toTimeString()
"07:00:00 GMT+0700"
js> new Date(Date.UTC(2016, 7, 31)).toLocaleTimeString()
"06:00:00"
(Assignee)

Comment 13

11 months ago
Created attachment 8786840 [details] [diff] [review]
intl-dtf-timezone.patch

Updated patch:
- Add missing "usage comments" to builtin/Intl.h
- Remove LocalTZA from vm/SelfHosting.cpp because it's no longer used
Attachment #8786022 - Attachment is obsolete: true
Attachment #8786022 - Flags: feedback?(jwalden+bmo)
Attachment #8786840 - Flags: feedback?(jwalden+bmo)
(In reply to André Bargull from comment #1)
> - Do we really need/want to work around
> http://bugs.icu-project.org/trac/ticket/12044 ?

It does seem reasonable to me to do so.  Possibly not an absolute requirement, but now that it's done, it seems a shame to throw away.

> - timeZoneCache.timeZones (in builtin/Intl.js) is a list of all time zones,
> that means we keep a list of about ~600 strings alive for the whole runtime.
> Do you think this will affect memory usage and that we should try to
> minimize the list size?

Keeping a list alive for the runtime doesn't seem absolutely horrible -- although possibly we might want to store the set in C++ and do operations on it in C++, so that it could be guaranteed single-byte strings and avoid other overhead.

That said, if memory serves, your approach here won't just have the list per runtime, but per *global* that uses Intl DateTimeFormat stuff (whether explicitly or implicitly through Date.prototype.toLocale{,Date,Time}String).

I guess this is a cost we already eat for existing IntlData.js stuff, so possibly this isn't a blocker.  But we should probably investigate and find out just how much it is.  Would you mind testing just how much data this adds to the runtime for zero Intl.DateTimeFormat uses, for one in one compartment, for two across two compartments, so we at least know what cost we're looking at?  It might be that we'll need to move the name-set and canonicalization actions into C++, but we should see whether the current cost is acceptable first.

> enumerating all time zones is just too slow in ICU. You can measure
> the time needed in a standalone cpp program, or just use JavaScriptCore [1].
> ;-)

lol

(In reply to André Bargull from comment #10)
> Executing a single shell script (bug 1299189). :-)

Good -- please fold that into the patch here, then.
Comment on attachment 8786840 [details] [diff] [review]
intl-dtf-timezone.patch

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

This mostly looks pretty nice!

::: js/src/builtin/Intl.cpp
@@ +1938,5 @@
> +    MOZ_ASSERT(args.length() == 0);
> +
> +    RootedObject timeZones(cx, NewDenseEmptyArray(cx));
> +    if (!timeZones)
> +        return false;

I'd prefer you moved this down to just before the for-loop, since that's the earliest you need it.

@@ +1951,5 @@
> +
> +    RootedString jsTimeZone(cx);
> +    RootedValue element(cx);
> +    for (uint32_t index = 0;;) {
> +        const char* timeZone = uenum_next(values, nullptr, &status);

int32_t len;
const char* timeZone = uenum_next(values, &len, &status);

@@ +1959,5 @@
> +        }
> +        if (timeZone == nullptr)
> +            break;
> +
> +        jsTimeZone = JS_NewStringCopyZ(cx, timeZone);

NewStringCopyN<CanGC>(cx, timeZone, size_t(len));

ICU doesn't seem to document that this is always going to be Latin-1.  But if it isn't Latin-1, interpreting as Latin-1 is at least *safe* in terms of not reading past the end, &c.  And someone in the field is likely to report it to us.

@@ +1962,5 @@
> +
> +        jsTimeZone = JS_NewStringCopyZ(cx, timeZone);
> +        if (!jsTimeZone)
> +            return false;
> +        element = StringValue(jsTimeZone);

Mild preference for |element.setString(jsTimeZone);|.

@@ +2003,5 @@
> +    }
> +    if (U_FAILURE(status)) {
> +        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_INTERNAL_INTL_ERROR);
> +        return false;
> +    }

MOZ_ASSERT(size >= 0);

@@ +2005,5 @@
> +        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_INTERNAL_INTL_ERROR);
> +        return false;
> +    }
> +
> +    RootedString str(cx, NewStringCopyN<CanGC>(cx, chars.begin(), size));

You can just use

  JSString* str = NewStringCopyN<CanGC>(cx, chars.begin(), size);

and avoid rooting, as this is being consumed so quickly.

@@ +2039,5 @@
> +    }
> +    if (U_FAILURE(status)) {
> +        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_INTERNAL_INTL_ERROR);
> +        return false;
> +    }

MOZ_ASSERT(size >= 0);

@@ +2041,5 @@
> +        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_INTERNAL_INTL_ERROR);
> +        return false;
> +    }
> +
> +    RootedString str(cx, NewStringCopyN<CanGC>(cx, chars.begin(), size));

JSString* again.

@@ +2132,4 @@
>          return nullptr;
>  
>      AutoStableStringChars timeZoneChars(cx);
> +    JSFlatString* timeZoneFlat = value.toString()->ensureFlat(cx);

Rooted<JSFlatString*>, please.  It's not the clearest thing in the world, I think, that this is rooting-safe as written.

@@ +2137,5 @@
> +        return nullptr;
> +
> +    const UChar* uTimeZone = Char16ToUChar(timeZoneChars.twoByteRange().start().get());
> +    if (!uTimeZone)
> +        return nullptr;

No need to null-check, this is just a cast under the hood and we guaranteed non-null by the initTwoByte check.

@@ +2153,2 @@
>      if (!uPattern)
>          return nullptr;

Hm, I guess this should be removed as well.

::: js/src/builtin/Intl.h
@@ +159,5 @@
>  intl_availableCalendars(JSContext* cx, unsigned argc, Value* vp);
>  
>  /**
> + * Return an array with the names of the supported time zones. The time zone
> + * names are not canonicalized.

I'm having to do a bunch of reading in the latest tzdata to understand this (hardly surprising), particularly as to what "canonicalized" means here.  We should try to provide some minimal amount of information such that the reader at least has tea leaves to read to understand this.  I suggest this comment:

"""
Return an array of the supported time zone names, derived from the IANA time zone database.  https://www.iana.org/time-zones

There are two kinds of IANA time zone names: Zone and Link (denoted as such in database source files).  Zone names are the canonical, preferred name for a time zone, e.g. Asia/Kolkata.  Link names simply refer to target Zone names for their meaning, e.g. Asia/Calcutta targets Asia/Kolkata.  That a name is a Link doesn't *necessarily* reflect a sense of deprecation: some Link names also exist partly for convenience, e.g. UTC and GMT as Link names targeting the Zone name Etc/UTC.

The array of names returned by this function includes both Zone and Link names.
"""

That's enough detail/information, I think, for even a relative novice to make sense of the other functions' documentation here.

::: js/src/builtin/Intl.js
@@ +632,5 @@
> + * Compares two strings, ignoring case differences for ASCII characters.
> + */
> +function equalsIgnoreCaseASCII(str1, str2) {
> +    assert(typeof str1 === "string", "first argument isn't a string value");
> +    assert(typeof str2 === "string", "second argument isn't a string value");

I'd like to see some sort of assertion that both strings are pure-ASCII.  Last I looked, it was functionally impossible to do anything with RegExps and strings and indexing and matching and replacing in an integrity-safe way, so you might have to add an intrinsic to test this.

@@ +664,5 @@
> + * time zone name, its IANA time zone name is returned. Otherwise returns null.
> + *
> + * ES2017 Intl draft rev 4a23f407336d382ed5e3471200c690c9b020b5f3
> + */
> +function ValidateTimeZoneName(timeZone) {

I'd probably name this the spec name, not something different.  That we repurpose the return value to be something better than the spec provides isn't quite enough to justify a different name, IMO -- not when the return values are consistently truthy/falsy in the different cases.

@@ +675,5 @@
> +    }
> +
> +    var index = -1;
> +    for (var i = 0; i < timeZones.length; i++) {
> +        if (equalsIgnoreCaseASCII(timeZone, timeZones[i])) {

A linear search through 627 time zones (the number in ICU trunk from a month or so ago, and I assume the number's not significantly lower in the ICU we use) isn't going to be acceptable.  I think you need to save the available time zones in a Map of {lower,upper}case -> the canonical casing so that existence checks aren't non-constant-time expensive.

@@ +683,5 @@
> +    }
> +
> +    // Return |null| if the input is not a valid time zone name. Also return
> +    // |null| for non-IANA time zones.
> +    if (index < 0 || timeZones[index] in legacyICUTimeZones)

Probably worth filtering out the legacy time zones when creating the Map, rather than here.

@@ +712,5 @@
> +
> +    // Step 3.
> +    if (ianaTimeZone === "Etc/UTC" || ianaTimeZone === "Etc/GMT") {
> +        // ICU/CLDR canonicalizes Etc/UCT to Etc/GMT, but following IANA and
> +        // ECMA-402 to the letter means Etc/UCT is a separate time zone.

This seems like the sort of thing that possibly should be a spec bug fixed in the spec, maybe?  Have we tried raising an issue?

@@ +719,5 @@
> +        else
> +            ianaTimeZone = "UTC";
> +    } else {
> +        // ICU/CLDR doesn't map all names to their new IANA time zone names.
> +        // http://bugs.icu-project.org/trac/ticket/12044

It might be worth adding a note about checking up on this at each ICU upgrade, in intl/update-icu.sh, if we think they'll actually fix this soonish.

::: js/src/builtin/IntlTzData.js
@@ +1,4 @@
> +// Mappings from old time zone names to their new names. Only includes time
> +// zone names which aren't correctly mapped in ICU 55-57.
> +// Derived from IANA Time Zone Database, version tzdata2016f.
> +// http://www.iana.org/time-zones

The contents of this file need to be script-generated for this to have any hope of remaining up-to-date.  Tack more stuff into make_intl_data.py to generate this file, please.

@@ +5,5 @@
> +
> +// Format:
> +// "LinkName": "Target" // ICU-Target [time zone file]"
> +var tzLinkNamesNonICU = {
> +    __proto__: null,

One unusual aspect of self-hosting support is that object literals in global code are cloned (for view by self-hosting code) with null [[Prototype]] -- see CloneObject in SelfHosting.cpp, and the nullptr passed to NewObjectWithGivenProto -- so this is unnecessary.  And in the other one(s) further down.

::: js/src/tests/Intl/DateTimeFormat/format_timeZone.js
@@ +76,5 @@
> +    },
> +    {
> +        timeZone: "Pacific/Kiritimati",
> +        date: Date.UTC(1978, 12-1, 6, 12, 0, 0),
> +        result: "12/6/1978, 1:20:00 AM",

Zone Pacific/Kiritimati -10:29:20 -     LMT     1901
                        -10:40  -       LINT    1979 Oct # Line Is Time
                        -10:00  -	LINT    1995
                         14:00  - 	LINT

UT-10:40:00 wut

@@ +81,5 @@
> +    },
> +    {
> +        timeZone: "Africa/Monrovia",
> +        date: Date.UTC(1971, 12-1, 6, 12, 0, 0),
> +        result: "12/6/1971, 11:15:30 AM",

# In 1972 Liberia was the last country to switch
# from a UTC offset that was not a multiple of 15 or 20 minutes.
# Howse reports that it was in honor of their president's birthday.
# Shank & Pottenger report the date as May 1, whereas Howse reports Jan;
# go with Shanks & Pottenger.
# For Liberia before 1972, Shanks & Pottenger report -0:44, whereas Howse and
# Whitman each report -0:44:30; go with the more precise figure.
# Zone  NAME            GMTOFF  RULES   FORMAT  [UNTIL]
Zone    Africa/Monrovia -0:43:08 -	LMT     1882
                        -0:43:08 -	MMT     1919 Mar # Monrovia Mean Time
                        -0:44:30 -      LRT     1972 May # Liberia Time
                         0:00   -       GMT

UT-00:44:30 wut

I don't want to live on this planet any more.

@@ +86,5 @@
> +    },
> +    {
> +        timeZone: "Asia/Riyadh",
> +        date: Date.UTC(1946, 12-1, 6, 12, 0, 0),
> +        result: "12/6/1946, 3:06:52 PM",

# Time in Saudi Arabia and other countries in the Arabian peninsula was not
# standardized until relatively recently; we don't know when, and possibly it
# has never been made official.  Richard P Hunt, in "Islam city yielding to
# modern times", New York Times (1961-04-09), p 20, wrote that only airlines
# observed standard time, and that people in Jeddah mostly observed quasi-solar
# time, doing so by setting their watches at sunrise to 6 o'clock (or to 12
# o'clock for "Arab" time).
#
# The TZ database cannot represent quasi-solar time; airline time is the best
# we can do.  The 1946 foreign air news digest of the U.S. Civil Aeronautics
# Board (OCLC 42299995) reported that the "... Arabian Government, inaugurated
# a weekly Dhahran-Cairo service, via the Saudi Arabian cities of Riyadh and
# Jidda, on March 14, 1947".  Shanks & Pottenger guessed 1950; go with the
# earlier date.
# 
# Shanks & Pottenger also state that until 1968-05-01 Saudi Arabia had two
# time zones; the other zone, at UTC+4, was in the far eastern part of
# the country.  Ignore this, as it's before our 1970 cutoff.
# 
# Zone  NAME            GMTOFF  RULES   FORMAT  [UNTIL]
Zone    Asia/Riyadh     3:06:52 -	LMT     1947 Mar 14
                        3:00    -       AST

UT+03:06:52 wut wut

Although at least it's not the Netherlands, which from 1909-1937 was legally UT+00:19:32.13 (a fractional offset that tzdata deliberately can't support).

::: js/src/tests/Intl/DateTimeFormat/timeZone.js
@@ +31,5 @@
> +    }
> +}
> +
> +
> +// ECMA-402 doesn't normalize Etc/UCT to UTC.

Possibly this should be fixt upstream?

@@ +116,5 @@
> +    }, RangeError);
> +}
> +assertThrowsInstanceOf(() => {
> +    new Intl.DateTimeFormat(undefined, {timeZone: Symbol()});
> +}, TypeError);

// ToString(<symbol>) throws TypeError (in deliberate contrast with String(<symbol>) which does not).

::: js/src/tests/jstests.list
@@ +34,5 @@
>  skip script test262/ch13/13.2/13.2-15-1.js
>  skip script test262/ch11/11.4/11.4.1/11.4.1-5-a-28-s.js
>  
> +# This test asserts that the resolved time zone for DateTimeFormat is undefined.
> +skip script test262/intl402/ch12/12.3/12.3.3.js

This should say that ECMA-402 1st ed. required |timeZoneName: undefined|, but newer versions have changed to it being the default time zone, referring specifically to the NOTE by Intl.DateTimeFormat.prototype.resolvedOptions().  I can read the test, and the sentence you provide is correct, but it doesn't say *why* skipping is correct.

::: js/src/vm/SelfHosting.cpp
@@ +2475,5 @@
>      JS_FN("intl_Collator_availableLocales", intl_Collator_availableLocales, 0,0),
>      JS_FN("intl_CompareStrings", intl_CompareStrings, 3,0),
>      JS_FN("intl_DateTimeFormat", intl_DateTimeFormat, 2,0),
>      JS_FN("intl_DateTimeFormat_availableLocales", intl_DateTimeFormat_availableLocales, 0,0),
> +    JS_FN("intl_defaultTimeZone", intl_defaultTimeZone, 0,0),

intl_DefaultTimeZone, intl_AvailableTimeZones, intl_CanonicalizeTimeZone seem like the more-common style for this stuff -- please use that instead.
Attachment #8786840 - Flags: feedback?(jwalden+bmo) → feedback-
(Assignee)

Comment 16

11 months ago
Created attachment 8790408 [details] [diff] [review]
intl-dtf-timezone.patch

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15)
> Comment on attachment 8786840 [details] [diff] [review]
> intl-dtf-timezone.patch
> 
> Review of attachment 8786840 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This mostly looks pretty nice!

Thanks! ^^

I've updated the patch per your review comments and included (a slightly
updated) version of the tzdata update script from bug 1299189. And I've also
included the downloaded ICU tzdata files for 2015f (tzdata version used in
ICU56) under intl/tzdata (this is the directory used in the update script) to
act as the base version. I didn't directly update to 2016f because 2016g will
be released shortly, and there's no point in doing two updates in short
succession.

Newer ICU data files are no longer licensed under the ICU license, instead all
ICU content uses the Unicode license. I've changed toolkit/content/license.html
to note that the files in intl/tzdata are Unicode licensed.


> @@ +1951,5 @@
> > +        const char* timeZone = uenum_next(values, nullptr, &status);
> 
> ICU doesn't seem to document that this is always going to be Latin-1.  But
> if it isn't Latin-1, interpreting as Latin-1 is at least *safe* in terms of
> not reading past the end, &c.  And someone in the field is likely to report
> it to us.

uenum_next [1] reports an U_INVARIANT_CONVERSION_ERROR if one of the
characters isn't an "invariant character" [2, 3]. And, unless we start to
support EBCDIC platforms, all invariant characters are ASCII characters.

(Disclaimer: I didn't know any of that when I wrote the code, but it appears
we can rely on uenum_next returning only ASCII characters.)

[1] http://icu-project.org/apiref/icu4c/uenum_8h.html#a99298eabaa3874cdfd9793b207848f68
[2] http://icu-project.org/apiref/icu4c/platform_8h.html#a7fb0b0fede299f9d74973b15e79d3085
[3] https://www-01.ibm.com/software/globalization/cdra/appendix_a.html#fig45


> ::: js/src/builtin/Intl.js
> @@ +632,5 @@
> > + * Compares two strings, ignoring case differences for ASCII characters.
> > + */
> > +function equalsIgnoreCaseASCII(str1, str2) {
> > +    assert(typeof str1 === "string", "first argument isn't a string value");
> > +    assert(typeof str2 === "string", "second argument isn't a string value");
> 
> I'd like to see some sort of assertion that both strings are pure-ASCII. 
> Last I looked, it was functionally impossible to do anything with RegExps
> and strings and indexing and matching and replacing in an integrity-safe
> way, so you might have to add an intrinsic to test this.

I don't quite understand why you want to assert that both strings are pure-ASCII.
Apart from that, `/^[\x00-\x7f]*$/.exec(str1) !== null` should work to test for
ASCII strings. Except we need to write something like:
```
assert(typeof str1 === "string", "first argument isn't a string value");
// No ASCII check for str1, because that string is user-supplied.
assert(typeof str2 === "string", "second argument isn't a string value");
// Ensure str2 is an ASCII string, because we don't trust ICU?!
assert(regexp_exec_no_statics(getAsciiRE(), str2) !== null, "second argument isn't an ASCII string");
```

And somewhere else in Intl.js:
```
function getAsciiRE() {
    return internalIntlRegExps.asciiRE ||
           (internalIntlRegExps.asciiRE = RegExpCreate("^[\x00-\x7f]*$"));
}
```

Later: Doesn't actually matter any more now that the time zones are stored in a map.

> @@ +675,5 @@
> > +    }
> > +
> > +    var index = -1;
> > +    for (var i = 0; i < timeZones.length; i++) {
> > +        if (equalsIgnoreCaseASCII(timeZone, timeZones[i])) {
> 
> A linear search through 627 time zones (the number in ICU trunk from a month
> or so ago, and I assume the number's not significantly lower in the ICU we
> use) isn't going to be acceptable.  I think you need to save the available
> time zones in a Map of {lower,upper}case -> the canonical casing so that
> existence checks aren't non-constant-time expensive.

Sure, I can do that. This will improve the test case from comment #1 from
250ms to 200ms with timeZone="UTC". But it won't matter in practice given
that JSC/V8 need about ~5000ms and Edge ~35000ms (not a typo!) for that test.

> @@ +712,5 @@
> > +
> > +    // Step 3.
> > +    if (ianaTimeZone === "Etc/UTC" || ianaTimeZone === "Etc/GMT") {
> > +        // ICU/CLDR canonicalizes Etc/UCT to Etc/GMT, but following IANA and
> > +        // ECMA-402 to the letter means Etc/UCT is a separate time zone.
> 
> This seems like the sort of thing that possibly should be a spec bug fixed
> in the spec, maybe?  Have we tried raising an issue?

No, there's no spec bug about this issue. I'll put it on the pile of other
ecma402 bugs I still need to file.

> 
> @@ +719,5 @@
> > +        else
> > +            ianaTimeZone = "UTC";
> > +    } else {
> > +        // ICU/CLDR doesn't map all names to their new IANA time zone names.
> > +        // http://bugs.icu-project.org/trac/ticket/12044
> 
> It might be worth adding a note about checking up on this at each ICU
> upgrade, in intl/update-icu.sh, if we think they'll actually fix this
> soonish.

I've added a check to make_intl_data.py which prints a warning if neither
incorrect zones nor incorrect links were found.

> 
> ::: js/src/builtin/IntlTzData.js
> @@ +1,4 @@
> > +// Mappings from old time zone names to their new names. Only includes time
> > +// zone names which aren't correctly mapped in ICU 55-57.
> > +// Derived from IANA Time Zone Database, version tzdata2016f.
> > +// http://www.iana.org/time-zones
> 
> The contents of this file need to be script-generated for this to have any
> hope of remaining up-to-date.  Tack more stuff into make_intl_data.py to
> generate this file, please.

I had to write my first Python script for this task. Woohoo! :-)


> ::: js/src/vm/SelfHosting.cpp
> @@ +2475,5 @@
> >      JS_FN("intl_Collator_availableLocales", intl_Collator_availableLocales, 0,0),
> >      JS_FN("intl_CompareStrings", intl_CompareStrings, 3,0),
> >      JS_FN("intl_DateTimeFormat", intl_DateTimeFormat, 2,0),
> >      JS_FN("intl_DateTimeFormat_availableLocales", intl_DateTimeFormat_availableLocales, 0,0),
> > +    JS_FN("intl_defaultTimeZone", intl_defaultTimeZone, 0,0),
> 
> intl_DefaultTimeZone, intl_AvailableTimeZones, intl_CanonicalizeTimeZone
> seem like the more-common style for this stuff -- please use that instead.

I've kept the lower case names because that matches the conventions for the
Intl functions: Spec methods are in upper case (intl_CompareStrings,
intl_FormatNumber), whereas helper methods use lower case (intl_numberingSystem,
intl_availableCalendars, intl_patternForSkeleton, etc.)
Attachment #8790408 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

11 months ago
Attachment #8786840 - Attachment is obsolete: true
(Assignee)

Comment 17

11 months ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> That said, if memory serves, your approach here won't just have the list per
> runtime, but per *global* that uses Intl DateTimeFormat stuff (whether
> explicitly or implicitly through Date.prototype.toLocale{,Date,Time}String).

Err, yes that's what I meant to say. 


> I guess this is a cost we already eat for existing IntlData.js stuff, so
> possibly this isn't a blocker.  But we should probably investigate and find
> out just how much it is.  Would you mind testing just how much data this
> adds to the runtime for zero Intl.DateTimeFormat uses, for one in one
> compartment, for two across two compartments, so we at least know what cost
> we're looking at?  It might be that we'll need to move the name-set and
> canonicalization actions into C++, but we should see whether the current
> cost is acceptable first.
> 

Hmm, I don't know how to test this. Can you recommend any tools?
about:memory measures strings. How big are these strings? If they're all Latin1 then we can basically assume 1 byte per char and just calculate the size.
(Assignee)

Comment 19

11 months ago
@njn: Thanks for the hint!

I've used this html file to measure the memory impact:
---
<!DOCTYPE html>
<button id="btn1">Load Intl</button>
<script>document.getElementById("btn1").onclick = () => { Intl.DateTimeFormat; };</script>
---

I've opened three tabs, loaded the file in each tab, and then performed the following steps:
1) Open about:memory and measure memory
2) Click on "Load Intl" in the first tab, repeat memory measurement
3) Click on "Load Intl" in the second tab, repeat memory measurement
4) Click on "Load Intl" in the third tab, repeat memory measurement

js-main-runtime/zones/string/gc-heap/latin1 (is this the correct property?) showed the following results:

Rows: Number of times the Intl object was initialized
Columns: Test run No.

With patch (*):
(0) 2.01 MB (02.84%)    2.07 MB (02.52%)
(1) 2.05 MB (02.85%)    2.11 MB (02.84%)
(2) 2.09 MB (02.95%)    2.16 MB (02.75%)
(3) 2.15 MB (02.89%)    2.20 MB (02.81%)

Nightly:
(0) 1.80 MB (02.38%)    1.86 MB (02.47%)
(1) 1.81 MB (02.40%)    1.87 MB (02.38%)
(2) 1.82 MB (02.27%)    1.88 MB (02.39%)
(3) 1.84 MB (02.27%)    1.90 MB (02.39%)

Reduced available time zones to ["US/Pacific", "UTC", "Etc/UTC", "Etc/GMT", "GMT"]:
(0) 1.84 MB (02.39%)    1.86 MB (02.57%)
(1) 1.85 MB (02.56%)    1.88 MB (02.60%)
(2) 1.87 MB (02.42%)    1.89 MB (02.44%)
(3) 1.88 MB (02.52%)    1.90 MB (02.58%)

(*) The initial memory usage is already at 2 MB, because the Intl module was initialized a few times in other compartments.

So we seem to allocate additional 30 KB when Intl methods are used.


(In reply to Nicholas Nethercote [:njn] from comment #18)
> How big are these strings? If they're all Latin1 then we can basically
> assume 1 byte per char and just calculate the size.

Yes, they're all latin1 strings. We're currently using 583 time zone names, with a total length of 8299 characters. We also need 8299 characters to save the property keys for the time zone map (the time zones are keyed by their upper cased name in that map).

js> Object.keys(getSelfHostedValue("toTimeZoneMap")(getSelfHostedValue("intl_availableTimeZones")())).length
583
js> Object.values(getSelfHostedValue("toTimeZoneMap")(getSelfHostedValue("intl_availableTimeZones")())).reduce((a, s) => a + s.length, 0)
8299
js> Object.keys(getSelfHostedValue("toTimeZoneMap")(getSelfHostedValue("intl_availableTimeZones")())).reduce((a, s) => a + s.length, 0)
8299
Comment on attachment 8790408 [details] [diff] [review]
intl-dtf-timezone.patch

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

::: intl/update-tzdata.sh
@@ +101,5 @@
> +# Search for ICU data files.
> +# Little endian data files.
> +icudata_file_le=`find "${icudata_dir}" -type f -name 'icudt*l.dat'`
> +if [ -f "${icudata_file_le}" ]; then
> +  icudata_file_le=`readlink -f "${icudata_file_le}"`

Use --canonicalize for readability, and below.

::: js/src/builtin/Intl.h
@@ +159,5 @@
>  intl_availableCalendars(JSContext* cx, unsigned argc, Value* vp);
>  
>  /**
> + * Return an array of the supported time zone names, derived from the IANA time
> + * zone database (https://www.iana.org/time-zones).

<> if you're going to delimit a link, () are valid in URLs.

::: js/src/builtin/Intl.js
@@ +48,5 @@
> +        if (0x61 <= c && c <= 0x7A) {
> +            result += callFunction(std_String_fromCharCode, null, c & ~0x20);
> +        } else {
> +            result += s[i];
> +        }

result += ternary expression;

@@ +632,5 @@
> +
> +/**
> + * Create a time zone map. Time zones are keyed by their upper case name.
> + */
> +function toTimeZoneMap(timeZones) {

Might as well name this CreateTimeZoneMap and have it do |var timeZones = intl_availableTimeZones();| itself.

@@ +634,5 @@
> + * Create a time zone map. Time zones are keyed by their upper case name.
> + */
> +function toTimeZoneMap(timeZones) {
> +    var map = std_Object_create(null);
> +    for (var i = 0; i < timeZones.length; i++) {

30K is maybe borderline for memory consumption for this, on a per-global basis.  (At least if we don't know of an insane framework footgunning itself because of this, as always.)  Combine it with redoing a bunch of work every time a global does date/time formatting, and it seems like that probably pushes it past the point we should permit for too long.

File a followup to move IntlData.js and IntlTzData.js into a single store in the runtime, please.  Should be fairly easy to fix, but let's get this featurework landed to not have reviewing bog down.

@@ +666,5 @@
> +
> +    var item = timeZones[toASCIIUpperCase(timeZone)];
> +    if (item === undefined)
> +        return null;
> +    return item;

return timeZones[toASCIIUpperCase(timeZone)] || null;

@@ +741,5 @@
> +    // Canonicalize the ICU time zone, e.g. change Etc/UTC to UTC.
> +    var defaultTimeZone = CanonicalizeTimeZoneName(timeZone);
> +
> +    timeZoneCache.icuDefaultTimeZone = icuDefaultTimeZone;
> +    timeZoneCache.defaultTimeZone = defaultTimeZone;

Set these in the opposite order, else oomTest is going to eventually start complaining.

::: js/src/builtin/make_intl_data.py
@@ +240,5 @@
> +        self.filename = filename
> +    def __eq__(self, other):
> +        return hasattr(other, "name") and self.name == other.name
> +    def __cmp__(self, other):
> +        return 0 if self.name == other.name else -1 if self.name < other.name else 1

Bit word-salad there, too many conditions running together.  Split this into a few ifs or something.

@@ +274,5 @@
> +    def _tarlines(self, tar, m):
> +        f = tar.extractfile(m)
> +        for line in codecs.EncodedFile(f, "utf-8"):
> +            yield line
> +        f.close()

with tar.extractfile(m) as f:
  for line in codecs.EncodedFile(f, "utf-8")
    yield line

to avoid manual closing?  Assuming this supports the with-protocol, I hope.

@@ +282,5 @@
> +    if links.viewkeys() & zones:
> +        raise RuntimeError("Links also present in zones: %s" % (links.viewkeys() & zones))
> +
> +    zonenames = [z.name for z in zones]
> +    if not (set(links.viewvalues()) <= set(zonenames)):

Ugh, too many inscrutable operators.  Use named functions instead:

  linkzones = links.viewkeys()
  intersect = linkzones.intersection(zones)
  if intersect:
    raise RE("..." % intersect)

  zonenames = set(z.name for z in zones)
  linktargets = links.viewvalues()
  if not linktargets.issubset(zonenames):
    raise RE("..." % linktargets.difference(zonenames))

@@ +309,5 @@
> +        for line in tzdataDir.readlines(filepath):
> +            if line.startswith("Zone"):
> +                zones.append(createZone(line, filename))
> +            if line.startswith("Link"):
> +                links.append(createLink(line, filename))

Cut out the middlemen:

  zones = set()
  links = {}

and then

  zones.add(createZone(...))

and

  (link, target) = createLink(...)
  links[link] = target

@@ +333,5 @@
> +    (backzones, backlinks) = readIANAFiles(tzdataDir, bkfiles)
> +
> +    # Merge with backzone data.
> +    zones |= backzones
> +    zones.remove(Zone("Factory"))

That's one bizarro Zone.

@@ +450,5 @@
> +    removedTimeZones = [zone for zone in timezones if not isIANATimeZone(zone)]
> +    if removedTimeZones:
> +        raise RuntimeError("Removed zones? %s" % removedTimeZones)
> +
> +    result = missingTimeZones + incorrectMapping

If you use itertools.chain(missingTimeZones, incorrectMapping) here, you can make those two into generator expressions and not compute a large intermediate.  Just good practice to observe, obviously perf not important here.

@@ +454,5 @@
> +    result = missingTimeZones + incorrectMapping
> +
> +    # Remove unnecessary UTC mappings.
> +    utcnames = ["Etc/UTC", "Etc/UCT", "Etc/GMT"]
> +    result = [(zone, alias) for (zone, alias) in result if zone.name not in utcnames]

This should be a generator expression as well.

@@ +472,5 @@
> +    # Links which have a different target in ICU.
> +    incorrectMapping = [(zone, target, aliases[zone]) for (zone, target) in links.iteritems() if isICULink(zone) and target != aliases[zone]]
> +
> +    # Links which are zones in ICU.
> +    incorrectMapping2 = [(zone, target, zone.name) for (zone, target) in links.iteritems() if isICUZone(zone)]

Again, three generator expressions.

@@ +479,5 @@
> +    removedTimeZones = [zone for zone in aliases.iterkeys() if not isIANATimeZone(zone)]
> +    if removedTimeZones:
> +        raise RuntimeError("Removed zones? %s" % removedTimeZones)
> +
> +    result = missingTimeZones + incorrectMapping + incorrectMapping2

chain(missingTimeZones, incorrectMapping, incorrectMapping2)

@@ +483,5 @@
> +    result = missingTimeZones + incorrectMapping + incorrectMapping2
> +
> +    # Remove unnecessary UTC mappings.
> +    utcnames = ["Etc/UTC", "Etc/UCT", "Etc/GMT"]
> +    result = [(zone, target, alias) for (zone, target, alias) in result if not (target in utcnames and alias in utcnames)]

Generator expression.

@@ +515,5 @@
> +        println(u"// ICU tzdata version = %s" % icuversion)
> +        println(u"")
> +
> +        println(u"// Format:")
> +        println(u"// \"ZoneName\": true // ICU-Name [time zone file]")

Switch to single-quotes for all lines that would otherwise have to have escaping in them -- much more readable.

::: js/src/tests/Intl/DateTimeFormat/timeZone_link.js
@@ +22,5 @@
> +    ["America/Lower_Princes", "America/Curacao"],
> +    ["America/Marigot", "America/Port_of_Spain"],
> +    ["America/Mendoza", "America/Argentina/Mendoza"],
> +    ["America/Porto_Acre", "America/Rio_Branco"],
> +    // ["America/Santa_Isabel", "America/Tijuana"],

Why not just remove this?

::: toolkit/content/license.html
@@ +5057,5 @@
>  
>      <h1><a id="unicode"></a>Unicode License</h1>
>  
>      <p>This license applies to certain files in the directories
> +    <span class="path">intl/icu</span>, <span class="path">intl/tzdata</span> and

Serial comma or gtfo.  :-P
Attachment #8790408 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #20)

> ::: intl/update-tzdata.sh
> @@ +101,5 @@
> > +# Search for ICU data files.
> > +# Little endian data files.
> > +icudata_file_le=`find "${icudata_dir}" -type f -name 'icudt*l.dat'`
> > +if [ -f "${icudata_file_le}" ]; then
> > +  icudata_file_le=`readlink -f "${icudata_file_le}"`

Please note that OSX does not support the -f option to readlink
(Assignee)

Comment 22

11 months ago
@Fallen: Thanks for the reminder! I'll just steal the readfile_f function from [1].

[1] https://github.com/anba/es6draft/blob/80d3566f7beeec57f9b96652823ffac5a20a7a8a/src/main/bin/es6draft#L42-L54
(Assignee)

Comment 23

11 months ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #20)
> Comment on attachment 8790408 [details] [diff] [review]
> intl-dtf-timezone.patch
> 
> Review of attachment 8790408 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> File a followup to move IntlData.js and IntlTzData.js into a single store in
> the runtime, please.  Should be fairly easy to fix, but let's get this
> featurework landed to not have reviewing bog down.
> 

Filed bug 1303091.

> 
> ::: js/src/builtin/make_intl_data.py
> @@ +274,5 @@
> > +    def _tarlines(self, tar, m):
> > +        f = tar.extractfile(m)
> > +        for line in codecs.EncodedFile(f, "utf-8"):
> > +            yield line
> > +        f.close()
> 
> with tar.extractfile(m) as f:
>   for line in codecs.EncodedFile(f, "utf-8")
>     yield line
> 
> to avoid manual closing?  Assuming this supports the with-protocol, I hope.

No, it doesn't, I had to use `contextlib.closing` instead.


> ::: js/src/tests/Intl/DateTimeFormat/timeZone_link.js
> @@ +22,5 @@
> > +    ["America/Lower_Princes", "America/Curacao"],
> > +    ["America/Marigot", "America/Port_of_Spain"],
> > +    ["America/Mendoza", "America/Argentina/Mendoza"],
> > +    ["America/Porto_Acre", "America/Rio_Branco"],
> > +    // ["America/Santa_Isabel", "America/Tijuana"],
> 
> Why not just remove this?

Added `["America/Santa_Isabel", "America/Santa_Isabel"]` with a note to re-enable `["America/Santa_Isabel", "America/Tijuana"]` when we support tzdata2016+.


> 
> ::: toolkit/content/license.html
> @@ +5057,5 @@
> >  
> >      <h1><a id="unicode"></a>Unicode License</h1>
> >  
> >      <p>This license applies to certain files in the directories
> > +    <span class="path">intl/icu</span>, <span class="path">intl/tzdata</span> and
> 
> Serial comma or gtfo.  :-P

So embarrassing. :$
(Assignee)

Comment 24

11 months ago
Created attachment 8791683 [details] [diff] [review]
intl-dtf-timezone.patch

Updated patch, carrying r+ from Waldo.
Attachment #8790408 - Attachment is obsolete: true
Attachment #8791683 - Flags: review+
(In reply to André Bargull from comment #22)
> I'll just steal the readfile_f function from [1].

Make sure you get permission from the author!
(Assignee)

Comment 26

11 months ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #25)
> (In reply to André Bargull from comment #22)
> > I'll just steal the readfile_f function from [1].
> 
> Make sure you get permission from the author!

Didn't get the permission from the author, so I needed to use something else. :-p


I've changed my mind and decided to use a simpler approach, so it's easier for other readers to understand the code. (And I didn't want to spend time figuring out from where "the other author" got that code ;-)
(Assignee)

Updated

10 months ago
Duplicate of this bug: 1299189
(Assignee)

Comment 28

10 months ago
The failures in [1] seem to indicate js/src/jsapi-tests/testGCOutOfMemory.cpp needs a higher initial "maxbytes" value for JS_NewContext. Bumping the value from 768 to 1024 resolved this issue [2].

[2] also shows that certain tests calling gczeal(4) (e.g. js/src/jit-test/tests/basic/bug763440.js) are timing out when called with --no-baseline --no-ion (i.e. interpreter only). This happens in createTimeZoneMap() (js/src/builtin/Intl.js) for whatever reason - I'll move this code to C++, hopefully this avoids the timeouts...

GTest are also failing [3] with "MOZ_CRASH(Accessing the Subject Principal without an AutoJSAPI on the stack is forbidden)", I haven't yet found out why that happens...

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=274f4e50a05d
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d4b6467bc92
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=809d9b5b2a4b
Andre, thanks a lot for your work on this! This is super important missing piece of Intl API!

Is your patch going to fix https://github.com/tc39/test262/commit/da4f4385fdf88ff2c8acf036efaaa62f8cd6bd58 or is it something I should report separately?
Flags: needinfo?(andrebargull)
(Assignee)

Comment 30

10 months ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #29)
> Is your patch going to fix
> https://github.com/tc39/test262/commit/
> da4f4385fdf88ff2c8acf036efaaa62f8cd6bd58 or is it something I should report
> separately?

No, the patch only updates time zone name support. Apart from that, the test262 commit is wrong, because ECMA402 doesn't require to canonicalize language extension sequences [1]:
---
The specifications for extensions to BCP 47 language tags, such as RFC 6067, may include canonicalization rules for the extension subtag sequences they define that go beyond the canonicalization rules of RFC 5646 section 4.5. Implementations are allowed, but not required, to apply these additional rules. 
---

RFC 5646 doesn't specify Unicode extension sequences, so it doesn't make sense to refer to RFC 5646 section 4.5 in the test262 commit. Furthermore Unicode extension sequence canonicalization (RFC 6067, 2.1.1 [2]) doesn't specify that subtags within a keyword are updated when deprecated values are used. So I'd say the test262 commit is invalid and should be reverted.


[1] https://tc39.github.io/ecma402/#sec-canonicalizelanguagetag
[2] https://tools.ietf.org/html/rfc6067#section-2.1.1
Flags: needinfo?(andrebargull)
Thanks! Reported back as https://github.com/tc39/test262/issues/743#issuecomment-252066935
(Assignee)

Comment 32

10 months ago
(In reply to André Bargull from comment #28)
> [2] also shows that certain tests calling gczeal(4) (e.g.
> js/src/jit-test/tests/basic/bug763440.js) are timing out when called with
> --no-baseline --no-ion (i.e. interpreter only). This happens in
> createTimeZoneMap() (js/src/builtin/Intl.js) for whatever reason - I'll move
> this code to C++, hopefully this avoids the timeouts...

The gczeal problem was fixed by moving the code to C++ [1], but apparently there's still more to do, because the "cgc" test fails for "OS X 10.7 opt" [1,2]. Except I don't know what I'm supposed to do, because the failure happens when "js/src/jit-test/tests/basic/destructuring-iterator.js" times out, and "destructuring-iterator.js" doesn't actually use Date or Intl methods... :-/


> GTest are also failing [3] with "MOZ_CRASH(Accessing the Subject Principal
> without an AutoJSAPI on the stack is forbidden)", I haven't yet found out
> why that happens...

No longer reproducible with the new try builds [1,2]. \o/


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6196d6f74ab50461f470112fae7e77eb918f29d
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b1bc09ef40df84e4d46cbf64217388f5a8d62bb
(Assignee)

Comment 33

10 months ago
Created attachment 8798737 [details] [diff] [review]
bug837961-part1.patch

Split the build scripts changes from the actual patch, these are safe to push. That should make it easier for me to investigate the cgc problem mentioned in comment #32.
Attachment #8791683 - Attachment is obsolete: true
Attachment #8798737 - Flags: review+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed, leave-open
FYI, I get some lint-looking warnings on Part 1:
js/src/builtin/make_intl_data.py:43:1: F401 'imap' imported but unused
js/src/builtin/make_intl_data.py:586:1: F821 undefined name 'RuntimError'

Comment 35

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95bf06da8e8d
Part 1: Add scripts to update tzdata in ICU data files. r=Waldo
Keywords: checkin-needed

Comment 36

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/95bf06da8e8d
(Assignee)

Comment 37

10 months ago
(In reply to André Bargull from comment #32)
> there's still more to do, because the "cgc" test fails for "OS X 10.7 opt"

Maybe this is just another fallout from bug 1308721...
(Assignee)

Comment 38

10 months ago
Created attachment 8800420 [details] [diff] [review]
bug837961-part2.patch

The remaining patch for this bug, also fixes the lint errors reported in comment 34.

The previously observed cgc failures on OS X 10.7 opt (comment 32) are no longer present, so let's try to land this patch again.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a971609d904970f912846e134178ef16c071a6c1

Carrying r+ from Waldo.
Attachment #8800420 - Flags: review+
(Assignee)

Comment 39

10 months ago
Checkin-needed for part 2 (part 1 has already landed).
Keywords: checkin-needed

Comment 40

10 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2fbdba6f5c7
Part 2: Implement timeZone support for Intl.DateTimeFormat. r=Waldo
Keywords: checkin-needed

Comment 41

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b2fbdba6f5c7
Awesome work, thanks Andre!

I'm testing the nightly build with this patch already and it seems like everything works great :)

Comment 43

10 months ago
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/7452437b3ab5
Backed out changeset b2fbdba6f5c7 for failing jsreftest.html?test=ecma_3/Date/15.9.5.5.js. r=backout a=backout
Backed out from central for jsreftest failures:

https://hg.mozilla.org/mozilla-central/rev/7452437b3ab571b1d60aed4e973d82a1471f72b2

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c83a89713c3a595481ce96e623dcf9189c2b4781
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=37560496&repo=mozilla-inbound

var now = Date.now();
now.toLocaleString('en-US')

returns 1,476,377,577,239 on my German OS. So it seems to ignore the "en-US" but on an English system, it would likely use "." as thousand separator and - now.valueOf() not work because toLocaleString is a string and you get NaN as value.
Flags: needinfo?(andrebargull)
Excerpt from the log:
07:03:09     INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/jsreftest/tests/jsreftest.html?test=ecma_3/Date/15.9.5.5.js | Math.abs(Date.parse(now.toLocaleString('en-US')) - now.valueOf()) < 1000 wrong value  item 3
Duplicate of this bug: 1309825
(Assignee)

Comment 47

10 months ago
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo if bug is intermittent) from comment #44)
> Backed out from central for jsreftest failures:
> 
> https://hg.mozilla.org/mozilla-central/rev/
> 7452437b3ab571b1d60aed4e973d82a1471f72b2
> 
> Push with failures:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=c83a89713c3a595481ce96e623dcf9189c2b4781
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=37560496&repo=mozilla-
> inbound

Is the local time of the test server set to UTC-8? If it's not set to UTC-8, the failures from the log are likely to be expected. 

Quick explanation:
We set TZ environment variable to "PST8PDT" in [1], but this setting doesn't affect ICU. ICU is always querying the OS, and in case of Windows it's using GetTimeZoneInformation, which ignores the TZ environment variable. This difference is easily detectable when calling toTimeString() and toLocaleTimeString(). The former honours the TZ env variable, whereas the latter (because it's using ICU) doesn't:

$ TZ=PST8PDT dist/bin/js.exe
js> d = new Date();
(new Date(1476385231399))
js> d.toTimeString()
"12:00:31 GMT-0700 (PST)"
js> d.toLocaleTimeString()
"15:00:31"

[1] https://dxr.mozilla.org/mozilla-central/rev/22be4ae74653b25186665f22e52a50e7027fd36b/js/src/tests/lib/tests.py#62



> 
> var now = Date.now();
> now.toLocaleString('en-US')
> 
> returns 1,476,377,577,239 on my German OS. So it seems to ignore the "en-US"
> but on an English system, it would likely use "." as thousand separator and
> - now.valueOf() not work because toLocaleString is a string and you get NaN
> as value.

Err, I don't understand. This is the correct output, en-US uses the comma for the thousands separator.
Flags: needinfo?(andrebargull) → needinfo?(aryx.bugmail)
> Err, I don't understand. This is the correct output, en-US uses the comma
> for the thousands separator.
Sorry, got confused.

From the timestamps in the log, they seem to use UTC-8 (afaik AWS default timezone is UTC). But a test which fail in all timezones except one timezone is not acceptable. Even if you skip the other ones, this can cause an unnoticed regression during development. Maybe philor has ideas how to fix this, afaik he interacted with timezone related test issues in the past.
Flags: needinfo?(aryx.bugmail) → needinfo?(philringnalda)
From the timestamps in the log they sure do appear to use UTC-8.

It's currently summer in the northern hemisphere, and PDT is UTC-7.

Some days I hate our infra. Other days I don't connect to the internet. Those seem to be the two available choices.

So was your failure only because it fails on machines which are set to a timezone which observes Daylight Saving time, but has the option to observe it unchecked? That might be an acceptable thing to fail on, while failing locally for every developer who is not in Pacific time is... rather a lot less acceptable.
Flags: needinfo?(philringnalda)
Depends on: 1310051
(Assignee)

Comment 50

10 months ago
I was able to reproduce this issue locally on WinXP & Win7:

Time zone set to "(GMT-08:00) Pacific Time (US & Canada)".

Test case: `new Date().toLocaleString("en-US", {timeZoneName: "short"})`

WinXP/Win7, without patch:
- Automatically adjust clock for DST enabled: "10/14/2016, 10:15:48 AM PDT"
- Automatically adjust clock for DST disabled: "10/14/2016, 9:16:04 AM GMT-8"

WinXP/Win7, with patch:
- Automatically adjust clock for DST enabled: "10/14/2016, 10:15:46 AM PDT"
- Automatically adjust clock for DST disabled: "10/14/2016, 5:16:06 PM GMT"

Win10: "10/14/2016, 10:20:44 AM PDT" for all four configurations....


I think I know what's going on here: ICU reports that the default time zone is "GMT-8", but we reject this time zone name, because it's not an IANA compatible time zone name, and instead use "UTC" (= "GMT") as the time zone. Shouldn't be too hard to fix... :-)

Comment 51

10 months ago
[Tracking Requested - why for this release]:
(Assignee)

Comment 52

10 months ago
(In reply to André Bargull from comment #50)
> Shouldn't be too hard to fix... :-)

LOL, famous last words!

So, *takes breath*, this is what happens. ICU doesn't use its custom time zone name format "GMT-8", but instead reports "Pacific Standard Time". Well okay, then we just need to call ucal_getTimeZoneIDForWindowsID() to map "Pacific Standard Time" to an IANA time zone name?! Well no. It only works as long as the OS locale is English, because on Win7 with German locale, ICU returns "Pacific Normalzeit". Great, a mix-up of the English time zone name ("Pacific") with the localized DST modifier ("Normalzeit")... :-/

My next attempt is as follows: If the default time zone returned from ICU isn't a valid IANA time zone name, instead of directly defaulting to "UTC", compute the UTC offset and use the "Etc/GMT[+-]<offset>" IANA time zone format. Unfortunately this will be a bit confusing for users, because "Etc/GMT[+-]<offset>" seems to use 'POSIX-style signs', instead of 'ISO 8601 style signs', which means the signs are switched, e.g. "Etc/GMT+8" denotes "UTC-8" [1]. 

Btw, Chrome has a related issue because of this ICU bug: When you disable DST, restart Chrome and then evaluate `Intl.DateTimeFormat().resolvedOptions().timeZone`, Chrome reports `undefined` instead of a proper time zone name. 


[1] https://github.com/eggert/tz/blob/6df55775ee950b633c773f4c758b48037bf617e6/etcetera#L36-L42
(Assignee)

Comment 53

10 months ago
(In reply to possessiumness92 from comment #51)
> [Tracking Requested - why for this release]:

Please use https://landfill.bugzilla.org/ for testing bugzilla.
(In reply to André Bargull from comment #52)
> So, *takes breath*, this is what happens. ICU doesn't use its custom time
> zone name format "GMT-8", but instead reports "Pacific Standard Time". Well
> okay, then we just need to call ucal_getTimeZoneIDForWindowsID() to map
> "Pacific Standard Time" to an IANA time zone name?!

Mapping windows timezones to olson timezone names is also not always unambiguous, as you can see in http://unicode.org/repos/cldr/trunk/common/supplemental/windowsZones.xml. It may be sufficient for this case though.

For the record, Lightning has this piece of (fairly old) code that guesses the system timezone which you may be able to get some concepts from. It uses a series of reference dates to match the offsets with those of the timezones. The fallback is to check each timezone for the offsets which is not exactly fast, but maybe it will be of some help to you:

https://dxr.mozilla.org/comm-central/source/calendar/base/src/calTimezoneService.js#299
(Assignee)

Comment 55

9 months ago
(In reply to André Bargull from comment #52)
> Great, a mix-up of the English time zone name
> ("Pacific") with the localized DST modifier ("Normalzeit")... :-/

"Pacific" seems to be a special case, all other time zone names, which aren't mapped correctly, are localized. 


I've tested the updated patch (not yet attached here) with all Windows time zones, and found the following problematic time zones.

Case 1: tzdata2015f, DST not observed

Windows name                Resolved    Expected                Time zone name returned from ICU (Win7+German)

Marquesas Islands,          UTC,        Pacific/Marquesas,      Marquesas Normalzeit
Astrakhan,                  Etc/GMT-4,  Europe/Astrakhan,       Astrachan Normalzeit
Barnaul, Gorno-Altaysk,     Etc/GMT-7,  Asia/Barnaul,           Altai Normalzeit
Tomsk,                      Etc/GMT-7,  Asia/Tomsk,             Tomsk Normalzeit
Pyongyang,                  UTC,        Asia/Pyongyang,         Nordkoreanische Normalzeit
Eucla,                      UTC,        Australia/Eucla,        Zentralaustralische Normalzeit
Chita,                      Etc/GMT-9,  Asia/Chita,             Transbaikal Standardzeit
Sakhalin,                   Etc/GMT-11, Asia/Sakhalin,          Sachalin Normalzeit


Case 2: tzdata2015f, DST enabled

Windows name                Resolved    Expected                Time zone name returned from ICU (Win7+German)

Aleutian Islands,           Etc/GMT+10, America/Adak,           Aleuten Normalzeit
Easter Island,              Etc/GMT+6,  Pacific/Easter,         Osterinsel Normalzeit
Havana,                     Etc/GMT+5,  America/Havana,         Kuba Normalzeit
Saint Pierre and Miquelon,  Etc/GMT+3,  America/Miquelon,       Saint-Pierre Normalzeit
Chisinau (*)                Etc/GMT-2   Europe/Chisinau         Osteuropäische Zeit
Gaza, Hebron,               Etc/GMT-2,  Asia/Hebron,            Westjordanland Gaza Normalzeit
Hovd,                       Etc/GMT-7,  Asia/Hovd,              W. Mongolei Normalzeit
Lord Howe Island,           UTC,        Australia/Lord_Howe,    Lord-Howe Normalzeit
Chatham Islands,            UTC,        Pacific/Chatham,        Chatham-Inseln Normalzeit

(*) "Chisinau" triggers an internal assertion error in ICU when `Intl.DateTimeFormat("en", {timeZoneName: "short"}).format()` is evaluated:
      Assertion failed: 0, file c:/hg/mozilla-inbound/intl/icu/source/common/uinvchar.c, line 208
    This happens because "ä" in "Osteuropäische" isn't an ICU invariant character.


Case 3: tzdata2015f, DST disabled

Windows name                Resolved    Expected                Time zone name returned from ICU (Win7+German)

Alaska,                     Etc/GMT+9,  Etc/GMT+9,              Alaska Normalzeit
Baja California,            Etc/GMT+8,  Etc/GMT+8,              Pacific Normalzeit (Mexiko)
Pacific Time (US & Canada), Etc/GMT+8,  Etc/GMT+8,              Pacific Normalzeit


Case 4: tzdata2015f and tzdata2016g, DST disabled

Windows name                Resolved    Expected                Time zone name returned from ICU (Win7+German)

Newfoundland,               UTC,        N/A,                    Neufundland Normalzeit
Tehran,                     UTC,        N/A,                    Iran Normalzeit
Lord Howe Island,           UTC,        N/A,                    Lord-Howe Normalzeit
Chatham Islands,            UTC,        N/A,                    Chatham-Inseln Normalzeit


Conclusions:
1. We should probably update our tzdata version to 2016g before applying this patch. 
2. Even with 2016g and the new patch, there are still four Windows time zone configurations (Case 4 from above) which aren't supported and instead default to "UTC". I'm not sure it's worth the trouble to support these four configurations.
(Assignee)

Updated

9 months ago
Depends on: 1310733

Comment 56

9 months ago
Hi. Hopefully I can be of some assistance here.  I work at Microsoft and interact regularly with the Windows time zone servicing team, the IANA TZ list, and the CLDR mappings between the two sets of data.

A few points that may be helpful:

1. You really don't want to do your own mappings if you can avoid it.  You should just rely on ucal_getTimeZoneIDForWindowsID and ucal_getWindowsTimeZoneID if you can use ICU 52 or greater.  This uses the CLDR mappings.

2. There are always going to be a few zones that don't reflect in the CLDR and ICU release versions, because zone splits can occur at any time, and CLDR only releases semiannually.  You can see all the current mappings in the dev trunk copy of the CLDR at http://unicode.org/repos/cldr/trunk/common/supplemental/windowsZones.xml

3. In the lists above, you seem to be conflating the Windows ID with the Windows Display Name.  IANA zones only have one name/id ("America/Los_Angeles"), while Windows zones have an ID ("Pacific Standard Time") and a Display Name ("Pacific Time (US & Canada)").  Only the Display Names are localized.  The IDs are always in English, and are guaranteed to not change over time.  Don't try to pass Display Names (in any language) into the ICU methods that expect IDs.  Some will match in English, but most will not.

4. The Windows IDs themselves are somewhat confusing, because they take several different and inconsistent forms.  For example, "Pacific Standard Time", "UTC-11" and "Russia Time Zone 3" are all valid Windows IDs.  Don't try to make too much sense of that - there are historical reasons for the discrepancies, but that's not important.  The important part is that the ID is used to reference a key in the Windows registry, used in APIs, and is never shown to the user. Also, "Standard" in the name of an ID does NOT mean that it's just the standard time.  "Pacific Standard Time" is the ID for the US Pacific time zone, inclusive of both PST and PDT.

5. In the cases listed above, you will find all zones resolved if you are using the latest Windows updates.  In particular, the June 2016 update added all zones necessary for near-parity with TZDB.  Lord Howe, Chatham, etc. all have valid Windows zones now.  The only exceptions are a few of the Antarctica research stations, and some legacy TZDB zones that shouldn't really be in active use.  Then again, keep in mind that due to CLDR release cycles, these just shipped with CLDR 30 and ICU 58.  If you're running older versions, or you don't have at least the June 2016 Windows DST update installed (or newer), then you may find mismatches.

Hope this helps.  Let me know if you have any specific questions.  I'll keep monitoring this issue for updates, and I look forward to testing your implementation.

Cheers!
Matt Johnson
http://codeofmatt.com
@mj1856
(Assignee)

Comment 57

9 months ago
(In reply to Matt Johnson from comment #56)
> 1. You really don't want to do your own mappings if you can avoid it.  You
> should just rely on ucal_getTimeZoneIDForWindowsID and
> ucal_getWindowsTimeZoneID if you can use ICU 52 or greater.  This uses the
> CLDR mappings.

Right now we're only using ucal_openTimeZones, ucal_getCanonicalTimeZoneID and ucal_getDefaultTimeZone. And let ICU handle all the CLDR mapping. 


> 2. There are always going to be a few zones that don't reflect in the CLDR
> and ICU release versions, because zone splits can occur at any time, and
> CLDR only releases semiannually.  You can see all the current mappings in
> the dev trunk copy of the CLDR at
> http://unicode.org/repos/cldr/trunk/common/supplemental/windowsZones.xml

AFAICT the only remaining issue for the patch is caused by
(1) outdated tzdata and CLDR mappings (bug 1310733, updates ICU's time zone files to https://ssl.icu-project.org/repos/icu/data/trunk/tzdata/icunew/2016g/44/)
(2) and this ICU bug: http://bugs.icu-project.org/trac/ticket/12810


> 3. In the lists above, you seem to be conflating the Windows ID with the
> Windows Display Name.  IANA zones only have one name/id
> ("America/Los_Angeles"), while Windows zones have an ID ("Pacific Standard
> Time") and a Display Name ("Pacific Time (US & Canada)").  Only the Display
> Names are localized.  The IDs are always in English, and are guaranteed to
> not change over time.  Don't try to pass Display Names (in any language)
> into the ICU methods that expect IDs.  Some will match in English, but most
> will not.

Yes, I agree that it doesn't make sense to pass the display name to ICU when IDs are expected. Except in this case, the roles are switched and it is ICU which returns the display name. And my code simply didn't handle that case. :-(


> 5. In the cases listed above, you will find all zones resolved if you are
> using the latest Windows updates.  In particular, the June 2016 update added
> all zones necessary for near-parity with TZDB.  Lord Howe, Chatham, etc. all
> have valid Windows zones now.  The only exceptions are a few of the
> Antarctica research stations, and some legacy TZDB zones that shouldn't
> really be in active use.  Then again, keep in mind that due to CLDR release
> cycles, these just shipped with CLDR 30 and ICU 58.  If you're running older
> versions, or you don't have at least the June 2016 Windows DST update
> installed (or newer), then you may find mismatches.

The mismatch seems to be in the other direction. For example Chatham (UTC+12:45) without DST is allowed in Windows, but IANA doesn't have a zone to represent UTC+12:45 year-round. 


Thanks for your input, much appreciated!
(Assignee)

Comment 58

9 months ago
Created attachment 8802251 [details] [diff] [review]
bug837961-part2.patch

Part 2 updated again, carrying r+ from Waldo.

Changes:
- Updated to use tzdata2016g
- Add fallback mechanism in DefaultTimeZone (builtin/Intl.js) when the default time zone is not a valid IANA time zone
Attachment #8800420 - Attachment is obsolete: true
Attachment #8802251 - Flags: review+
(Assignee)

Comment 59

9 months ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=86cf94147997f0676e8a12b2416eadd8dcc32676
Comment on attachment 8802251 [details] [diff] [review]
bug837961-part2.patch

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

I skimmed the Intl.js/Intl.cpp adjustment's at anba's semi-prodding.

::: js/src/builtin/Intl.js
@@ +725,5 @@
> +        // hour offsets.
> +        const msPerHour = 60 * 60 * 1000;
> +        var offset = intl_defaultTimeZoneOffset();
> +        var offsetHours = (offset / msPerHour) | 0;
> +        if (offset === offsetHours * msPerHour) {

Couldn't this be

  const msPerHour = 60 * 60 * 1000;
  var offsetVal = intl_defaultTimeZoneOffset();
  var offset = offsetVal | 0;
  assert(offset === offsetVal,
         "milliseconds offset shouldn't be able to exceed int32_t range");
  var offsetHoursFraction = offset % msPerHour, offsetHours = offset / msPerHour;
  if (offsetHoursFraction === 0) {
    ...
  }

which seems 1) clearer to me that it's trying to only do extra detection for time zones that are offsets by multiples of an hour, and 2) possibly faster in that it doesn't have to perform a division (on a double value) followed by a multiplication, and might benefit from divmod support either in LIR or on the CPU through microcode?  Or maybe I'm misreading this quickly, and the two aren't equivalent -- say so if I am.
Comment on attachment 8802251 [details] [diff] [review]
bug837961-part2.patch

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

I skimmed the Intl.js/Intl.cpp adjustment's at anba's semi-prodding.

::: js/src/builtin/Intl.js
@@ +725,5 @@
> +        // hour offsets.
> +        const msPerHour = 60 * 60 * 1000;
> +        var offset = intl_defaultTimeZoneOffset();
> +        var offsetHours = (offset / msPerHour) | 0;
> +        if (offset === offsetHours * msPerHour) {

Couldn't this be

  const msPerHour = 60 * 60 * 1000;
  var offsetVal = intl_defaultTimeZoneOffset();
  var offset = offsetVal | 0;
  assert(offset === offsetVal,
         "milliseconds offset shouldn't be able to exceed int32_t range");
  var offsetHoursFraction = offset % msPerHour, offsetHours = offset / msPerHour;
  if (offsetHoursFraction === 0) {
    ...
  }

which seems 1) clearer to me that it's trying to only do extra detection for time zones that are offsets by multiples of an hour, and 2) possibly faster in that it doesn't have to perform a division (on a double value) followed by a multiplication, and might benefit from divmod support either in LIR or on the CPU through microcode?  Or maybe I'm misreading this quickly, and the two aren't equivalent -- say so if I am.
Comment on attachment 8802251 [details] [diff] [review]
bug837961-part2.patch

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

Oh, and the other Intl.cpp changes.

I guess I remain leery about having 30K per global still, but if we're fixing fast enough I guess I'm still okay with that temporary state.

::: js/src/builtin/Intl.cpp
@@ +1991,5 @@
> + */
> +static UniqueChars
> +TimeZoneKey(JSContext* cx, const char* timeZone, size_t size)
> +{
> +    UniqueChars timeZoneKey = cx->make_pod_array<char>(size);

Just as cx->make_unique<T>() is idiomatic to return UniquePtr<T>, I argue we should consider make_pod_array similarly idiomatic.  I think you should use |auto| here rather than rename the type.

@@ +1993,5 @@
> +TimeZoneKey(JSContext* cx, const char* timeZone, size_t size)
> +{
> +    UniqueChars timeZoneKey = cx->make_pod_array<char>(size);
> +    if (!timeZoneKey)
> +        return timeZoneKey;

I think you can return nullptr here, which I mildly prefer.

@@ +2001,5 @@
> +    char* chars = timeZoneKey.get();
> +    for (unsigned i = 0; i < size; i++) {
> +        char c = chars[i];
> +        if ('a' <= c && c <= 'z')
> +            chars[i] = c & ~0x20;

Can you add to this block

  MOZ_ASSERT(unicode::ToUpperCase(c) == chars[i]);

from #include "vm/Unicode.h" to verify this did the right thing?

@@ +2050,5 @@
> +        if (!jsTimeZone)
> +            return false;
> +        element.setString(jsTimeZone);
> +
> +        if (!DefineProperty(cx, timeZones, atom->asPropertyName(), element))

I'm a little surprised asPropertyName works there -- I thought a Handle was required.  Still, okay.
(Assignee)

Comment 63

9 months ago
Created attachment 8805224 [details] [diff] [review]
bug837961-part2.patch

Updated part 2 per review comments, carrying r+ from Waldo.
Attachment #8802251 - Attachment is obsolete: true
Attachment #8805224 - Flags: review+
(Assignee)

Updated

9 months ago
Keywords: checkin-needed

Comment 64

9 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ee0568282adc
Part 2: Implement timeZone support for Intl.DateTimeFormat. r=Waldo
Keywords: checkin-needed
Backed out     for "No such property on self-hosted object: offsetVal" failures in Windows XP debug Jit tests:

https://hg.mozilla.org/integration/autoland/rev/5868e557eae14ab7ac1a3fc89fe50b71a8665b55

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=5816332&repo=autoland

23:24:41     INFO -  TEST-PASS | tests\jit-test\jit-test\tests\TypedObject\jit-read-unsized.js | Success (code 0, args "--no-baseline --no-ion")
23:24:41     INFO -  C:\slave\test\build\tests\jit-test\jit-test\tests\TypedObject\jit-write-references-2.js:1:43 Error: No such property on self-hosted object: offsetVal
23:24:41     INFO -  Stack:
23:24:41     INFO -    @C:\slave\test\build\tests\jit-test\jit-test\tests\TypedObject\jit-write-references-2.js:1:43
23:24:41     INFO -  Exit code: 3
23:24:41     INFO -  FAIL - TypedObject\jit-write-references-2.js
23:24:41  WARNING -  TEST-UNEXPECTED-FAIL | tests\jit-test\jit-test\tests\TypedObject\jit-write-references-2.js | C:\slave\test\build\tests\jit-test\jit-test\tests\TypedObject\jit-write-references-2.js:1:43 Error: No such property on self-hosted object: offsetVal (code 3, args "")

offsetVal is used here: https://hg.mozilla.org/integration/autoland/rev/ee0568282adc0d6cddf74c3e635568145d5a987d#l4.137
Flags: needinfo?(andrebargull)
(Assignee)

Comment 66

9 months ago
Created attachment 8805505 [details] [diff] [review]
bug837961-part2.patch

Updated - Now with fewer typos. :-/
Attachment #8805224 - Attachment is obsolete: true
Flags: needinfo?(andrebargull)
Attachment #8805505 - Flags: review+
(Assignee)

Comment 67

9 months ago
Verified locally that the newest patch works on WinXP (with and without automatic DST adjustment). Sorry for the extra troubles!
Keywords: checkin-needed

Comment 68

9 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7823287275f
Part 2: Implement timeZone support for Intl.DateTimeFormat. r=Waldo
Keywords: checkin-needed

Comment 69

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f7823287275f
(Assignee)

Updated

9 months ago
Keywords: leave-open
(Assignee)

Updated

9 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1314354
Keywords: dev-doc-needed
Depends on: 1316565
Added to developer release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/52#JavaScript

Updated reference pages:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DateTimeFormat
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleString
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleTimeString
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleDateString
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.