Bug 92737 – Enable kerning on Android

• Component: New Bugs
• Status: RESOLVED FIXED

[Iain Merrick]

Kerning was disabled in http://trac.webkit.org/changeset/121609, which includes this code:

#if OS(ANDROID)
  // Kerning does not currently work on Android.
  m_item.shaperFlags = HB_ShaperFlag_NoKerning;
#endif

[Iain Merrick]

Created attachment 157245 [details]
Patch

[Iain Merrick]

+abarth

Adam, you did the last change to this file, so if you have time could you take a look at this, please?

I'm having trouble tracking down what the original problem actually was. We had an old bug where the character spacing was messed up in text styled with "text-rendering: optimizelegibility", and that one is definitely fixed now.

I'll run all the downstream tests to check that this doesn't cause problems.

[Peter Beverloo]

Comment on attachment 157245 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157245&action=review

Looks good to me, although I'd like a clarification in the ChangeLog re: testing.

> Source/WebCore/ChangeLog:12
> +        Testing: existing UI tests (not all unforked yet)

Kerning is something that also comes up in layout tests, will this influence our results there?

> Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp:-83
> -#if OS(ANDROID)

Nit: since this change is specific to Chromium and doesn't touch other platforms, you could prefix the bug's subject with [Chromium].

[Adam Barth]

Comment on attachment 157245 [details]
Patch

Please address Peter's comments before landing.

[Iain Merrick]

Comment on attachment 157245 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157245&action=review

>> Source/WebCore/ChangeLog:12
>> +        Testing: existing UI tests (not all unforked yet)
> 
> Kerning is something that also comes up in layout tests, will this influence our results there?

Potentially yes, although most layout tests try to avoid dependencies on font metrics.

Simplified to "tests".

>> Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp:-83
>> -#if OS(ANDROID)
> 
> Nit: since this change is specific to Chromium and doesn't touch other platforms, you could prefix the bug's subject with [Chromium].

Done

[Iain Merrick]

Created attachment 157443 [details]
Patch

[Peter Beverloo]

Comment on attachment 157443 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157443&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Adam already r+'ed your patch :-). If you change this line to read "Reviewed by Adam Barth.", we can land it right now.

> Source/WebCore/ChangeLog:12
> +        Testing: existing tests (not all unforked yet)

The "not all unforked yet" doesn't really add information. Could you change this line to say "Covered by existing tests (layout and instrumentation tests)."?

[Iain Merrick]

Created attachment 157456 [details]
Patch

[Iain Merrick]

Comment on attachment 157443 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157443&action=review

>> Source/WebCore/ChangeLog:6
>> +        Reviewed by NOBODY (OOPS!).
> 
> Adam already r+'ed your patch :-). If you change this line to read "Reviewed by Adam Barth.", we can land it right now.

Oh, cool! Although it seems like that would allow me to submit anything, which is surely dodgy, so I assume there's a catch...

Done.

>> Source/WebCore/ChangeLog:12
>> +        Testing: existing tests (not all unforked yet)
> 
> The "not all unforked yet" doesn't really add information. Could you change this line to say "Covered by existing tests (layout and instrumentation tests)."?

Done.

[Peter Beverloo]

Comment on attachment 157456 [details]
Patch

The catch is that you need committer rights to set cq+ (or commit manually, of course), and we do trust our committers :-).

[WebKit Review Bot]

Comment on attachment 157456 [details]
Patch

Clearing flags on attachment: 157456

Committed r125189: <http://trac.webkit.org/changeset/125189>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.