Warn when Date.prototype.toLocaleFormat is used

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

({dev-doc-complete, site-compat})

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 affected, firefox55 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
Prerequisite for bug 818634
(Assignee)

Updated

3 years ago
Depends on: 1313793
(Assignee)

Updated

3 years ago
Depends on: 1313794
(Assignee)

Updated

3 years ago
Depends on: 1313795
(Assignee)

Updated

3 years ago
Depends on: 1313796
(Assignee)

Updated

3 years ago
Depends on: 1313797
(Assignee)

Updated

3 years ago
Depends on: 1313798
(Assignee)

Updated

3 years ago
Depends on: 1313799
(Assignee)

Comment 2

3 years ago
Posted patch bug1299900-part1.patch (obsolete) — Splinter Review
Collect telemetry for Date.prototype.toLocaleFormat if a suitable replacement is available (i.e. if Intl isn't disabled).
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8808272 - Flags: review?(jdemooij)
(Assignee)

Comment 3

3 years ago
Posted patch bug1299900-part2.patch (obsolete) — Splinter Review
Warn when Date.prototype.toLocaleFormat is used (and Intl is available).
Attachment #8808274 - Flags: review?(jdemooij)
Comment on attachment 8808272 [details] [diff] [review]
bug1299900-part1.patch

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

::: js/src/jsdate.cpp
@@ +2805,5 @@
>      Rooted<DateObject*> dateObj(cx, &args.thisv().toObject().as<DateObject>());
>  
> +#if EXPOSE_INTL_API
> +    if (JSScript *script = cx->currentScript()) {
> +        const char *filename = script->filename();

Nit: * goes with the type, twice.
Attachment #8808272 - Flags: review?(jdemooij) → review+
Comment on attachment 8808274 [details] [diff] [review]
bug1299900-part2.patch

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

Thanks!

::: js/src/jsdate.cpp
@@ +2814,5 @@
> +        if (!JS_ReportErrorFlagsAndNumberASCII(cx, JSREPORT_WARNING, GetErrorMessage, nullptr,
> +                                               JSMSG_DEPRECATED_TOLOCALEFORMAT))
> +        {
> +            return false;
> +        }

Could use JS_ReportWarning here, but this gets the job done.
Attachment #8808274 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 6

2 years ago
Posted patch bug1299900.patch (obsolete) — Splinter Review
Updated to drop part 1 because of bug 1331909. Carrying r+ from jandem.
Attachment #8808272 - Attachment is obsolete: true
Attachment #8808274 - Attachment is obsolete: true
Attachment #8827918 - Flags: review+
(Assignee)

Updated

2 years ago
Depends on: 1332351
(Assignee)

Comment 7

2 years ago
Updated to apply cleanly on inbound. Carrying r+ from jandem.
Attachment #8827918 - Attachment is obsolete: true
Attachment #8844544 - Flags: review+

Comment 9

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22933322f28b
Warn about deprecated Date.prototype.toLocaleFormat method. r=jandem
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/22933322f28b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 11

2 years ago
André, I was under the impression that a somewhat harmless *warning* message was implemented there.
Sadly this is coming out as JS error and causing us massive test failures: Bug 1345832.
(Unless I'm completely wrong and the test failures come from elsewhere.)

Ah, well, we'll just have to address it quickly as was the intention ;-)
(Assignee)

Comment 12

2 years ago
(In reply to Jorg K (GMT+1) from comment #11)
> André, I was under the impression that a somewhat harmless *warning* message
> was implemented there.
> Sadly this is coming out as JS error and causing us massive test failures:
> Bug 1345832.
> (Unless I'm completely wrong and the test failures come from elsewhere.)

This happens when the "werror" context option is set [1]. Without that option, it should only print a warning message, but continue execution. But I guess the test environment sets "werror" to catch additional errors?

[1] http://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/js/src/jscntxt.cpp#419-421
I'm postponing writing a site compatibility note on this deprecation warning, because, as I wrote in Bug 818634, there's no simple alternative solution on Firefox for Android now. If the Intl API is enabled on Fennec during the 55 development cycle, I'll post a doc shortly.

Updated

2 years ago
Depends on: 1350679
You need to log in before you can comment on or make changes to this bug.