Closed Bug 1490609 Opened 6 years ago Closed 6 years ago

Add JS_EncodeStringToASCII to CharacterEncoding.h

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(1 file, 1 obsolete file)

Follow-up from bug 1485066.
Attached patch bug1490609.patch (obsolete) — Splinter Review
I've named the JSAPI function JS_EncodeStringToASCII with upper case "ASCII" for consistency with other uses of ASCII in CharacterEncoding.h. But the vm-internal function was named EncodeAscii with title case "Ascii", because that seems the preferred form nowadays. 

js/src/builtin/TestingFunctions.cpp
- It probably makes sense to require ASCII strings in SetTimeZone, so use JS_EncodeStringToASCII there to remove another caller of JS_EncodeStringToLatin1.
- Also change SetDefaultLocale to perform two separate checks for ASCII contents and BCP 47 valid characters to remove another call to JS_EncodeStringToLatin1.

js/src/builtin/intl/CommonFunctions.cpp
- Change js::intl::EncodeLocale to call EncodeAscii, because the input is required to be an ASCII string.

js/src/jsnum.cpp
- Use EncodeAscii as mentioned in bug 1485066.

js/src/proxy/ScriptedProxyHandler.cpp
- We can also use EncodeAscii here, because the handler name is always an ASCII string.

js/src/vm/Debugger.cpp
- Disallow non-ASCII strings for Debugger::ObjectQuery to avoid a lossy JS_EncodeStringToLatin1 call and because class names are required to be ASCII strings.

js/src/vm/StringType.cpp
- Drive-by change: Change a loop over |char*| in debug-code to call |JS::StringIsASCII| instead.
Attachment #9008346 - Flags: review?(jwalden+bmo)
Comment on attachment 9008346 [details] [diff] [review]
bug1490609.patch

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

::: js/src/builtin/intl/CommonFunctions.cpp
@@ +100,5 @@
> +    // (Lambdas can't be placed inside MOZ_ASSERT, so move the checks in an
> +    // #ifdef block.)
> +    auto alnumOrDash = [](char c) { return mozilla::IsAsciiAlphanumeric(c) || c == '-'; };
> +    MOZ_ASSERT_IF(chars, mozilla::IsAsciiAlpha(chars[0]));
> +    MOZ_ASSERT_IF(chars, std::all_of(chars.get(), chars.get() + locale->length(), alnumOrDash));

I'd prefer

  if (chars) {
    MOZ_ASSERT(...);
    MOZ_ASSERT(...);
  }

partly because MOZ_ASSERT_IF's error message production aesthetics are not the greatest, if memory servces, partly because you're repeating chars and _IF, partly because there's enough implicit control flow the way you have it that it's better to make it explicit and more readable.

::: js/src/vm/Debugger.cpp
@@ +5070,5 @@
>                  return false;
>              }
> +            JSLinearString* str = cls.toString()->ensureLinear(cx);
> +            if (!str)
> +                return false;

ugh this stupid style :-(

::: js/src/vm/StringType.cpp
@@ +1042,5 @@
>  
>  bool
> +js::StringIsAscii(JSLinearString* str)
> +{
> +    auto containsOnlyAsciiCharacters = [](auto* chars, size_t length) {

If this can be |const auto*|, prefer that.
Attachment #9008346 - Flags: review?(jwalden+bmo) → review+
Attached patch bug1490609.patchSplinter Review
Updated per review comments, carrying r+.
Attachment #9008346 - Attachment is obsolete: true
Attachment #9008700 - Flags: review+
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a0a5ee29697
Add JS_EncodeStringToASCII to CharacterEncoding.h. r=Waldo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4a0a5ee29697
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: