1937538 - (icu_harfbuzz) Use ICU4X as the Unicode back end for HarfBuzz

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement)

Description

[Henri Sivonen (:hsivonen)]

We should use the icu_harfbuzz crate to make the Unicode Database operations that HarfBuzz has callbacks for to go to ICU4X instead of ICU4C.

We probably shouldn't involve Diplomat FFI here at all. Let's write a single FFI function for gfxHarfBuzzShaper::Initialize call and have that FFI function connect the callbacks as seen in https://github.com/unicode-org/icu4x/blob/main/tutorials/rust/harfbuzz/src/main.rs .

For performance and API settling down, it probably makes sense to wait until we have ICU4X 2.0 in mozilla-central before doing this.

Comment 1

[Henri Sivonen (:hsivonen)]

Specifically, this is about replacing this part of the current HarfBuzz initialization:
https://searchfox.org/mozilla-central/rev/937d8e66a5bfe2fc8eb9aa0080629ce7f8dd6cc6/gfx/thebes/gfxHarfBuzzShaper.cpp#1244-1257

Comment 2

[Henri Sivonen (:hsivonen)]

jfkthame, do we need to continue to have the fix for bug 1012365? It seems that Firefox for Android now requires Android 5 or newer, the buggy font was on a Samsung device running Android 4.3. There was a comment about Android 4.4.2 being OK, but it's unclear to me if that was Samsung's Android or some other Android 4.4.2.

Comment 3

[Henri Sivonen (:hsivonen)]

The font appears not to be included on Samsung A54 G5, according to Jens.

Comment 4

[Jonathan Kew [:jfkthame]]

I don't have any current information regarding this. As far as I can tell, Samsung has shifted to their "SamsungOne" font as their primary font family, and Devanagari is mentioned as one of the writing systems it supports, but this doesn't seem to be readily available (short of getting a device that includes it), so I can't currently check it.

(Most likely it's all OK now, but my niggling concern is the possibility that they could have just taken their old SamsungDevanagari font and merged it into the SamsungOne "family", without necessarily fixing things.)

Maybe we could start by removing the hack from Nightly, and then ask a few people with Samsung devices to check whether the test file from bug 1012365 still renders the character.

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1937538 - Use ICU4X as the Unicode back end for HarfBuzz.

Comment 6

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=143795

(In reply to Jonathan Kew [:jfkthame] from comment #4)

Maybe we could start by removing the hack from Nightly, and then ask a few people with Samsung devices to check whether the test file from bug 1012365 still renders the character.

Thanks. OK. I'll try that.

Comment 7

[Henri Sivonen (:hsivonen)]

(I will need to figure out what to do about harfbuzz-sys containing a copy of the source of HarfBuzz.)

Comment 8

[Henri Sivonen (:hsivonen)]

https://perf.compare/compare-lando-results?baseLando=143820&newLando=143821&baseRepo=try&newRepo=try&framework=13
Base revision's try run: https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=143820
Local revision's try run: https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=143821

Comment 9

[Henri Sivonen (:hsivonen)]

It's bad that harfbuzz-sys contains a copy of the source code of HarfBuzz itself.

Is there some directive that I can set to make mach vendor rust filter it out, or should I fork the repo, delete the copy of HarfBuzz in the fork, and then vendor from the fork?

Comment 10

[Mike Hommey [:glandium] OoO until Jun 18]

Is there some directive that I can set to make mach vendor rust filter it out

There isn't

Comment 11

[Henri Sivonen (:hsivonen)]

Try run to check that it builds for various targets given that it previously built for desktop Linux but not for Android:
https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=180102

Comment 12

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=181881

Comment 13

[Henri Sivonen (:hsivonen)]

Perhaps getting around the Lando bug this time:
https://treeherder.mozilla.org/jobs?repo=try&revision=b7d9f2a3927327574c4a02f34e7a6622cd446ce9

Comment 14

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/jobs?repo=try&revision=189f189c9db6e10e6dc35647c0430d2e3a65f235

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e4bfac06e17a
https://hg.mozilla.org/integration/autoland/rev/2471982e2146
Use ICU4X as the Unicode back end for HarfBuzz. r=supply-chain-reviewers,afranchuk,glandium,sylvestre,jfkthame

Comment 16

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/2471982e2146