Spun off from bug 924851 comment 19. nsBidi::GetCharTypeAt() returns a value from the internal direction-property array, which will not be possible for an ICU-based replacement of nsBidi (this array is an internal detail of the implementation of the bidi algorithm, and its content are not exposed in the ubidi API). The only caller of this method is nsBidiPresUtils::CalculateCharType. It appears that CalculateCharType doesn't really need to access this internal type, because it then follows up with its own processing for the numeric types that may be changed by the algorithm. So we can just look up the standard category instead of this internal value from inside the bidi algorithm.

Tryserver says this passes current tests (which exercise bidi pretty hard - in particular, a bunch of the tests in layout/reftests/bidi/numeral/ will go through this code), so I'm reasonably confident it should be OK: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40b46e8b1352.

Comment on attachment 8798140 [details] [diff] [review] Remove the nsBidi::GetCharTypeAt() method and use plain GetBidiCat() instead Review of attachment 8798140 [details] [diff] [review]: ----------------------------------------------------------------- Thanks.

Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a3124188da80 Remove the nsBidi::GetCharTypeAt() method and use plain GetBidiCat() instead. r=xidorn

<driveby>I suspect you can go a stage further and remove (most if not all) of CalculateCharType altogether. IIRC it was a workaround used in an early stage of the bidi code when we supported platforms with no native bidi support and had to reverse right-to-left runs manually.</driveby>

It seems to me that without CalculateCharType, we wouldn't have the right char type for number characters, and consequently the handling in FormatUnicodeText may not be correct, unless that code isn't useful anymore. Is that what you meant?

I added the weasel "(most if not all)" because we possibly do still need to assign the aCharType and aPrevCharType to handle some cases of numerical shaping, but I don't think we need anything else, and maybe even that could be moved into FormatUnicodeText or HandleNumbers so we don't need to loop through the text twice.

That sounds reasonable. Filed bug 1308356.