Bug 145681 – [iOS] Arabic text styled with Georgia is rendered as boxes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Arabic text styled with Georgia is rendered as boxes

[Myles C. Maxfield]

Created attachment 254334 [details]
Patch

[Myles C. Maxfield]

Created attachment 254335 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/21169844>

Times New Roman has been working with Arabic for a while, but last time we tried allowing it, it was prohibitively slow. Has that changed?

[Myles C. Maxfield]

(In reply to comment #4)
> Times New Roman has been working with Arabic for a while, but last time we
> tried allowing it, it was prohibitively slow. Has that changed?

I don't know; I didn't test performance. How was it tested before? Were there any radars filed about it? Do you have any other information?

[Build Bot]

Comment on attachment 254335 [details]
Patch

Attachment 254335 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4670180730863616

New failing tests:
fast/text/arabic-times-new-roman.html

[Build Bot]

Created attachment 254337 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

(In reply to comment #5)
> (In reply to comment #4)
> > Times New Roman has been working with Arabic for a while, but last time we
> > tried allowing it, it was prohibitively slow. Has that changed?
> 
> I don't know; I didn't test performance. How was it tested before?

It was tested using Apple’s PLT.

> Were there any radars filed about it? Do you have any other information?

<rdar://problem/12096835> is the Apple bug tracking allowing Times New Roman to be used for Arabic, and it contains the most recent measurements.

[Build Bot]

Comment on attachment 254335 [details]
Patch

Attachment 254335 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6693017448611840

New failing tests:
fast/text/arabic-times-new-roman.html

[Build Bot]

Created attachment 254339 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Myles C. Maxfield]

Created attachment 254371 [details]
Patch

[Darin Adler]

Comment on attachment 254371 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:-89
> -    // Times New Roman contains Arabic glyphs, but Core Text doesn't know how to shape them. <rdar://problem/9823975>
> -    // FIXME <rdar://problem/12096835> remove this function once the above bug is fixed.
> -    // Arial and Tahoma are have performance issues so don't use them as well.

Have we tested both the correctness of shaping Times New Roman, and the performance of shaping in Arial and Tahoma? Where can I see some details of how we tested and what results we got?

[Myles C. Maxfield]

Comment on attachment 254371 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:-89
>> -    // Arial and Tahoma are have performance issues so don't use them as well.
> 
> Have we tested both the correctness of shaping Times New Roman, and the performance of shaping in Arial and Tahoma? Where can I see some details of how we tested and what results we got?

I'm testing the performance right now.

I probably don't actually need to change Arial and Tahoma for this bug, since it only concerns Times New Roman. I'll revert that part.

[Myles C. Maxfield]

The patch, as posted, is a slowdown by almost 1 standard deviation. So that's unacceptable.

[Myles C. Maxfield]

(In reply to comment #14)
> The patch, as posted, is a slowdown by almost 1 standard deviation. So
> that's unacceptable.

(Just tested on kooora.com)

[Myles C. Maxfield]

Calling CTFontCreatePhysicalFontDescriptorForCharactersWithLanguage() on TimesNewRomanPSMT returns GeezaPro

[Myles C. Maxfield]

Comment on attachment 254371 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:-89
>>> -    // Arial and Tahoma are have performance issues so don't use them as well.
>> 
>> Have we tested both the correctness of shaping Times New Roman, and the performance of shaping in Arial and Tahoma? Where can I see some details of how we tested and what results we got?
> 
> I'm testing the performance right now.
> 
> I probably don't actually need to change Arial and Tahoma for this bug, since it only concerns Times New Roman. I'll revert that part.

The smaller version of this patch has no regression on iPhone 5s

[Myles C. Maxfield]

Created attachment 254405 [details]
Patch

Comment on attachment 254405 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Georgia doesn't support Arabic, so we ask CoreText what font does support Arabic. It returns
> +        TimesNewRomanPSMT. In the past, this font used to be broken with Arabic, so we explicitly disallowed it.

Are there any fonts that don’t support Arabic for which Core Text returns Arial or Tahoma as a fallback?

[Myles C. Maxfield]

(In reply to comment #19)
> Comment on attachment 254405 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254405&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Georgia doesn't support Arabic, so we ask CoreText what font does support Arabic. It returns
> > +        TimesNewRomanPSMT. In the past, this font used to be broken with Arabic, so we explicitly disallowed it.
> 
> Are there any fonts that don’t support Arabic for which Core Text returns
> Arial or Tahoma as a fallback?

Just wrote a tiny iOS program, looks like the answer is no. The only fonts that CTFontCreatePhysicalFontDescriptorForCharactersWithLanguage() will return for Arabic will be:

CourierNewPS-BoldMT
CourierNewPSMT
GeezaPro
GeezaPro-Bold
TimesNewRomanPS-BoldMT
TimesNewRomanPSMT

[Myles C. Maxfield]

Comment on attachment 254405 [details]
Patch

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

>>> Source/WebCore/ChangeLog:10
>>> +        TimesNewRomanPSMT. In the past, this font used to be broken with Arabic, so we explicitly disallowed it.
>> 
>> Are there any fonts that don’t support Arabic for which Core Text returns Arial or Tahoma as a fallback?
> 
> Just wrote a tiny iOS program, looks like the answer is no. The only fonts that CTFontCreatePhysicalFontDescriptorForCharactersWithLanguage() will return for Arabic will be:
> 
> CourierNewPS-BoldMT
> CourierNewPSMT
> GeezaPro
> GeezaPro-Bold
> TimesNewRomanPS-BoldMT
> TimesNewRomanPSMT

Tahoma doesn't even seem to be installed on iOS.

[Darin Adler]

Comment on attachment 254405 [details]
Patch

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

> Source/WebCore/ChangeLog:24
> +        * platform/graphics/Font.cpp:
> +        (WebCore::Font::Font): Deleted.
> +        (WebCore::createAndFillGlyphPage): Deleted.
> +        * platform/graphics/Font.h:
> +        (WebCore::Font::shouldNotBeUsedForArabic): Deleted.
> +        * platform/graphics/cocoa/FontCascadeCocoa.mm:
> +        (WebCore::FontCascade::fontForCombiningCharacterSequence): Deleted.
> +        * platform/graphics/cocoa/FontCocoa.mm:
> +        (WebCore::fontFamilyShouldNotBeUsedForArabic): Deleted.
> +        (WebCore::Font::platformInit): Deleted.

This is now almost entirely wrong. Need to regenerate the list of affected functions.

[Myles C. Maxfield]

Here's my raw data of performance of this patch:

iPhone 5s:      Time Std. Dev.
With patch:    1.496     0.102
Without patch: 1.523     0.105
With patch:    1.497     0.098
Without patch: 1.499     0.102
With patch:    1.507     0.104
Without patch: 1.490     0.103

iPad Mini:      Time Std. Dev.
With patch:    3.674     0.182
Without patch: 3.638     0.176
With patch:    3.610     0.144
Without patch: 3.630     0.249
With patch:    3.665     0.187
Without patch: 3.637     0.204

[Myles C. Maxfield]

Created attachment 254545 [details]
Patch for landing

[Myles C. Maxfield]

Metz: Given that the font works as expected, perf seems to be okay, and the patch has been positively reviewed, I'm going to land this if I don't hear from you within a day or two.

[Myles C. Maxfield]

(In reply to comment #25)
> Metz: Given that the font works as expected, perf seems to be okay, and the
> patch has been positively reviewed, I'm going to land this if I don't hear
> from you within a day or two.

*Mitz

[Myles C. Maxfield]

(In reply to comment #25)
> Metz: Given that the font works as expected, perf seems to be okay, and the
> patch has been positively reviewed, I'm going to land this if I don't hear
> from you within a day or two.

I have now heard from Mitz.

[Myles C. Maxfield]

(In reply to comment #27)
> (In reply to comment #25)
> > Metz: Given that the font works as expected, perf seems to be okay, and the
> > patch has been positively reviewed, I'm going to land this if I don't hear
> > from you within a day or two.
> 
> I have now heard from Mitz.

Dan and I discussed a better way to fix this bug.

[Myles C. Maxfield]

Created attachment 255273 [details]
Patch

[Myles C. Maxfield]

Created attachment 255282 [details]
Patch

[Myles C. Maxfield]

Created attachment 255283 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:450:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 255283 [details]
Patch

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

> Source/WebCore/platform/graphics/Font.h:72
> +#if PLATFORM(IOS)
> +bool fontFamilyShouldNotBeUsedForArabic(CFStringRef);
> +#endif

Pretty ugly to have a super-platform-specific function using a platform-specific type right here at the top of Font.h. Is there a better home for this?

> Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:449
> +    if (RetainPtr<CFStringRef> foundFontName = adoptCF(static_cast<CFStringRef>(CTFontDescriptorCopyAttribute(fallbackFontDescriptor.get(), kCTFontNameAttribute)))) {

Could use auto instead of RetainPtr<CFStringRef>.

> Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:451
> +            RetainPtr<CFStringRef> familyName = adoptCF(static_cast<CFStringRef>(CTFontDescriptorCopyAttribute(fallbackFontDescriptor.get(), kCTFontFamilyNameAttribute)));

Could use auto instead of RetainPtr<CFStringRef>.

[Myles C. Maxfield]

Comment on attachment 255283 [details]
Patch

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

>> Source/WebCore/platform/graphics/Font.h:72
>> +#endif
> 
> Pretty ugly to have a super-platform-specific function using a platform-specific type right here at the top of Font.h. Is there a better home for this?

I tried to find a better place, and I can't seem to find a better one. We don't have any platform-specific headers for font stuff (rather, we have platform-specific implementation files). I think the best I can do is to move this to the bottom of the file.

[Myles C. Maxfield]

Created attachment 255358 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 255358 [details]
Patch for landing

Rejecting attachment 255358 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 255358, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/5839308690817024

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:450:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Committed r185842: <http://trac.webkit.org/changeset/185842>

[Alex Christensen]

fast/text/arabic-times-new-roman.html always fails on Windows.

[Myles C. Maxfield]

Committed r185883: <http://trac.webkit.org/changeset/185883>