Bug 84431 - [chromium] Complex text support for Android.
Summary: [chromium] Complex text support for Android.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hao Zheng
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-20 04:27 PDT by Hao Zheng
Modified: 2012-04-26 14:50 PDT (History)
7 users (show)

See Also:


Attachments
Patch (12.45 KB, patch)
2012-04-20 04:39 PDT, Hao Zheng
no flags Details | Formatted Diff | Diff
Patch (12.38 KB, patch)
2012-04-21 00:46 PDT, Hao Zheng
no flags Details | Formatted Diff | Diff
Patch (12.13 KB, patch)
2012-04-21 02:07 PDT, Hao Zheng
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hao Zheng 2012-04-20 04:27:18 PDT
[chromium] Complex text support for Android.
Comment 1 Hao Zheng 2012-04-20 04:39:41 PDT
Created attachment 138067 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-20 04:42:07 PDT
Attachment 138067 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:15:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Kenichi Ishibashi 2012-04-20 10:54:53 PDT
Comment on attachment 138067 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        No new tests. (OOPS!)
> 
> You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]

Please state why the patch doesn't contain tests.

> Source/WebCore/platform/graphics/FontCache.h:108
> +    FontPlatformData* getCachedFallbackScriptFontPlatformData(const FontDescription&, const AtomicString& family);

Please don't add this kind of public function. FontPlatformData shouldn't be exposed from FontCache.

However, as mac port specifies that ComplexTextController is a friend of FontCache, I think the current FontCache has structural problems with complex text rendering. How about:
- remove this function
- make ComplexTextController to be a friend of FontCache on Android
- call fontCache()->getCachedFontPlatformData(fontDescription, SkGetFallbackScriptID(fallbackScript), true) in ComplexTextController::getComplexFontPlatformData()

This still looks ugly, though.

> Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp:242
> +    return 0;

How about "return &fontData->fontDataForCharacter(' ')->platformData()" ? This way you can remove L262, L264-L266.
Comment 4 Hao Zheng 2012-04-21 00:46:12 PDT
Created attachment 138224 [details]
Patch
Comment 5 Hao Zheng 2012-04-21 00:49:13 PDT
(In reply to comment #3)
> (From update of attachment 138067 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138067&action=review
> 
> >> Source/WebCore/ChangeLog:15
> >> +        No new tests. (OOPS!)
> > 
> > You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
> 
> Please state why the patch doesn't contain tests.

Done.

> 
> > Source/WebCore/platform/graphics/FontCache.h:108
> > +    FontPlatformData* getCachedFallbackScriptFontPlatformData(const FontDescription&, const AtomicString& family);
> 
> Please don't add this kind of public function. FontPlatformData shouldn't be exposed from FontCache.
> 
> However, as mac port specifies that ComplexTextController is a friend of FontCache, I think the current FontCache has structural problems with complex text rendering. How about:
> - remove this function
> - make ComplexTextController to be a friend of FontCache on Android
> - call fontCache()->getCachedFontPlatformData(fontDescription, SkGetFallbackScriptID(fallbackScript), true) in ComplexTextController::getComplexFontPlatformData()
> 
> This still looks ugly, though.
> 

Done.

> > Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp:242
> > +    return 0;
> 
> How about "return &fontData->fontDataForCharacter(' ')->platformData()" ? This way you can remove L262, L264-L266.

Done. Good Suggestion. But I need to surround surrogatePairAwareFirstCharacter() in #ifdef, as it would generate 'defined but not used' error on Android.
Comment 6 WebKit Review Bot 2012-04-21 01:04:29 PDT
Comment on attachment 138224 [details]
Patch

Attachment 138224 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12462441
Comment 7 Hao Zheng 2012-04-21 02:07:42 PDT
Created attachment 138228 [details]
Patch
Comment 8 Hao Zheng 2012-04-21 02:19:46 PDT
(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 138067 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=138067&action=review
> > 
> > >> Source/WebCore/ChangeLog:15
> > >> +        No new tests. (OOPS!)
> > > 
> > > You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
> > 
> > Please state why the patch doesn't contain tests.
> 
> Done.
> 
> > 
> > > Source/WebCore/platform/graphics/FontCache.h:108
> > > +    FontPlatformData* getCachedFallbackScriptFontPlatformData(const FontDescription&, const AtomicString& family);
> > 
> > Please don't add this kind of public function. FontPlatformData shouldn't be exposed from FontCache.
> > 
> > However, as mac port specifies that ComplexTextController is a friend of FontCache, I think the current FontCache has structural problems with complex text rendering. How about:
> > - remove this function
> > - make ComplexTextController to be a friend of FontCache on Android
> > - call fontCache()->getCachedFontPlatformData(fontDescription, SkGetFallbackScriptID(fallbackScript), true) in ComplexTextController::getComplexFontPlatformData()
> > 
> > This still looks ugly, though.
> > 
> 
> Done.
> 
> > > Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp:242
> > > +    return 0;
> > 
> > How about "return &fontData->fontDataForCharacter(' ')->platformData()" ? This way you can remove L262, L264-L266.
> 
> Done. Good Suggestion. But I need to surround surrogatePairAwareFirstCharacter() in #ifdef, as it would generate 'defined but not used' error on Android.

I found the logic is not expected. If a script is not supported by fallback fonts on Android, we still need to use system fonts. So the normal logic is also needed by Android. Anyway, I move some code into the check 'if (!platformData)'.

And I find the parameter name checkingAlternateName of getCachedFontPlatformData is confusing. From the code in FontCache.cpp, it seems it should be called noCheckingAlternateName.

206     if (!foundResult && !checkingAlternateName) {
207         // We were unable to find a font.  We have a small set of fonts that we alias to other names, 
208         // e.g., Arial/Helvetica, Courier/Courier New, etc.  Try looking up the font under the aliased name.
209         const AtomicString& alternateName = alternateFamilyName(familyName);
210         if (!alternateName.isEmpty())
211             result = getCachedFontPlatformData(fontDescription, alternateName, true);
212         if (result)
213             gFontPlatformDa
Comment 9 Kenichi Ishibashi 2012-04-21 12:03:11 PDT
Comment on attachment 138228 [details]
Patch

Patch looks good to me (but I'm not a reviewer).
Comment 10 Hao Zheng 2012-04-22 02:03:02 PDT
(In reply to comment #9)
> (From update of attachment 138228 [details])
> Patch looks good to me (but I'm not a reviewer).

Thanks, bashi. Tony, could you please take a review?
Comment 11 WebKit Review Bot 2012-04-26 14:50:17 PDT
Comment on attachment 138228 [details]
Patch

Clearing flags on attachment: 138228

Committed r115373: <http://trac.webkit.org/changeset/115373>
Comment 12 WebKit Review Bot 2012-04-26 14:50:24 PDT
All reviewed patches have been landed.  Closing bug.