Closed
Bug 818634
Opened 12 years ago
Closed 7 years ago
Remove support for Date.prototype.toLocaleFormat
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: bruant.d, Assigned: rofael, Mentored)
References
(Blocks 2 open bugs)
Details
(Keywords: dev-doc-complete, good-first-bug, site-compat)
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
zbraniecki
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•12 years ago
|
||
It's used here and there in Firefox:
http://mxr.mozilla.org/mozilla-central/search?string=toLocaleFormat
Is it used in addons?
Comment 2•12 years ago
|
||
not only in Firefox: http://mxr.mozilla.org/comm-central/search?string=toLocaleFormat
Comment 3•12 years ago
|
||
> Is it used in addons?
https://mxr.mozilla.org/addons/search?string=toLocaleFormat
Found 366 matching lines in 159 files
Comment 4•12 years ago
|
||
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.
Keywords: addon-compat
Updated•10 years ago
|
Assignee: general → nobody
Updated•10 years ago
|
Keywords: dev-doc-needed,
site-compat
Comment 5•10 years ago
|
||
this could be my second bug.So could you please provide more information what exactly is to be done?
Comment 6•10 years ago
|
||
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•9 years ago
|
||
Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/date-prototype-tolocaleformat-will-be-removed/
Comment 8•8 years ago
|
||
14 uses left in code, but more in testcases.
Comment 9•8 years ago
|
||
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).
Depends on: 1344625
Comment 10•8 years ago
|
||
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•7 years ago
|
||
There is no more toLocaleFormat uses in mozilla-central.
We should be good to remove it completely!
Updated•7 years ago
|
Blocks: post-57-api-changes
Comment 12•7 years ago
|
||
(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•7 years ago
|
||
(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.
Updated•7 years ago
|
Keywords: addon-compat
Comment 14•7 years ago
|
||
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•7 years ago
|
||
(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•7 years ago
|
||
(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•7 years ago
|
||
Warnings were already added in bug 1299900, but we don't yet collect telemetry because of bug 1331909.
Comment 18•7 years ago
|
||
> 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•7 years ago
|
||
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•7 years ago
|
||
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.
Mentor: gandalf
Keywords: good-first-bug
Assignee | ||
Comment 21•7 years ago
|
||
(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)?
Flags: needinfo?(gandalf)
Comment 22•7 years ago
|
||
(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.
Flags: needinfo?(gandalf)
Assignee | ||
Comment 23•7 years ago
|
||
Sounds good! Could I have it?
Comment 24•7 years ago
|
||
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!
Assignee | ||
Comment 25•7 years ago
|
||
This look good? mach build was successful.
Attachment #8919979 -
Flags: review?(gandalf)
Comment 26•7 years ago
|
||
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.
Updated•7 years ago
|
Assignee: nobody → me
Status: NEW → ASSIGNED
Assignee | ||
Comment 27•7 years ago
|
||
(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?
Assignee | ||
Comment 28•7 years ago
|
||
> 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.
Assignee | ||
Comment 29•7 years ago
|
||
Attachment #8919979 -
Attachment is obsolete: true
Attachment #8919979 -
Flags: review?(gandalf)
Attachment #8920201 -
Flags: review?(gandalf)
Comment 30•7 years ago
|
||
Comment on attachment 8920201 [details] [diff] [review]
818634-2.patch
Thanks! That looks good to me!
Redirecting r? to anba for SM sanity.
Attachment #8920201 -
Flags: review?(gandalf)
Attachment #8920201 -
Flags: review?(andrebargull)
Attachment #8920201 -
Flags: feedback+
Comment 31•7 years ago
|
||
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.
Attachment #8920201 -
Flags: review?(andrebargull) → review-
Comment 32•7 years ago
|
||
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
Flags: needinfo?(arthuredelstein)
Assignee | ||
Comment 33•7 years ago
|
||
(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•7 years ago
|
||
> 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•7 years ago
|
||
(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? :-)
Assignee | ||
Comment 36•7 years ago
|
||
(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•7 years ago
|
||
> 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!
Assignee | ||
Comment 38•7 years ago
|
||
(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.
Assignee | ||
Comment 39•7 years ago
|
||
To refresh from the weekend, this is everything I've done so far.
Attachment #8920201 -
Attachment is obsolete: true
Attachment #8921152 -
Flags: review?(gandalf)
Attachment #8921152 -
Flags: review?(andrebargull)
Comment 40•7 years ago
|
||
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
Attachment #8921152 -
Flags: review?(andrebargull) → review+
Comment 41•7 years ago
|
||
@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
Assignee | ||
Comment 42•7 years ago
|
||
I've made your changes in comment #40, Andre. I'll tag Zibi in this one to get his look.
Attachment #8921152 -
Attachment is obsolete: true
Attachment #8921152 -
Flags: review?(gandalf)
Attachment #8921699 -
Flags: review?(gandalf)
Comment 43•7 years ago
|
||
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
Attachment #8921699 -
Flags: review?(gandalf) → review+
Assignee | ||
Comment 44•7 years ago
|
||
Comment to nsAppRunner added and XPCLocale comment (hopefully) fixed, kind of funny to think my biggest hurdles have been with comments.
Attachment #8921699 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8922082 -
Flags: review?(gandalf)
Comment 45•7 years ago
|
||
Comment on attachment 8922082 [details] [diff] [review]
818634-5.patch
That looks good!
Feel free to land (or add `checkin-needed` keyword)
Attachment #8922082 -
Flags: review?(gandalf) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 46•7 years ago
|
||
Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/date-prototype-tolocaleformat-has-been-removed/
Comment 47•7 years ago
|
||
(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!
Assignee | ||
Comment 48•7 years ago
|
||
(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•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d376cac28778
Remove support for Date.prototype.toLocaleFormat. r=gandalf, r=anba
Keywords: checkin-needed
Comment 50•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 51•7 years ago
|
||
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•7 years ago
|
||
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
Keywords: dev-doc-needed → dev-doc-complete
Updated•5 years ago
|
Flags: needinfo?(arthur)
You need to log in
before you can comment on or make changes to this bug.
Description
•