Bug 92737 - Enable kerning on Android
Summary: Enable kerning on Android
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Android
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-07-31 04:03 PDT by Iain Merrick
Modified: 2012-08-09 11:41 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.75 KB, patch)
2012-08-08 10:25 PDT, Iain Merrick
no flags Details | Formatted Diff | Diff
Patch (1.76 KB, patch)
2012-08-09 05:21 PDT, Iain Merrick
no flags Details | Formatted Diff | Diff
Patch (1.77 KB, patch)
2012-08-09 05:48 PDT, Iain Merrick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Iain Merrick 2012-07-31 04:03:59 PDT
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
Comment 1 Iain Merrick 2012-08-08 10:25:19 PDT
Created attachment 157245 [details]
Patch
Comment 2 Iain Merrick 2012-08-08 10:29:04 PDT
+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.
Comment 3 Peter Beverloo 2012-08-08 10:29:50 PDT
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].
Comment 4 Adam Barth 2012-08-08 10:40:28 PDT
Comment on attachment 157245 [details]
Patch

Please address Peter's comments before landing.
Comment 5 Iain Merrick 2012-08-09 05:20:52 PDT
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
Comment 6 Iain Merrick 2012-08-09 05:21:32 PDT
Created attachment 157443 [details]
Patch
Comment 7 Peter Beverloo 2012-08-09 05:36:04 PDT
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)."?
Comment 8 Iain Merrick 2012-08-09 05:48:15 PDT
Created attachment 157456 [details]
Patch
Comment 9 Iain Merrick 2012-08-09 05:48:33 PDT
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.
Comment 10 Peter Beverloo 2012-08-09 05:49:55 PDT
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 :-).
Comment 11 WebKit Review Bot 2012-08-09 11:41:20 PDT
Comment on attachment 157456 [details]
Patch

Clearing flags on attachment: 157456

Committed r125189: <http://trac.webkit.org/changeset/125189>
Comment 12 WebKit Review Bot 2012-08-09 11:41:25 PDT
All reviewed patches have been landed.  Closing bug.