Bug 104410

Summary:

[Chromium-Android] Use harfbuzz-ng instead of harfbuzz-old on Android

Product:

WebKit

Reporter:

Xianzhu Wang <wangxianzhu>

Component:

Layout and Rendering

Assignee:

Xianzhu Wang <wangxianzhu>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, bashi, behdad, eric, glenn, jshin, laszlo.gombos, leviw, peter, phajdan.jr, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Android

OS:

Android

Attachments:

Description	Flags
Patch	none

Xianzhu Wang

Reported 2012-12-07 16:12:18 PST

It's really critical to switch to harfbuzz-ng because so many bugs are resolved with the switch.

Attachments
Patch (5.23 KB, patch) 2012-12-10 13:39 PST, Xianzhu Wang	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Xianzhu Wang

Comment 1 2012-12-10 13:39:59 PST

Created attachment 178624 [details] Patch

Xianzhu Wang

Comment 2 2012-12-10 13:43:41 PST

Just enabled harfbuzz-ng in gyp without any C++ code change. The original Android-specific code in ComplexTextControllerHarfBuzz.cpp (depending on harfbuzz-old) is not needed in the new code path because we already have proper font fallback mechanism on in Skia for Android and used it in WebKit. Perhaps we should cleanup the obsolete code if no one uses harfbuzz-old.

Peter Beverloo

Comment 3 2012-12-12 04:01:36 PST

Do we want this before the M25 branch point? It seems like a risky change to do on such short notice..

Xianzhu Wang

Comment 4 2012-12-12 09:05:51 PST

(In reply to comment #3) > Do we want this before the M25 branch point? It seems like a risky change to do on such short notice.. This is not urgent. I think we can land it after M25 branch point.

Eric Seidel (no email)

Comment 5 2012-12-12 10:30:20 PST

I doubt its particularly risky, given that linux has been on ng for several months iirc. But there is also no hurry in my view.

Eric Seidel (no email)

Comment 6 2012-12-12 10:32:10 PST

Comment on attachment 178624 [details] Patch Lgtm, but be sure to coordinate with peter wrt when this should be landed.

Tony Chang

Comment 7 2012-12-12 11:24:29 PST

Is anyone still on old harfbuzz after this change? Can we delete the old harfbuzz code (there was some extra abstraction to handle both).

Peter Beverloo

Comment 8 2012-12-13 06:28:55 PST

(In reply to comment #5) > I doubt its particularly risky, given that linux has been on ng for several months iirc. But there is also no hurry in my view. We used to have a few modifications in harfbuzz in regards to memory usage, switching Android to harfbuzz-ng just before the branch point can have unexpected side-effects unless it's really thoroughly tested. While it's not a particularly risky change, it's safer to do it in the M26 period. The amount of issues we have with M25 is rather high as it is. Landing this on Tuesday morning (assuming branch occurs on Monday) is perfectly fine with me. Thanks for doing this, Xianzhu!

WebKit Review Bot

Comment 9 2012-12-21 15:28:57 PST

Comment on attachment 178624 [details] Patch Clearing flags on attachment: 178624 Committed r138401: <http://trac.webkit.org/changeset/138401>

WebKit Review Bot

Comment 10 2012-12-21 15:29:01 PST

All reviewed patches have been landed. Closing bug.

Behdad Esfahbod

Comment 11 2012-12-21 16:22:43 PST

Awesome! Thanks everyone.

Note You need to log in before you can comment on or make changes to this bug.