Created attachment 157245 [details] Patch

+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 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 on attachment 157245 [details] Patch Please address Peter's comments before landing.

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

Created attachment 157443 [details] Patch

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)."?

Created attachment 157456 [details] Patch

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 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 on attachment 157456 [details] Patch Clearing flags on attachment: 157456 Committed r125189: <http://trac.webkit.org/changeset/125189>

All reviewed patches have been landed. Closing bug.