1428774 - nsBidiUtils::HasRTLChars() does unnecessary surrogate handling

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Henri Sivonen (:hsivonen)]

The supplementary RTL ranges have the property that it's sufficient to examine the high surrogate to determine if an astral character is RTL.

Once 1428771 is fixed, nsBidiUtils::HasRTLChars() should, therefore, use loop by code unit and check each code unit using UCS2_CHAR_IS_BIDI instead of checking for surrogate pairs.

Downside: Certain unpaired high surrogates will be classified as RTL. This should not be a practical problem.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1428774 - Avoid surrogate math in HasRTLChars().

Behavior change: Certain unpaired high surrogates will result in
the text being considered RTL where that wasn't the case before.

Review commit: https://reviewboard.mozilla.org/r/211268/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211268/

Comment 2

[Henri Sivonen (:hsivonen)]

Aside: I intend to SIMD-accelerate this in Rust as part of bug 1402247.

Comment 3

[Henri Sivonen (:hsivonen)]

Comment on attachment 8940990 [details]
Bug 1428774 - Avoid surrogate math in HasRTLChars().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211268/diff/1-2/

Comment 4

[Henri Sivonen (:hsivonen)]

Comment on attachment 8940990 [details]
Bug 1428774 - Avoid surrogate math in HasRTLChars().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211268/diff/2-3/

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 8940990 [details]
Bug 1428774 - Avoid surrogate math in HasRTLChars().

https://reviewboard.mozilla.org/r/211268/#review220458

Seems OK -- the potential change in behavior for certain unpaired surrogates might seem a bit fishy, but I can't think of any reason it actually matters here.

Comment 6

[Henri Sivonen (:hsivonen)]

This is indeed a perf win:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=fd3b069d070ffb5f5314b1f91ddca6090434fab7&newProject=try&newRevision=1d611c4897eb8b6a1115ef71b087bd93c9027620&framework=6

Thanks for the r+.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e903401fe83
Avoid surrogate math in HasRTLChars(). r=jfkthame

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/1e903401fe83