Bug 84431 – [chromium] Complex text support for Android.

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

[Hao Zheng]

[chromium] Complex text support for Android.

[Hao Zheng]

Created attachment 138067 [details]
Patch

[WebKit Review Bot]

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.

[Kenichi Ishibashi]

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.

[Hao Zheng]

Created attachment 138224 [details]
Patch

[Hao Zheng]

(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.

[WebKit Review Bot]

Comment on attachment 138224 [details]
Patch

Attachment 138224 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12462441

[Hao Zheng]

Created attachment 138228 [details]
Patch

[Hao Zheng]

(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

[Kenichi Ishibashi]

Comment on attachment 138228 [details]
Patch

Patch looks good to me (but I'm not a reviewer).

[Hao Zheng]

(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?

[WebKit Review Bot]

Comment on attachment 138228 [details]
Patch

Clearing flags on attachment: 138228

Committed r115373: <http://trac.webkit.org/changeset/115373>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.