1308359 - Remove our impl of bidi engine completely and use ICU directly

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P3)

Description

[Xidorn Quan [:xidorn] UTC+11]

In bug 924851, we switch to use ICU for bidi when it is available. Since it is likely that we will have ICU available everywhere eventually, we want to remove our own bidi engine at some point, as well as the glue layer, and use ICU directly.

It would enable us to use ICU functions we didn't adopt into our own bidi engine.

Comment 1

[Julien Cristau [:jcristau]]

Too late for firefox 52, mass-wontfix.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: patch 1 - Remove the nsBidi_noICU implementation of nsBidi, and rename the sources for the _ICU version to the base nsBidi.{h,cpp} filenames

Now that mozilla-central is firefox-58, I think we can go ahead and do this cleanup.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - Move trivial ICU-wrapper nsBidi methods to the header file as inlines, and remove unnecessary nsresult return values from methods that cannot fail

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8910867 [details] [diff] [review]
patch 1 - Remove the nsBidi_noICU implementation of nsBidi, and rename the sources for the _ICU version to the base nsBidi.{h,cpp} filenames

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

It would be great if you used file move in the patch for nsBidi_ICU.h -> nsBidi.h as well just like what you did for the cpp file. Anyway, it looks good to me.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8910870 [details] [diff] [review]
patch 2 - Move trivial ICU-wrapper nsBidi methods to the header file as inlines, and remove unnecessary nsresult return values from methods that cannot fail

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

::: layout/base/nsBidi.h
@@ +57,5 @@
>     *      Any other value between 0 and <code>NSBIDI_MAX_EXPLICIT_LEVEL</code>
>     *      is also valid, with odd levels indicating RTL.
>     */
> +  nsresult
> +  SetPara(const char16_t* aText, int32_t aLength, nsBidiLevel aParaLevel)

I don't think you need to put return type in its own line for function definitions inside class declaration.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf8977ee7a0
patch 1 - Remove the nsBidi_noICU implementation of nsBidi, and rename the sources for the _ICU version to the base nsBidi.{h,cpp} filenames. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/593bddd0b6bd
patch 2 - Move trivial ICU-wrapper nsBidi methods to the header file as inlines, and remove unnecessary nsresult return values from methods that cannot fail. r=xidorn

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5bf8977ee7a0
https://hg.mozilla.org/mozilla-central/rev/593bddd0b6bd