818634 - Remove support for Date.prototype.toLocaleFormat

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Bruant]

This is Firefox-specific as far as I can tell, not part of any version of ECMAScript to my knowledge and the internationalization API [1] has an to-be-standard replacement.

[1] http://norbertlindenberg.com/ecmascript/intl.html

Comment 1

[David Bruant]

It's used here and there in Firefox:
http://mxr.mozilla.org/mozilla-central/search?string=toLocaleFormat

Is it used in addons?

Comment 2

[Matthias Versen [:Matti]]

not only in Firefox: http://mxr.mozilla.org/comm-central/search?string=toLocaleFormat

Comment 3

[Jesse Ruderman]

> Is it used in addons?

https://mxr.mozilla.org/addons/search?string=toLocaleFormat

Found 366 matching lines in 159 files

Comment 4

[Jesse Ruderman]

This is a bad API with a terrible implementation (I just filed three more bugs against it).  I hope we remove it.

Given the number of addons that use it, it might be best to wait until the replacement API has shipped, then add a console warning and notify add-on authors.

Comment 5

[sssarvjeet27]

this could be my second bug.So could you please provide more information what exactly is to be done?

Comment 6

[Chris Peterson [:cpeterson]]

sssarvjeet27, thanks for your help! I recommend that you can ask on Mozilla's #jsapi IRC channel for more information about what JS API will replace toLocaleFormat().

There are many calls to the old toLocaleFormat() API and they can be replace file by file in separate patches. You don't need to fix all calls at once. :)

Comment 7

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/date-prototype-tolocaleformat-will-be-removed/

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

14 uses left in code, but more in testcases.

Comment 9

[Kohei Yoshino [:kohei]]

Note: Intl API has been implemented in Fennec, but still disabled in the Release channel (Bug 1343725). This change should be blocked until it's enabled (Bug 1344625).

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Or we could do what Thunderbird is doing and implement a .jsm for Fennec - bug 1332351.

I'd personally would prefer this route forward so that we can clean up the platform and isolate the code that still needs it rather than make the whole platform pay the maintenance price tag.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

There is no more toLocaleFormat uses in mozilla-central.

We should be good to remove it completely!

Comment 12

[Dão Gottwald [:dao]]

(In reply to Jesse Ruderman from comment #4)
> This is a bad API with a terrible implementation (I just filed three more
> bugs against it).  I hope we remove it.

Bad maybe, handy nonetheless. :)

Having to implement something like toISODateString in JS just feels wrong: https://bug1123372.bmoattachments.org/attachment.cgi?id=8598264

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Dão Gottwald [::dao] from comment #12)
> Having to implement something like toISODateString in JS just feels wrong:
> https://bug1123372.bmoattachments.org/attachment.cgi?id=8598264

I think that the problem with the API is that it tried to pretend that it's an internationalization API, while in fact it's a date formatting API.

There's nothing i18n/l10n related in formatting an ISO date.

Comment 14

[Tom S [:evilpie]]

This is a non-standard API, there shouldn't be any discussion about removing this, unless we are willing and able to get this specified. Seems like we can remove this now, so we should.

Comment 15

[Dão Gottwald [:dao]]

(In reply to Limited Availabilty till Oct | Tom Schuster [:evilpie] from comment #14)
> This is a non-standard API, there shouldn't be any discussion about removing
> this, unless we are willing and able to get this specified. Seems like we
> can remove this now, so we should.

As stated that binary choice seems overly simplistic. For instance, to satisfy comment 13 we could rename the API and make it chrome-only while trying to get it specified.

Comment 16

[Masatoshi Kimura [:emk]]

(In reply to Limited Availabilty till Oct | Tom Schuster [:evilpie] from comment #14)
> This is a non-standard API, there shouldn't be any discussion about removing
> this, unless we are willing and able to get this specified. Seems like we
> can remove this now, so we should.

If this is not web-visible, you are right. But unfortunately it is. So we will have to take a standard deprecation procedure (Telemetry, console warning, disable on Nightly first, send "Intent to unship" to dev-platform, etc.) :(

Comment 17

[André Bargull [:anba]]

Warnings were already added in bug 1299900, but we don't yet collect telemetry because of bug 1331909.

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

> As stated that binary choice seems overly simplistic. For instance, to satisfy comment 13 we could rename the API and make it chrome-only while trying to get it specified.

The API is way too big for the purpose you'd like to keep it for. `DateUtils.formatToISO(date)` - all other uses of the toLocaleFormat that I encountered should use Intl API instead.

And such `formatToISO` function would be much smaller than `toLocaleFormat` since it is literally:

```
const pad = (d) => d.toString().padStart(2, 0);
formatToISO(date){
  return `${date.getUTCFullYear()}-${pad(date.getUTCMonth() + 1)}-${pad(date.getUTCDate())}`;
}
```

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

Whoever will take this will also have the pleasure of removing this code: http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/toolkit/xre/nsAppRunner.cpp#5324

which is only needed for this function to not leak locale.

This may turn into a perf win since we're removing a very early prefs read :)

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

At this point its a pretty simple change to just remove all of that: http://searchfox.org/mozilla-central/search?q=toLocaleFormat&path=

I'd be happy to mentor if someone wants to pick it up.

Comment 21

[Rofael Aleezada [:rofael]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #20)
> At this point its a pretty simple change to just remove all of that:
> http://searchfox.org/mozilla-central/search?q=toLocaleFormat&path=
> 
> I'd be happy to mentor if someone wants to pick it up.

So from my understanding, this is an API that is now deprecated that needs to be removed. I assume that all of the references should be removed, and that I won't have to worry about replacing any code? 

Also, since there's so many changes that need to be made, how would the changes be broken up in terms of patches (I'm guessing several smaller patches i.e. a patch for removing the JS test cases, and a patch for each Core file modified)?

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Rofael Aleezada from comment #21)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #20)
> > At this point its a pretty simple change to just remove all of that:
> > http://searchfox.org/mozilla-central/search?q=toLocaleFormat&path=
> > 
> > I'd be happy to mentor if someone wants to pick it up.
> 
> So from my understanding, this is an API that is now deprecated that needs
> to be removed. I assume that all of the references should be removed, and
> that I won't have to worry about replacing any code? 

Exactly! :)
There are no more users of this code in our codebase, so it's about:
 - removing the code for it
 - removing the helper functions for it
 - removing the deprecation warning functions
 - removing the comments mentioning it
 - removing tests for it
 - removing code mentioned in comment 19

Sweet, fun, just removal :)

 
> Also, since there's so many changes that need to be made, how would the
> changes be broken up in terms of patches (I'm guessing several smaller
> patches i.e. a patch for removing the JS test cases, and a patch for each
> Core file modified)?

I don't think you need to separate it into multiple patches. It's a one time removal that is not easily or necessarily chunkable.

Comment 23

[Rofael Aleezada [:rofael]]

Sounds good! Could I have it?

Comment 24

[Zibi Braniecki [:zbraniecki][:gandalf]]

All yours! I'm jealous a bit, since removing old code is my favorite thing to do, but I don't have time to work on it now :(

So, be my guest, and let me know if you have any questions here or on IRC :)

Have fun!

Comment 25

[Rofael Aleezada [:rofael]]

Attachment: 818634.patch

This look good? mach build was successful.

Comment 26

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8919979 [details] [diff] [review]
818634.patch

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

Overall, this looks great, thank you!

Unfortunately, there's a set of internal functions that are still in use for WITHOUT_INTL_API scenario so we have to keep them.
Please, restore them in your patch.

Once you're done, I'll take a look again and add :anba as the final reviewer.

::: js/src/jsdate.cpp
@@ -2788,5 @@
> -    return ToLocaleFormatHelper(cx, dateObj, format, args.rval());
> -}
> -
> -static bool
> -date_toLocaleString(JSContext* cx, unsigned argc, Value* vp)

oh, damn :( 

I'm afraid you have to leave those functions and the `ToLocaleFormatHelper` for now.

We have a special if-else case for building SpiderMonkey without INTL_API.
If we build it without INTL_API then we use those functions for `Date.toLocaleString, Date.toLocaleTimeString and Date.toLocaleDateString`.

See: http://searchfox.org/mozilla-central/source/js/src/jsdate.cpp#3048

That means that for now, unfortunately you can only remove the `date_toLocaleFormat` and `date_toLocaleFormat_impl` parts here.

::: js/src/vm/Time.cpp
@@ -322,5 @@
>       * Note that FAKE_YEAR_BASE should be a multiple of 100 to make 2-digit
>       * year formats (%y) work correctly (since we won't find the fake year
>       * in that case).
> -     * e.g. new Date(1873, 0).toLocaleFormat('%Y %y') => "1873 73"
> -     * See bug 327869.

It may be possible to even remove the whole block. Please, keep it for now as-is, but I'm flagging it for the next reviewer.

Comment 27

[Rofael Aleezada [:rofael]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #26)
> Comment on attachment 8919979 [details] [diff] [review]
> 818634.patch
> 
> Review of attachment 8919979 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall, this looks great, thank you!
> 
> Unfortunately, there's a set of internal functions that are still in use for
> WITHOUT_INTL_API scenario so we have to keep them.
> Please, restore them in your patch.
> 
> Once you're done, I'll take a look again and add :anba as the final reviewer.
> 
> ::: js/src/jsdate.cpp
> @@ -2788,5 @@
> > -    return ToLocaleFormatHelper(cx, dateObj, format, args.rval());
> > -}
> > -
> > -static bool
> > -date_toLocaleString(JSContext* cx, unsigned argc, Value* vp)
> 
> oh, damn :( 
> 
> I'm afraid you have to leave those functions and the `ToLocaleFormatHelper`
> for now.
> 
> We have a special if-else case for building SpiderMonkey without INTL_API.
> If we build it without INTL_API then we use those functions for
> `Date.toLocaleString, Date.toLocaleTimeString and Date.toLocaleDateString`.
> 
> See: http://searchfox.org/mozilla-central/source/js/src/jsdate.cpp#3048
> 
> That means that for now, unfortunately you can only remove the
> `date_toLocaleFormat` and `date_toLocaleFormat_impl` parts here.
> 
> ::: js/src/vm/Time.cpp
> @@ -322,5 @@
> >       * Note that FAKE_YEAR_BASE should be a multiple of 100 to make 2-digit
> >       * year formats (%y) work correctly (since we won't find the fake year
> >       * in that case).
> > -     * e.g. new Date(1873, 0).toLocaleFormat('%Y %y') => "1873 73"
> > -     * See bug 327869.
> 
> It may be possible to even remove the whole block. Please, keep it for now
> as-is, but I'm flagging it for the next reviewer.

So I should undo all the changes I made to jsdate.cpp?

Comment 28

[Rofael Aleezada [:rofael]]

> That means that for now, unfortunately you can only remove the
> `date_toLocaleFormat` and `date_toLocaleFormat_impl` parts here.

Oh, I missed that last line. Got it.

Comment 29

[Rofael Aleezada [:rofael]]

Attachment: 818634-2.patch

Comment 30

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8920201 [details] [diff] [review]
818634-2.patch

Thanks! That looks good to me!

Redirecting r? to anba for SM sanity.

Comment 31

[André Bargull [:anba]]

Comment on attachment 8920201 [details] [diff] [review]
818634-2.patch

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

::: dom/canvas/test/webgl-conf/checkout/deqp/temp_externs/es3.js
@@ -1492,5 @@
> - * @return {string}
> - * @nosideeffects
> - * @see http://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/Date/toLocaleFormat
> - */
> -Date.prototype.toLocaleFormat = function(formatString) {};

This looks like an imported file from somewhere else, so we shouldn't modify its content.

::: js/src/jsdate.cpp
@@ -2881,5 @@
> -    return ToLocaleFormatHelper(cx, dateObj, fmtbytes.ptr(), args.rval());
> -}
> -
> -static bool
> -date_toLocaleFormat(JSContext* cx, unsigned argc, Value* vp)

The line referencing this function needs to be removed, too. 

-> http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/js/src/jsdate.cpp#3047

::: js/src/tests/ecma_5/extensions/extension-methods-reject-null-undefined-this.js
@@ +34,4 @@
>                 "sup", "sub", "substr", "trimLeft", "trimRight", "toJSON"],
>      Boolean:  ["toSource", "toJSON"],
>      Number:   ["toSource", "toJSON"],
> +    Date:     ["toSource", "getYear", 

There's a trailing white space in this line. But actually it looks like the complete array fits into a single line without exceeding the 100 characters limit. So let's collapse the two lines into:

```
Date: ["toSource", "getYear", "setYear", "toGMTString"],
```

::: js/xpconnect/src/XPCLocale.cpp
@@ -175,5 @@
>  
> -  bool
> -  ToUnicode(JSContext* cx, const char* src, MutableHandleValue rval)
> -  {
> -    // This code is only used by our prioprietary toLocaleFormat method

The code in this file shouldn't be removed, because in addition to toLocaleFormat, it's actually also used for Date.prototype.toLocale(Date|Time)String when ICU isn't enabled.

::: toolkit/xre/nsAppRunner.cpp
@@ -5324,5 @@
> -OverrideDefaultLocaleIfNeeded() {
> -  // Read pref to decide whether to override default locale with US English.
> -  if (mozilla::Preferences::GetBool("javascript.use_us_english_locale", false)) {
> -    // Set the application-wide C-locale. Needed to resist fingerprinting
> -    // of Date.toLocaleFormat(). We use the locale to "C.UTF-8" if possible,

I don't really trust this comment that we only need to change the locale because of Date.prototype.toLocaleFormat. We probably should ask :arthuredelstein about this one.

Comment 32

[André Bargull [:anba]]

Is this comment [1] in nsAppRunner.cpp still accurate, that means is it really only necessary to change LC_ALL for Date.prototype.toLocaleFormat? And can we remove the call to set LC_ALL when Date.prototype.toLocaleFormat gets removed?

Thanks,
André


[1] http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/toolkit/xre/nsAppRunner.cpp#5230-5233

Comment 33

[Rofael Aleezada [:rofael]]

(In reply to André Bargull [:anba] from comment #31)
> :::
> js/src/tests/ecma_5/extensions/extension-methods-reject-null-undefined-this.
> js
> @@ +34,4 @@
> >                 "sup", "sub", "substr", "trimLeft", "trimRight", "toJSON"],
> >      Boolean:  ["toSource", "toJSON"],
> >      Number:   ["toSource", "toJSON"],
> > +    Date:     ["toSource", "getYear", 
> 
> There's a trailing white space in this line. But actually it looks like the
> complete array fits into a single line without exceeding the 100 characters
> limit. So let's collapse the two lines into:
> 
> ```
> Date: ["toSource", "getYear", "setYear", "toGMTString"],
> ```

Ah, I was wondering how thee line formatting worked!

I've gone ahead and implemented the changes, including undoing all the ToUnicode related deletions, but I'll hold off on posting a patch until we hear back.

Comment 34

[Zibi Braniecki [:zbraniecki][:gandalf]]

> The code in this file shouldn't be removed, because in addition to toLocaleFormat, it's actually also used for Date.prototype.toLocale(Date|Time)String when ICU isn't enabled.

Andre, we do not currently support not enabling ICU in Firefox. We kept the non-ICU codepath just for experimental uses of SM with Servo etc.

I do not believe we should keep the Firefox related privacy special codepaths not affecting Firefox in SM anymore.

Comment 35

[André Bargull [:anba]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #34)
> > The code in this file shouldn't be removed, because in addition to toLocaleFormat, it's actually also used for Date.prototype.toLocale(Date|Time)String when ICU isn't enabled.
> 
> Andre, we do not currently support not enabling ICU in Firefox. We kept the
> non-ICU codepath just for experimental uses of SM with Servo etc.

Okay, so that means we can remove:
- http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/js/xpconnect/src/XPCLocale.cpp#126-136
- http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/js/xpconnect/src/XPCLocale.cpp#139-196

Set "localeCompare" and "localeToUnicode" to nullptr in:
http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/js/xpconnect/src/XPCLocale.cpp#82-83

And verify that "localeCompare" and "localeToUnicode" are nullptr in:
http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/js/xpconnect/src/XPCLocale.cpp#118-119

Assuming XPCLocale is only used when ICU is definitely available. Is that correct?

> 
> I do not believe we should keep the Firefox related privacy special
> codepaths not affecting Firefox in SM anymore.

Do you mean my comment about the change in toolkit/xre/nsAppRunner.cpp? But that's not a SpiderMonkey component, is it? :-)

Comment 36

[Rofael Aleezada [:rofael]]

(In reply to André Bargull [:anba] from comment #35)

> Assuming XPCLocale is only used when ICU is definitely available. Is that
> correct?

I've gone ahead and changed XPCLocale on top of what I changed yesterday. I'll post the patch once we're sure this is the case.

Comment 37

[Zibi Braniecki [:zbraniecki][:gandalf]]

> Assuming XPCLocale is only used when ICU is definitely available. Is that correct?

Yes.
XPCLocale includes mozilla/intl/LocaleService which has a hard-dependency on ICU. I'd say that if there was a scenario where XPCLocale was meant to work without ICU, it would be already breaking hard for a while.

> Do you mean my comment about the change in toolkit/xre/nsAppRunner.cpp? But that's not a SpiderMonkey component, is it? :-)

Yes, that's what I was referring to.

Basically, the "javascript.use_us_english_locale" is checked in two places:

 - in XPCLocale where, if true, it sets the JS context default locale to "en-US" - this handles all Intl APIs.
 - in nsAppRunner.cpp specifically because toLocaleFormat was not using any Intl API and instead was looking into POSIX env directly.

With the removal of toLocaleFormat, we do not have any other JS API that skips the context's defaultLocale, so we don't need to keep this function on the startup path.

I'll keep NI on arthur to confirm that :)


Rofael - thank you for your work and patience and I apologies for making your work harder. Your patch is cleaning up a pretty hacky piece of our codebase, and Andre is right to be cautious about what we can and cannot rip out yet.

I believe you can proceed with the assumption that XPCLocale does depend on ICU and apply Andre's feedback from comment 35.

With regards to nsAppRunner piece, let's wait for Arthur's feedback.

Thank you!

Comment 38

[Rofael Aleezada [:rofael]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #37)

Its all good, Zibi, working on Firefox has been great :)

I've made the changes in comment #35, and I'll hold off on anything else until we get the needed info.

Comment 39

[Rofael Aleezada [:rofael]]

Attachment: 818634-3.patch

To refresh from the weekend, this is everything I've done so far.

Comment 40

[André Bargull [:anba]]

Comment on attachment 8921152 [details] [diff] [review]
818634-3.patch

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

I forgot to check the previous patch for unused functions (the build is configured to fail when unused functions are detected), so some additional changes in jsdate.cpp and XPCLocale.cpp are still needed. Sorry for not spotting that earlier! Apart from that, I think the patch is in a landable shape, so I'm already setting r+. Thanks for the patch! :-)
 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd9092b03ff87af40988714a9a94a478cff99715&selectedJob=139169168 indicates this guard [1] now needs to be moved before ToLocaleFormatHelper [2], because ToLocaleFormatHelper is now only used when |EXPOSE_INTL_API| is false.

[1] https://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/js/src/jsdate.cpp#2770
[2] https://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/js/src/jsdate.cpp#2725-2726

::: js/xpconnect/src/XPCLocale.cpp
@@ +79,1 @@
>      // only consulted when EXPOSE_INTL_API is not set.)

Nit: The comment above should be updated to note that all hooks are disabled because EXPOSE_INTL_API is nowadays always set when XPCLocale is used.

@@ -131,5 @@
> -
> -  static bool
> -  LocaleCompare(JSContext* cx, HandleString src1, HandleString src2, MutableHandleValue rval)
> -  {
> -    return This(cx)->Compare(cx, src1, src2, rval);

|XPCLocaleCallbacks::This(JSContext*)| [1] is now also no longer used and can be removed as well.

[1] https://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/js/xpconnect/src/XPCLocale.cpp#96-103

@@ -138,3 @@
>  private:
> -  bool
> -  Compare(JSContext* cx, HandleString src1, HandleString src2, MutableHandleValue rval)

Nit: With Compare(...) being removed, we can also remove |mCollation| [1] and clean-up the includes [2] and maybe also [3] (looks like this include was only needed for NS_CopyNativeToUnicode).

[1] https://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/js/xpconnect/src/XPCLocale.cpp#203
[2] https://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/js/xpconnect/src/XPCLocale.cpp#11,13
[3] https://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/js/xpconnect/src/XPCLocale.cpp#15

Comment 41

[André Bargull [:anba]]

@gandalf:

I wonder if there's a better way to make sure we only create a single XPCLocaleObserver for each runtime instead of (ab)using XPCLocaleCallbacks as a boolean flag in [1]? And I also wonder if we should simply remove the comments about multi-threading and the assertions [2], because I don't think they still apply when all hooks are removed from XPCLocaleCallbacks.

[1] https://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/js/xpconnect/src/XPCLocale.cpp#211-218
[2] https://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/js/xpconnect/src/XPCLocale.cpp#65-70,92,122,198-201,205

Comment 42

[Rofael Aleezada [:rofael]]

Attachment: 818634-4.patch

I've made your changes in comment #40, Andre. I'll tag Zibi in this one to get his look.

Comment 43

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8921699 [details] [diff] [review]
818634-4.patch

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

This looks good!

> I wonder if there's a better way to make sure we only create a single XPCLocaleObserver for each runtime

There may be. I just followed what :smaug told me to do there! :)

On the nsAppRunner's OverrideDefaultLocaleIfNeeded - maybe it's not worth waiting for Arthur. Feel free to file a follow up specifically for the removal of that function, mark it in the code with a comment like:

```
//XXX: This function should be not needed anymore. See bug XXXXX for details.
```

and land this patch with the nit fixed.

::: js/xpconnect/src/XPCLocale.cpp
@@ +73,5 @@
>  
>      // Disable the toLocaleUpper/Lower case hooks to use the standard,
>      // locale-insensitive definition from String.prototype. (These hooks are
> +    // only consulted when EXPOSE_INTL_API is not set.) 
> +	// Note: Since EXPOSE_INTL_API is always set, all hooks are disabled.

this seems misaligned

Comment 44

[Rofael Aleezada [:rofael]]

Attachment: 818634-5.patch

Comment to nsAppRunner added and XPCLocale comment (hopefully) fixed, kind of funny to think my biggest hurdles have been with comments.

Comment 45

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8922082 [details] [diff] [review]
818634-5.patch

That looks good!

Feel free to land (or add `checkin-needed` keyword)

Comment 46

[Kohei Yoshino [:kohei]]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/date-prototype-tolocaleformat-has-been-removed/

Comment 47

[Jeff Walden [:Waldo]]

(In reply to Rofael Aleezada [:rofael] from comment #44)
> Comment to nsAppRunner added and XPCLocale comment (hopefully) fixed, kind
> of funny to think my biggest hurdles have been with comments.

Comments are important.  :-)  Barring compiler errors or code that invokes undefined behavior, the actual code itself always does what it says.  But comments don't "do" anything, so there's no effect of them that can be observed (and perhaps observed to be wrong).  So it's super-important that they remain accurate, and extra care and attention is required to ensure they don't become actively misleading -- which is in many ways worse even than *no* comments.  It's well worth the trouble to be extra-attentive when it comes to comments.

This function's been a thorn in our side forever, what with its almost explicit reliance on a thing under the hood that's unspecified and not specifiable without a bunch of pain -- plus it was an ugly, non-JS API to begin with.  Thanks a ton for helping us kill it!

Comment 48

[Rofael Aleezada [:rofael]]

(In reply to Jeff Walden [:Waldo] (I'm baaaaaaack...) from comment #47)
> Comments are important.  :-)  Barring compiler errors or code that invokes
> undefined behavior, the actual code itself always does what it says.  But
> comments don't "do" anything, so there's no effect of them that can be
> observed (and perhaps observed to be wrong).  So it's super-important that
> they remain accurate, and extra care and attention is required to ensure
> they don't become actively misleading -- which is in many ways worse even
> than *no* comments.  It's well worth the trouble to be extra-attentive when
> it comes to comments.

Of course! I have trouble reading old code I wrote myself because I didn't comment it, I understand how critical it is for any project, much less one as massive as Firefox. I was being specific about those extra spaces in the diff that didn't show up in VS2017.

> This function's been a thorn in our side forever, what with its almost
> explicit reliance on a thing under the hood that's unspecified and not
> specifiable without a bunch of pain -- plus it was an ugly, non-JS API to
> begin with.  Thanks a ton for helping us kill it!

There's something really neat about fixing a bug that's been open for so long. Sure it took a few days, but it was a really fun first bug! Zibi and Andre were both really great to work with, and I look forward to squashing more bugs!

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d376cac28778
Remove support for Date.prototype.toLocaleFormat. r=gandalf, r=anba

Comment 50

[Attila Craciun [:arny]]

https://hg.mozilla.org/mozilla-central/rev/d376cac28778

Comment 51

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thank you so much Rofael! As :waldo said, this is a very valuable change for us. You managed to remove a function that was consistently working against everything else that we have in SpiderMonkey and causing troubles for years.

I enjoyed mentoring you through this and would be happy to help you in your future patches!. If you'll ever need me, I'm on IRC or you can NI me :)

Arthur: I'm keeping the NI on you because I believe that nsAppRunner's OverrideDefaultLocaleIfNeeded can and should be removed now, but we'd like to check with you first.

Comment 52

[Florian Scholz (Open Web Docs)]

Developer release notes
https://developer.mozilla.org/en-US/Firefox/Releases/58#JavaScript

Reference pages
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleFormat
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Deprecated_toLocaleFormat

Compat data
https://github.com/mdn/browser-compat-data/pull/589