Arabic text styled with Georgia is rendered as boxes
Created attachment 254334 [details] Patch
Created attachment 254335 [details] Patch
<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?
(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?
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
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.
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
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
Created attachment 254371 [details] Patch
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?
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 patch, as posted, is a slowdown by almost 1 standard deviation. So that's unacceptable.
(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)
Calling CTFontCreatePhysicalFontDescriptorForCharactersWithLanguage() on TimesNewRomanPSMT returns GeezaPro
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
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?
(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
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.
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.
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
Created attachment 254545 [details] Patch for landing
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.
(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
(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.
(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.
Created attachment 255273 [details] Patch
Created attachment 255282 [details] Patch
Created attachment 255283 [details] Patch
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.
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>.
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.
Created attachment 255358 [details] Patch for landing
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
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.
Committed r185842: <http://trac.webkit.org/changeset/185842>
fast/text/arabic-times-new-roman.html always fails on Windows.
Committed r185883: <http://trac.webkit.org/changeset/185883>