nsBidiUtils::HasRTLChars() does unnecessary surrogate handling

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

a year ago
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 hidden (mozreview-request)
Assignee

Updated

a year ago
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Assignee

Comment 2

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

Updated

a year ago
Summary: nsBidiUtils::HasRTLChars() does useless surrogate handling → nsBidiUtils::HasRTLChars() does unnecessary surrogate handling
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Blocks: 1431025

Comment 5

a year ago
mozreview-review
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.
Attachment #8940990 - Flags: review?(jfkthame) → review+
Assignee

Updated

a year ago
Depends on: 1432728

Comment 7

a year ago
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e903401fe83
Avoid surrogate math in HasRTLChars(). r=jfkthame

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1e903401fe83
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.