Split up Intl bits across multiple files into a builtin/intl subdirectory

RESOLVED FIXED in Firefox 60

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

(Blocks: 3 bugs)

58 Branch
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(25 attachments)

3.99 KB, patch
anba
: review+
Details | Diff | Splinter Review
39.49 KB, patch
anba
: review+
Details | Diff | Splinter Review
53.43 KB, patch
anba
: review+
Details | Diff | Splinter Review
162.61 KB, patch
anba
: review+
Details | Diff | Splinter Review
7.22 KB, patch
anba
: review+
Details | Diff | Splinter Review
47.68 KB, patch
anba
: review+
Details | Diff | Splinter Review
46.71 KB, patch
anba
: review+
Details | Diff | Splinter Review
103.42 KB, patch
anba
: review+
Details | Diff | Splinter Review
54.42 KB, patch
anba
: review+
Details | Diff | Splinter Review
43.06 KB, patch
anba
: review+
Details | Diff | Splinter Review
15.83 KB, patch
anba
: review+
Details | Diff | Splinter Review
2.34 KB, patch
anba
: review+
Details | Diff | Splinter Review
9.56 KB, patch
anba
: review+
Details | Diff | Splinter Review
13.45 KB, patch
anba
: review+
Details | Diff | Splinter Review
1.68 KB, patch
anba
: review+
Details | Diff | Splinter Review
2.38 KB, patch
anba
: review+
Details | Diff | Splinter Review
144.01 KB, patch
anba
: review+
Details | Diff | Splinter Review
136.53 KB, patch
anba
: review+
Details | Diff | Splinter Review
129.43 KB, patch
anba
: review+
Details | Diff | Splinter Review
129.15 KB, patch
anba
: review+
Details | Diff | Splinter Review
97.94 KB, patch
anba
: review+
Details | Diff | Splinter Review
80.96 KB, patch
anba
: review+
Details | Diff | Splinter Review
24.19 KB, patch
anba
: review+
Details | Diff | Splinter Review
1.01 KB, patch
anba
: review+
Details | Diff | Splinter Review
15.60 KB, patch
anba
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
This was mentioned in bug 1402379 as a thing to do.  I finally got fed up and did it -- well, up to some yak-shaving build issues -- over the last couple days.  Since bug 1402379 comment 0 gives a little broader scope to that bug's issues, let's hang this in a blocking bug.
(Assignee)

Updated

a year ago
Blocks: 1402379
(Assignee)

Updated

a year ago
Depends on: 1432340
(Assignee)

Updated

a year ago
Depends on: 1432346
(Assignee)

Comment 1

a year ago
Attachment #8944594 - Flags: review?(andrebargull)
(Assignee)

Comment 2

a year ago
These could somewhat be split across files based on functionality, but keeping them all together is at least no worse than what we have now.  And it clears goo out of Intl.*.
Attachment #8944596 - Flags: review?(andrebargull)
(Assignee)

Comment 4

a year ago
I made varying attempts in all these patches to minimize total #includes used, and to not lean on |using namespace *| wherever possible, preferring more qualification and similar.  Not totally consistent on it in every single patch, but I made an attempt.  (And I made such attempt even harder in headers, because extra #includes and such have much worse cost there than in single *.cpp files.)

Worth noting: for whatever reason, if I have

void
NumberFormatObject::finalize(FreeOp* fop, JSObject* obj)
{
...

Windows builds fail, claiming they don't know what |FreeOp| is, then erroring that this is a redefinition of the |finalize(FreeOp*, JSObject*)| overload.  I *think* argument type lookup is supposed to look in CollatorObject (which is really |js::CollatorObject| via a |using|), then in its enclosing namespace, &c. to ultimately find js::FreeOp.  So this looks buggy.  Probably because of the |using|?  Dunno.

But it's not onerous to instead of the above, have

void
js::NumberFormatObject::finalize(FreeOp* fop, JSObject* obj)
{
...

which Windows does properly interpret, so I just did that.  (That form *definitely* has argument type lookup proceed into |js|.)  I also made similar change when I split out (in later patches) DateTimeFormat, PluralRules, &c.
Attachment #8944600 - Flags: review?(andrebargull)
(Assignee)

Comment 11

a year ago
It may *depend* on Intl, but it *lives* on String, so it should be defined in String code for easiest searching.  And with newly-slimmed builtin/intl/*.h headers, it's no real compile overhead to define this outside of Intl code.
Attachment #8944608 - Flags: review?(andrebargull)
(Assignee)

Comment 26

a year ago
I've made a bunch of uninteresting changes to these patches to accommodate bug 1432340.  Not bothering to obsolete them all over it.
I'm not sure how I feel about using dozens of 'using' statements for objects in the 'js' namespace when we're already have a 'using namespace js' in the same file (resp. replacing 'using namespace js' with dozens of single 'using' statements). This seems to go against the usual code conventions in SpiderMonkey source files. I'd feel much more comfortable if this is first discussed in the team before applying a completely different code style for the Intl files.
Comment on attachment 8944594 [details] [diff] [review]
Move ScopedICUObject to its own header

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

::: js/src/builtin/Intl.cpp
@@ +72,5 @@
>  
>  using JS::ClippedTime;
>  using JS::TimeClip;
>  
> +using js::ScopedICUObject;

See comment #27.
Attachment #8944594 - Flags: review?(andrebargull) → review+
Comment on attachment 8944596 [details] [diff] [review]
Move ICU stub functions into builtin/intl/ICUStubs.h

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

::: js/src/builtin/intl/ICUStubs.h
@@ +7,5 @@
> +/*
> + * ICU types and declarations used by the Intl implementation, with fallbacks
> + * for when the engine is being built without Intl support that nonetheless
> + * allow our Intl implementation to still compile.
> + */

Somewhat duplicates the other comment in this file about why we have these stubs.

@@ +771,5 @@
> +#endif // !ENABLE_INTL_API
> +
> +// Starting with ICU 59, UChar defaults to char16_t.
> +static_assert(mozilla::IsSame<UChar, char16_t>::value,
> +              "SpiderMonkey doesn't support redefining UChar to a different type");

This looks like a dangerous spot to place this assertion, because if we implement bug 1433325, we probably just delete this complete file, but then we need to take care about not loosing this assertion. So it seems better to me to move this assertion in intl/CommonFunctions (part 3).
Attachment #8944596 - Flags: review?(andrebargull) → review+
Comment on attachment 8944597 [details] [diff] [review]
Move functionality used in the implementation of multiple Intl.* constructors into builtin/intl/CommonFunctions.*

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

::: js/src/builtin/Intl.cpp
@@ +2268,1 @@
>                                       chars, size, status);

Nit: indentation

@@ +3207,5 @@
>  
> +    JSString* str =
> +        CallICU(cx, [rtf, t, relDateTimeUnit, relDateTimeType](UChar* chars, int32_t size,
> +                                                               UErrorCode* status)
> +        {

And not wanting to write indentations like this were the only reason I named the function just 'Call' instead of anything with a longer name. ;-)

::: js/src/builtin/intl/CommonFunctions.cpp
@@ +11,5 @@
> +#include "jscntxt.h"
> +#include "jsfriendapi.h" // for GetErrorMessage, JSMSG_INTERNAL_INTL_ERROR
> +#include "jsobj.h"
> +
> +#include "js/Value.h"

Already explicitly included through CommonFunctions.h

::: js/src/builtin/intl/CommonFunctions.h
@@ +8,5 @@
> +#define builtin_intl_CommonFunctions_h
> +
> +#include <stddef.h>
> +#include <stdint.h>
> +#include <string.h>

Do you think it makes sense for new files to use the C++ headers instead of the older C headers? So '<cstring>' instead of '<string.h>' etc.

@@ +18,5 @@
> +#include "vm/String.h"
> +
> +class JSObject;
> +
> +namespace js {

I wonder if we should add a new 'js::intl' namespace here to avoid adding functions/constants with quite general names ('StringsAreEqual', 'INITIAL_CHAR_BUFFER_SIZE', etc.) to the 'js' namespace.

@@ +61,5 @@
> +    return !strcmp(s1, s2);
> +}
> +
> +static inline const char*
> +IcuLocale(const char* locale)

Hmm, 'IcuLocale' or 'ICULocale' for consistency with 'callICU' below?

@@ +120,5 @@
> +
> +/**
> + * Return an object whose own property names are the locales indicated as
> + * available by |countAvailable| that provides an overall count, and by
> + * |getAvailable| that when called passing number less than that count,

Nit: s/passing number/passing a number/
Attachment #8944597 - Flags: review?(andrebargull) → review+
Comment on attachment 8944600 [details] [diff] [review]
Move Intl.NumberFormat functionality into builtin/intl/NumberFormat.*

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

::: js/src/builtin/intl/NumberFormat.cpp
@@ +13,4 @@
>  
> +#include <algorithm>
> +#include <stddef.h>
> +#include <stdint.h>

'stdint' is already explicitly included in NumberFormat.h

::: js/src/builtin/intl/NumberFormat.h
@@ +9,3 @@
>  
> +#include "mozilla/Attributes.h"
> +#include "mozilla/TypeTraits.h"

Do we need to include "mozilla/TypeTraits.h"? I don't see any use of type traits in the header file, but maybe I'm just missing something here.
Attachment #8944600 - Flags: review?(andrebargull) → review+
Attachment #8944601 - Flags: review?(andrebargull) → review+
Comment on attachment 8944602 [details] [diff] [review]
Move Intl.Collator functionality into builtin/intl/Collator.*

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

Please use also 'hg copy' for Collator.{h,cpp} to preserve the file history.

::: js/src/builtin/intl/Collator.cpp
@@ +15,5 @@
> +
> +#include "builtin/intl/CommonFunctions.h"
> +#include "builtin/intl/ICUStubs.h"
> +#include "builtin/intl/ScopedICUObject.h"
> +#include "js/Class.h"

Already explicitly included through the header file.

@@ +19,5 @@
> +#include "js/Class.h"
> +#include "js/TypeDecls.h"
> +#include "vm/FreeOp.h"
> +#include "vm/GlobalObject.h"
> +#include "vm/NativeObject.h"

Already explicitly included through the header file.

@@ +36,5 @@
> +using JS::PrivateValue;
> +using JS::Rooted;
> +using JS::RootedObject;
> +using JS::RootedValue;
> +using JS::Value;

Also not necessary if 'using namespace js;' was used (comment #27), because other headers already include 'NamespaceImports.h'.

@@ +46,5 @@
> +using js::GlobalObject;
> +using js::IcuLocale;
> +using js::NewObjectWithGivenProto;
> +using js::RootedLinearString;
> +using js::StringEqualsAscii;

See comment #27
Attachment #8944602 - Flags: review?(andrebargull) → review+
Comment on attachment 8944604 [details] [diff] [review]
Move SharedIntlData into its own builtin/intl/SharedIntlData.* files so the world doesn't have to import all shared Intl functionality

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

::: js/src/builtin/intl/SharedIntlData.cpp
@@ +10,5 @@
> +
> +#include "mozilla/Assertions.h"
> +#include "mozilla/HashFunctions.h"
> +
> +#include <stddef.h>

Nit: The header already uses 'size_t', so '#include <stddef.h>' should probably be moved there, if we care about header hygiene here.

@@ +13,5 @@
> +
> +#include <stddef.h>
> +#include <stdint.h>
> +
> +#include "jsalloc.h"

Already explicitly included through the header.

@@ +21,5 @@
> +#include "builtin/intl/CommonFunctions.h"
> +#include "builtin/intl/ICUStubs.h"
> +#include "builtin/intl/ScopedICUObject.h"
> +#include "builtin/IntlTimeZoneData.h"
> +#include "js/Utility.h"

Already explicitly included through the header.

::: js/src/builtin/intl/SharedIntlData.h
@@ +146,5 @@
> +    /**
> +     * Returns the validated time zone name in |result|. If the input time zone
> +     * isn't a valid IANA time zone name, |result| remains unchanged.
> +     */
> +    bool validateTimeZoneName(JSContext* cx, JS::Handle<JSString*> timeZone,

Was there a specific reason to switch from 'HandleString' to 'Handle<JSString*>'?
Attachment #8944604 - Flags: review?(andrebargull) → review+
Comment on attachment 8944605 [details] [diff] [review]
Move Intl.DateTimeFormat functionality into builtin/intl/DateTimeFormat.*

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

::: js/src/builtin/intl/DateTimeFormat.cpp
@@ +14,1 @@
>  #include "builtin/intl/CommonFunctions.h"

Already explicitly included in the header.

@@ +19,3 @@
>  #include "vm/FreeOp.h"
>  #include "vm/GlobalObject.h"
> +#include "vm/NativeObject.h"

Already explicitly included in the header.

@@ +55,2 @@
>  using js::ScopedICUObject;
> +using js::StartOfTime;

Comment #27

@@ +203,5 @@
>      return proto;
>  }
>  
>  bool
> +js::intl_DateTimeFormat(JSContext* cx, unsigned argc, Value* vp)

Why did you move these two functions? I'd prefer they are kept at their original position to keep the same function order in all Intl files.
Attachment #8944605 - Flags: review?(andrebargull) → review+
Comment on attachment 8944606 [details] [diff] [review]
Move Intl.PluralRules functionality into builtin/intl/PluralRules.*

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

'hg copy' for PluralRules.h would have been better, but I guess in this case there isn't much file history to preserve.

::: js/src/builtin/intl/PluralRules.cpp
@@ +46,1 @@
>  using js::ScopedICUObject;

Comment #27
Attachment #8944606 - Flags: review?(andrebargull) → review+
Comment on attachment 8944607 [details] [diff] [review]
Move Intl.RelativeTimeFormat functionality into builtin/intl/RelativeTimeFormat.*

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

::: js/src/builtin/intl/RelativeTimeFormat.cpp
@@ +40,5 @@
> +using js::GlobalObject;
> +using js::IcuLocale;
> +using js::NewObjectWithGivenProto;
> +using js::RelativeTimeFormatObject;
> +using js::ScopedICUObject;

Comment #27.
Attachment #8944607 - Flags: review?(andrebargull) → review+
Comment on attachment 8944608 [details] [diff] [review]
Move String-based Intl-dependent functionality out of Intl.cpp, into jsstr.cpp

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

::: js/src/jsstr.h
@@ +347,5 @@
> +/**
> + * Returns the input string converted to lower case based on the language
> + * specific case mappings for the input locale.
> + *
> + * This function only works if EXPOSE_INTL_API!

Maybe "if EXPOSE_INTL_API is true" or "if EXPOSE_INTL_API is enabled"?
Attachment #8944608 - Flags: review?(andrebargull) → review+
Attachment #8944609 - Flags: review?(andrebargull) → review+
Attachment #8944610 - Flags: review?(andrebargull) → review+
Comment on attachment 8944611 [details] [diff] [review]
Move various generated files from builtin/Intl* to builtin/intl/*, and add "Generated" to their names for clarity

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

It seems like the current convention is to use "Generated" as a suffix instead of using it as a prefix:
  https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/js/src/moz.build#638-643
  https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/js/src/moz.build#779-785

So please change it to a suffix.
Attachment #8944611 - Flags: review?(andrebargull) → review+
Attachment #8944612 - Flags: review?(andrebargull) → review+
Attachment #8944613 - Flags: review?(andrebargull) → review+
Attachment #8944614 - Flags: review?(andrebargull) → review+
Attachment #8944615 - Flags: review?(andrebargull) → review+
Attachment #8944616 - Flags: review?(andrebargull) → review+
Attachment #8944617 - Flags: review?(andrebargull) → review+
Attachment #8944618 - Flags: review?(andrebargull) → review+
Attachment #8944619 - Flags: review?(andrebargull) → review+
Attachment #8944620 - Flags: review?(andrebargull) → review+
Attachment #8944621 - Flags: review?(andrebargull) → review+
Attachment #8944623 - Flags: review?(andrebargull) → review+
The hg-copied files were especially easy to review, thanks for doing that! :-)

Comment 40

a year ago
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6518882d8471
Move ScopedICUObject to its own header.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb9f4e463db
Move ICU stub functions into builtin/intl/ICUStubs.h.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/20c515e44ae8
Move functionality used in the implementation of multiple Intl.* constructors into builtin/intl/CommonFunctions.*.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6db3ed5c6f6
Move Intl.NumberFormat functionality into builtin/intl/NumberFormat.*.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aabca30def8
Move NewUNumberFormatForPluralRules next to its only use.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf6b00396d19
Move Intl.Collator functionality into builtin/intl/Collator.*.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/a671868ffa9c
Move SharedIntlData into its own builtin/intl/SharedIntlData.* files so the world doesn't have to import all shared Intl functionality.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/31be1665a5d7
Move Intl.DateTimeFormat functionality into builtin/intl/DateTimeFormat.*.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/21463ee0ce1a
Move Intl.PluralRules functionality into builtin/intl/PluralRules.*.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa8df97799c
Move Intl.RelativeTimeFormat functionality into builtin/intl/RelativeTimeFormat.*.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e7562a2e2af
Move String-based Intl-dependent functionality out of Intl.cpp, into jsstr.cpp.  It may *depend* on Intl, but it *lives* on String, so it should be defined in String code for easiest searching.  And with newly-slimmed builtin/intl/*.h headers, it's no real compile overhead to define this outside of Intl code.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/147d42b58a5e
Trim builtin/Intl.cpp's #include set down to size, now that it contains *only* stuff related to Intl, its non-constructor properties, and initialization of Intl.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f315f5bdd5a
Move builtin/Intl.* to builtin/intl/IntlObject.* and trim the contents of them a little further.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c438d33f37d
Move various generated files from builtin/Intl* to builtin/intl/*, and add "Generated" to their names for clarity.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f84c0c14d7c
Rerun |python ./make_intl_data.py currency| generating currency data into the new location, demonstrating that the only changes that happen are because upstream data changed (in insignificant ways).  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/19ce0b9891f4
Rerun |python ./make_intl_data.py langtags| generating langtag mapping data into the new location, demonstrating that the only changes that happen are timestamp changes).  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/3afd136081b8
Move the self-hosting of non-constructor properties of Intl to a new builtin/intl/IntlObject.js file.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8bd220344f3
Move Intl.RelativeTimeFormat self-hosted code to a new builtin/intl/RelativeTimeFormat.js file.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/b07e1addf75a
Move Intl.PluralRules self-hosted code to a new builtin/intl/PluralRules.js file.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcd670c8e8bf
Move Intl.DateTimeFormat self-hosted code to a new builtin/intl/DateTimeFormat.js file.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/c404144650e3
Move Intl.NumberFormat self-hosted code to a new builtin/intl/NumberFormat.js file.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b62c6b3c1cf
Move Intl.Collator self-hosted code to a new builtin/intl/Collator.js file.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/78bfd0529ebc
Move builtin/Intl.js (which now contains only shared functionality) to builtin/intl/CommonFunctions.js.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/767fb4816abc
js/src/builtin/Intl* doesn't exist any more, so don't tag it as BUG_COMPONENT = component_intl.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/c50cbf6e8176
Move a bunch of functions in builtin/intl/CommonFunctions.js into more-specific files, where those functions are only used in a single more-specific file.  r=anba
(Assignee)

Comment 41

a year ago
(In reply to André Bargull [:anba] from comment #27)
> I'm not sure how I feel about using dozens of 'using' statements for objects
> in the 'js' namespace when we're already have a 'using namespace js' in the
> same file (resp. replacing 'using namespace js' with dozens of single
> 'using' statements). This seems to go against the usual code conventions in
> SpiderMonkey source files. I'd feel much more comfortable if this is first
> discussed in the team before applying a completely different code style for
> the Intl files.

The scattershot approach of |using| everything (and then some) in one declaration is pretty bad IMO for hygiene.  I reverted this for now, but I think it would be better for us to be explicit about this.  (Incidentally, this also helped me minimize #includes in these files better, by knowing precisely what I was using.)

(In reply to André Bargull [:anba] from comment #29)
> > +// Starting with ICU 59, UChar defaults to char16_t.
> > +static_assert(mozilla::IsSame<UChar, char16_t>::value,
> > +              "SpiderMonkey doesn't support redefining UChar to a different type");
> 
> This looks like a dangerous spot to place this assertion, because if we
> implement bug 1433325, we probably just delete this complete file, but then
> we need to take care about not loosing this assertion. So it seems better to
> me to move this assertion in intl/CommonFunctions (part 3).

Moved.

(In reply to André Bargull [:anba] from comment #30)
> And not wanting to write indentations like this were the only reason I named
> the function just 'Call' instead of anything with a longer name. ;-)

Blargh.  Really there's just nothing good for this.

> Do you think it makes sense for new files to use the C++ headers instead of
> the older C headers? So '<cstring>' instead of '<string.h>' etc.

Not unless we start using |std::uint32_t| and similar.  The global names are not guaranteed to be provided by the new-style headers.

> I wonder if we should add a new 'js::intl' namespace here to avoid adding
> functions/constants with quite general names ('StringsAreEqual',
> 'INITIAL_CHAR_BUFFER_SIZE', etc.) to the 'js' namespace.

Added, but mostly only for these types/names right now.  I pondered adding them for the various intl_* functions the other headers have, but those are directly named after the intrinsics as written out in self-hosted code, so renaming them seemed wrong.  We can loop back and futz if anyone really cares after.

> Hmm, 'IcuLocale' or 'ICULocale' for consistency with 'callICU' below?

Left as-is, not because I dislike consistency, but because I dislike the name entirely.  I don't remember the input IcuLocale takes, or the output it returns, and how it's supposed to be used.  Something like "convert user-facing locale to a locale ICU understands"?  Didn't remember, punted it hard.  But we should have a different name for this, that makes clear the intermediation it's doing.  (The implementation doesn't clarify it enough for me to even hazard writing a comment documenting it, or renaming it to something actually good, unfortunately.)

(In reply to André Bargull [:anba] from comment #34)
> >  bool
> > +js::intl_DateTimeFormat(JSContext* cx, unsigned argc, Value* vp)
> 
> Why did you move these two functions? I'd prefer they are kept at their
> original position to keep the same function order in all Intl files.

There was...a reason for this at some point.  Can't see one now.  Reverted.

(In reply to André Bargull [:anba] from comment #37)
> Maybe "if EXPOSE_INTL_API is true" or "if EXPOSE_INTL_API is enabled"?

Went with "#if EXPOSE_INTL_API" as being actually precise.

(In reply to André Bargull [:anba] from comment #38)
> It seems like the current convention is to use "Generated" as a suffix
> instead of using it as a prefix:

I did this, but I'm not convinced it's what we should have done.  Keeping generated files together in listings, versus interpolated with human-written files, is IMO messy.  Maybe worth renaming stuff in another bug/followup.
https://hg.mozilla.org/mozilla-central/rev/6518882d8471
https://hg.mozilla.org/mozilla-central/rev/ffb9f4e463db
https://hg.mozilla.org/mozilla-central/rev/20c515e44ae8
https://hg.mozilla.org/mozilla-central/rev/e6db3ed5c6f6
https://hg.mozilla.org/mozilla-central/rev/2aabca30def8
https://hg.mozilla.org/mozilla-central/rev/cf6b00396d19
https://hg.mozilla.org/mozilla-central/rev/a671868ffa9c
https://hg.mozilla.org/mozilla-central/rev/31be1665a5d7
https://hg.mozilla.org/mozilla-central/rev/21463ee0ce1a
https://hg.mozilla.org/mozilla-central/rev/8aa8df97799c
https://hg.mozilla.org/mozilla-central/rev/8e7562a2e2af
https://hg.mozilla.org/mozilla-central/rev/147d42b58a5e
https://hg.mozilla.org/mozilla-central/rev/4f315f5bdd5a
https://hg.mozilla.org/mozilla-central/rev/7c438d33f37d
https://hg.mozilla.org/mozilla-central/rev/6f84c0c14d7c
https://hg.mozilla.org/mozilla-central/rev/19ce0b9891f4
https://hg.mozilla.org/mozilla-central/rev/3afd136081b8
https://hg.mozilla.org/mozilla-central/rev/d8bd220344f3
https://hg.mozilla.org/mozilla-central/rev/b07e1addf75a
https://hg.mozilla.org/mozilla-central/rev/bcd670c8e8bf
https://hg.mozilla.org/mozilla-central/rev/c404144650e3
https://hg.mozilla.org/mozilla-central/rev/9b62c6b3c1cf
https://hg.mozilla.org/mozilla-central/rev/78bfd0529ebc
https://hg.mozilla.org/mozilla-central/rev/767fb4816abc
https://hg.mozilla.org/mozilla-central/rev/c50cbf6e8176
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.