Bug 31802 - Switch Chrome on Mac to Core Text APIs
Summary: Switch Chrome on Mac to Core Text APIs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-23 05:12 PST by Jeremy Moskovich
Modified: 2009-12-13 05:37 PST (History)
7 users (show)

See Also:


Attachments
Switch Chrome/Mac from ATSUI -> Core TExt (3.32 KB, patch)
2009-11-23 05:21 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Switch Chrome/Mac from ATSUI -> Core Text (3.32 KB, patch)
2009-11-23 05:22 PST, Jeremy Moskovich
mrowe: review-
Details | Formatted Diff | Diff
Switch Chrome/Mac from ATSUI -> Core Text (3.34 KB, patch)
2009-12-01 07:10 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Switch Chrome/Mac from ATSUI -> Core Text (3.36 KB, patch)
2009-12-01 08:02 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Switch Chrome/Mac from ATSUI -> Core Text (3.32 KB, patch)
2009-12-01 08:04 PST, Jeremy Moskovich
eric: review+
Details | Formatted Diff | Diff
Add optional ATSUI/Core Text runtime switch. (21.09 KB, patch)
2009-12-08 05:48 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Add optional ATSUI/Core Text runtime switch (21.07 KB, patch)
2009-12-09 05:42 PST, Jeremy Moskovich
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Moskovich 2009-11-23 05:12:22 PST
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.
Comment 1 Jeremy Moskovich 2009-11-23 05:21:01 PST
Created attachment 43703 [details]
Switch Chrome/Mac from ATSUI -> Core TExt
Comment 2 Jeremy Moskovich 2009-11-23 05:22:52 PST
Created attachment 43704 [details]
Switch Chrome/Mac from ATSUI -> Core Text

Fix Typo
Comment 3 Mark Rowe (bdash) 2009-11-23 07:44:30 PST
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)”.
Comment 4 mitz 2009-11-23 09:28:58 PST
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.
Comment 5 Jeremy Moskovich 2009-11-23 09:34:16 PST
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?
Comment 6 Mark Rowe (bdash) 2009-11-24 11:59:01 PST
(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.
Comment 7 Jeremy Moskovich 2009-12-01 07:10:41 PST
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.
Comment 8 WebKit Review Bot 2009-12-01 07:15:55 PST
style-queue ran check-webkit-style on attachment 44073 [details] without any errors.
Comment 9 Jeremy Moskovich 2009-12-01 08:02:05 PST
Created attachment 44077 [details]
Switch Chrome/Mac from ATSUI -> Core Text
Comment 10 WebKit Review Bot 2009-12-01 08:02:40 PST
style-queue ran check-webkit-style on attachment 44077 [details] without any errors.
Comment 11 Jeremy Moskovich 2009-12-01 08:04:45 PST
Created attachment 44078 [details]
Switch Chrome/Mac from ATSUI -> Core Text
Comment 12 WebKit Review Bot 2009-12-01 08:08:04 PST
style-queue ran check-webkit-style on attachment 44078 [details] without any errors.
Comment 13 Eric Seidel (no email) 2009-12-02 13:55:24 PST
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?
Comment 14 Jeremy Moskovich 2009-12-02 14:02:51 PST
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.
Comment 15 Eric Seidel (no email) 2009-12-02 14:05:46 PST
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.
Comment 16 Jeremy Moskovich 2009-12-03 05:53:37 PST
Comment on attachment 44078 [details]
Switch Chrome/Mac from ATSUI -> Core Text

Clear commit-queue, patch landed manually as r51633
Comment 17 Jeremy Moskovich 2009-12-08 05:48:10 PST
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.
Comment 18 WebKit Review Bot 2009-12-08 05:51:00 PST
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
Comment 19 Jeremy Moskovich 2009-12-08 06:02:31 PST
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.
Comment 20 Adam Barth 2009-12-08 10:01:59 PST
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.
Comment 21 Jeremy Moskovich 2009-12-09 05:42:17 PST
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.
Comment 22 WebKit Review Bot 2009-12-09 10:12:57 PST
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
Comment 23 Eric Seidel (no email) 2009-12-11 16:41:09 PST
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.
Comment 24 Jeremy Moskovich 2009-12-13 05:37:10 PST
Committed as r52067

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