[chromium] Complex text support for Android.
Created attachment 138067 [details] Patch
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 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.
Created attachment 138224 [details] Patch
(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 on attachment 138224 [details] Patch Attachment 138224 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12462441
Created attachment 138228 [details] Patch
(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 on attachment 138228 [details] Patch Patch looks good to me (but I'm not a reviewer).
(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 on attachment 138228 [details] Patch Clearing flags on attachment: 138228 Committed r115373: <http://trac.webkit.org/changeset/115373>
All reviewed patches have been landed. Closing bug.