Bug 31802 – Switch Chrome on Mac to Core Text APIs

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Jeremy Moskovich]

http://trac.webkit.org/changeset/50259/trunk caused a regression in Arabic text rendering on Chrome under10.6 described at http://crbug.com/27195 .

Chrome uses the ATSUI APIs rather than Core Text on both 10.5 & 10.6 for historical reasons.

ATSUI is deprecated on 10.6 in favor of Core Text so it makes sense to switch over the rendering APIs, rather than working around them in the ATSUI interface code.

[Jeremy Moskovich]

Created attachment 43703 [details]
Switch Chrome/Mac from ATSUI -> Core TExt

[Jeremy Moskovich]

Created attachment 43704 [details]
Switch Chrome/Mac from ATSUI -> Core Text

Fix Typo

[Mark Rowe (bdash)]

Comment on attachment 43704 [details]
Switch Chrome/Mac from ATSUI -> Core Text

> diff --git a/WebCore/platform/graphics/SimpleFontData.h b/WebCore/platform/graphics/SimpleFontData.h
> index 387a5c7..eb31f66 100644
> --- a/WebCore/platform/graphics/SimpleFontData.h
> +++ b/WebCore/platform/graphics/SimpleFontData.h
> @@ -115,7 +115,7 @@ public:
>      virtual String description() const;
>  #endif
>  
> -#if PLATFORM(MAC)
> +#if PLATFORM(DARWIN)
>      NSFont* getNSFont() const { return m_platformData.font(); }
>  #endif

NSFont is an AppKit level API, well above the Darwin layer.  It makes no sense to bracket it with “PLATFORM(DARWIN)”.

I’d be wary of using the Core Text code path on Leopard. It has received virtually no testing; last I tested it, on Leopard, it was slower than the ATSUI code path; and it requires you to use SPI.

[Jeremy Moskovich]

Dan: Thanks!  Since we compile a single binary for 10.5/10.6 are you OK with switching APIs at runtime rather than compile time?

Would you want such a change to be Chrome-only or are you OK with making that change on all platforms?

[Mark Rowe (bdash)]

(In reply to comment #5)
> Dan: Thanks!  Since we compile a single binary for 10.5/10.6 are you OK with
> switching APIs at runtime rather than compile time?

In most cases we know at compile time which implementation will be used.  We shouldn’t introduce overhead in to those cases.

> 
> Would you want such a change to be Chrome-only or are you OK with making that
> change on all platforms?

There’s nothing Chrome-specific about such a change.  It’s specific to building on one version of Mac OS X with a deployment target of an older version.  The code should be written as such.

[Jeremy Moskovich]

Created attachment 44073 [details]
Switch Chrome/Mac from ATSUI -> Core Text

mrowe: fixed.

After some testing and further discussion I'm going to try committing this, if we see regressions or other issues on our perf bots I'll revert and will look into toggling at runtime.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44073 [details] without any errors.

[Jeremy Moskovich]

Created attachment 44077 [details]
Switch Chrome/Mac from ATSUI -> Core Text

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44077 [details] without any errors.

[Jeremy Moskovich]

Created attachment 44078 [details]
Switch Chrome/Mac from ATSUI -> Core Text

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44078 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 44078 [details]
Switch Chrome/Mac from ATSUI -> Core Text

I don't see how this is different from the version Mark r-'d?

[Jeremy Moskovich]

Eric: If you look at SimpleFontData.h the version Mark r-ed changed
PLATFORM(MAC) -> PLATFORM(DARWIN)

The current patch uses:
PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))

The Changelog comment has also been updated to reflect this.

[Eric Seidel (no email)]

Comment on attachment 44078 [details]
Switch Chrome/Mac from ATSUI -> Core Text

OK.  I was reading comment 6 and ending up confused. :)

So long as this is the current approved method for doing deployment target stuff.  I thought that Mark Mentovai recently added some DEPLOY_ON macros or similar.  But I guess this is actually a building on question and not a deploy on question?  Since these symbols are present in both 10.5 and 10.6, they're just only public in 10.6.

[Jeremy Moskovich]

Comment on attachment 44078 [details]
Switch Chrome/Mac from ATSUI -> Core Text

Clear commit-queue, patch landed manually as r51633

[Jeremy Moskovich]

Created attachment 44468 [details]
Add optional ATSUI/Core Text runtime switch.

We observed the slowdown Dan was referring to on the Chrome perf bots and rolled back the previous patch.

This patch adds the ability to define both USE(ATSUI) and USE(CORE_TEXT) in which case the API is toggled at runtime.  If a port defines only one of these, things should work exactly as they do today.

I made sure that indexAt() and collectComplexTextRunsForCharacters() are indeed inlined by the compiler so there should be virtually no runtime hit for this patch.

[WebKit Review Bot]

Attachment 44468 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/mac/ComplexTextController.cpp:280:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/mac/ComplexTextController.cpp:280:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/mac/ComplexTextController.cpp:304:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 3

[Jeremy Moskovich]

Regarding the stylebot warnings:
ComplexTextController.cpp#280: This matches the example code for CTGetCoreTextVersion.
ComplexTextController#304: This isn't technically a style violation in my reading of the guide and IMHO the current code is a bit more readable.

I can change these if a reviewer feels strongly on the subject.

In summary: The review? flag still stands.

[Adam Barth]

It's up to whoever reviews your patch, but I'd ask you to change ComplexTextController.cpp#280.  I'm not sure what the sample code has to do with anything.  As for ComplexTextController#304, that's a grayer area, IMHO.

[Jeremy Moskovich]

Created attachment 44532 [details]
Add optional ATSUI/Core Text runtime switch

Fixed 2 style warnings, per abarth's comment.

Can't remove the == 0 since gcc warns that "the address of ‘uint32_t CTGetCoreTextVersion()’ will always evaluate as ‘true’" and Chrome compiles with -Werror.

[WebKit Review Bot]

Attachment 44532 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/mac/ComplexTextController.cpp:280:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1

[Eric Seidel (no email)]

Comment on attachment 44532 [details]
Add optional ATSUI/Core Text runtime switch

I dont' think the ALWAYS_INLINE actually does anything here:
 90         ALWAYS_INLINE CFIndex indexAt(size_t i) const;

One of these days we should change these to take an ENUM which is RTL vs. LTR, as that would make all the callsites easier to read:
 101         void createTextRunFromFontDataATSUI(bool ltr);
(Yes, totally out of scope for this patch.)

In general this looks fine.  It would be nice to see mitz's stamp of approval on this, but I feel pretty confident this is correct.

Lets give mitz a few days to comment in case he's just been swamped.

[Jeremy Moskovich]

Committed as r52067

Eric: Empirically ALWAYS_INLINE does appear to cause the function to be inlined, I was also surprised it made a difference :|