Issue 6572059: Disable automatic font fallback of SkFontHost_android when building for WebKit (Closed)
|
Created:
12 years, 2 months ago by Xianzhu Modified:
12 years, 1 month ago CC:
skia-review_googlegroups.com, klobag_chromium.org, Hao Zheng Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionDisable automatic font fallback of SkFontHost_android when building for WebKit
The internal font fallback mechanism prevents WebKit from getting
the properties of the actually used font. Disable the internal fallback
mechanism when building for WebKit and add SkGetFallbackFamilyNameForChar()
to let the upper level know the fallback font for individial characters.
BUG=913 (http://code.google.com/p/skia/issues/detail?id=913)
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 5
Patch Set 6 : #MessagesTotal messages: 16
https://codereview.appspot.com/6572059/diff/16/include/core/SkPaint.h File include/core/SkPaint.h (left): https://codereview.appspot.com/6572059/diff/16/include/core/SkPaint.h#oldcode855 include/core/SkPaint.h:855: unsigned getBaseGlyphCount(SkUnichar text) const; As much as I would like to remove this, we currently can't as the Android Framework is using it. The snippet of code where they use this API is on line 888... https://github.com/android/platform_frameworks_base/blob/master/core/jni/andr... If we think we can provide this functionality through some other means then I would be happy to discard this code. https://codereview.appspot.com/6572059/diff/16/src/ports/SkFontHost_android.cpp File src/ports/SkFontHost_android.cpp (right): https://codereview.appspot.com/6572059/diff/16/src/ports/SkFontHost_android.c... src/ports/SkFontHost_android.cpp:883: return 0; remember this code isn't just used by Chrome and while Chrome doesn't use this fallback chain the Android framework does.
Sign in to reply to this message.
Uploaded a new patch set. I keep the original code and use SK_BUILD_FOR_ANDROID_WEBKIT (which will be defined in chromium's skia/skia.gyp) to disable the automatic font fallback mechanism.
Sign in to reply to this message.
https://codereview.appspot.com/6572059/diff/4001/src/ports/SkFontHost_android... File src/ports/SkFontHost_android.cpp (right): https://codereview.appspot.com/6572059/diff/4001/src/ports/SkFontHost_android... src/ports/SkFontHost_android.cpp:882: #ifdef SK_BUILD_FOR_ANDROID_WEBKIT I would recommend that you use the following instead of introducing a new variable... #ifdef SK_BUILD_FOR_ANDROID_NDK This will enforce this behavior for all non-framework builds of Android. https://codereview.appspot.com/6572059/diff/4001/src/ports/SkFontHost_android... src/ports/SkFontHost_android.cpp:883: // We don't handle font fallback, WebKit does. update the comment to say something to the effect of. //Skia does not support font fallback for ndk applications in order to enable clients such as WebKit to customize their font selection.
Sign in to reply to this message.
https://codereview.appspot.com/6572059/diff/4001/src/ports/SkFontHost_android... File src/ports/SkFontHost_android.cpp (right): https://codereview.appspot.com/6572059/diff/4001/src/ports/SkFontHost_android... src/ports/SkFontHost_android.cpp:882: #ifdef SK_BUILD_FOR_ANDROID_WEBKIT On 2012/09/28 17:08:20, DerekS wrote: > I would recommend that you use the following instead of introducing a new > variable... > > #ifdef SK_BUILD_FOR_ANDROID_NDK > > This will enforce this behavior for all non-framework builds of Android. Done. https://codereview.appspot.com/6572059/diff/4001/src/ports/SkFontHost_android... src/ports/SkFontHost_android.cpp:883: // We don't handle font fallback, WebKit does. On 2012/09/28 17:08:20, DerekS wrote: > update the comment to say something to the effect of. > > //Skia does not support font fallback for ndk applications in order to enable > clients such as WebKit to customize their font selection. Done.
Sign in to reply to this message.
lgtm with one nit and one question about your added API https://codereview.appspot.com/6572059/diff/17/include/ports/SkTypeface_andro... File include/ports/SkTypeface_android.h (right): https://codereview.appspot.com/6572059/diff/17/include/ports/SkTypeface_andro... include/ports/SkTypeface_android.h:65: SK_API const char* SkGetFallbackFamilyNameForChar(SkUnichar uni); why return a string here instead of the typeface object? https://codereview.appspot.com/6572059/diff/17/src/ports/SkFontHost_android.cpp File src/ports/SkFontHost_android.cpp (right): https://codereview.appspot.com/6572059/diff/17/src/ports/SkFontHost_android.c... src/ports/SkFontHost_android.cpp:642: // shouldn't get here put an SkDebugf() statement here as we should note in the logs that we had a problem.
Sign in to reply to this message.
https://codereview.appspot.com/6572059/diff/17/include/ports/SkTypeface_andro... File include/ports/SkTypeface_android.h (right): https://codereview.appspot.com/6572059/diff/17/include/ports/SkTypeface_andro... include/ports/SkTypeface_android.h:65: SK_API const char* SkGetFallbackFamilyNameForChar(SkUnichar uni); On 2012/09/28 17:48:12, DerekS wrote: > why return a string here instead of the typeface object? This is required by the current WebKit API: FontCache::getCachedFontPlatformData(fontDescription, string familyName, ...). However I added SkGetFallbackTypefaceForChar() in case that we can modify the WebKit side to use the faster path in the future. https://codereview.appspot.com/6572059/diff/17/src/ports/SkFontHost_android.cpp File src/ports/SkFontHost_android.cpp (right): https://codereview.appspot.com/6572059/diff/17/src/ports/SkFontHost_android.c... src/ports/SkFontHost_android.cpp:642: // shouldn't get here On 2012/09/28 17:48:12, DerekS wrote: > put an SkDebugf() statement here as we should note in the logs that we had a > problem. Done. And changed an original similar place.
Sign in to reply to this message.
Derek, if the latest patch looks good to you, could you help land it in Skia? Thanks!
Sign in to reply to this message.
Hi Derek and Reed, Does the latest patch look good? If yes could you help land it in Skia? Several WebKit bugs depend on it. Thanks! Xianzhu
Sign in to reply to this message.
https://codereview.appspot.com/6572059/diff/14003/include/ports/SkTypeface_an... File include/ports/SkTypeface_android.h (right): https://codereview.appspot.com/6572059/diff/14003/include/ports/SkTypeface_an... include/ports/SkTypeface_android.h:70: * the specified chararacter. More robust would be to take SkString* to return the name in. That way we don't have to ensure that the string is permanently allocated/owned by the typeface. bool SkGetFallbackFamilyNameForChar(SkUnichar uni, SkString* name); I presume you want to call this at times when you don't also want the typeface? https://codereview.appspot.com/6572059/diff/14003/src/ports/SkFontHost_androi... File src/ports/SkFontHost_android.cpp (right): https://codereview.appspot.com/6572059/diff/14003/src/ports/SkFontHost_androi... src/ports/SkFontHost_android.cpp:1003: paint.setTextEncoding(SkPaint::kUTF32_TextEncoding); Can you do this w/o knowing about the glyph cache? uint16_t glyphID; paint.textToGlyphs(&uni, sizeof(uni), &glyphID); return glyphID != 0;
Sign in to reply to this message.
https://codereview.appspot.com/6572059/diff/14003/include/ports/SkTypeface_an... File include/ports/SkTypeface_android.h (right): https://codereview.appspot.com/6572059/diff/14003/include/ports/SkTypeface_an... include/ports/SkTypeface_android.h:70: * the specified chararacter. On 2012/10/04 16:35:45, reed1 wrote: > More robust would be to take SkString* to return the name in. That way we don't > have to ensure that the string is permanently allocated/owned by the typeface. > > bool SkGetFallbackFamilyNameForChar(SkUnichar uni, SkString* name); Done. > > I presume you want to call this at times when you don't also want the typeface? Yes. This is the use case of current WebKit. https://codereview.appspot.com/6572059/diff/14003/src/ports/SkFontHost_androi... File src/ports/SkFontHost_android.cpp (right): https://codereview.appspot.com/6572059/diff/14003/src/ports/SkFontHost_androi... src/ports/SkFontHost_android.cpp:939: Remove the redundant code (brought in when upstreaming Android changes). Also it lacks proper lock. https://codereview.appspot.com/6572059/diff/14003/src/ports/SkFontHost_androi... src/ports/SkFontHost_android.cpp:1003: paint.setTextEncoding(SkPaint::kUTF32_TextEncoding); On 2012/10/04 16:35:45, reed1 wrote: > Can you do this w/o knowing about the glyph cache? > > uint16_t glyphID; > paint.textToGlyphs(&uni, sizeof(uni), &glyphID); > return glyphID != 0; Done.
Sign in to reply to this message.
lgtm -- handing over to Derek
Sign in to reply to this message.
Hi Derek and Reed, What's the ETA of it to appear in Chromium? Thanks, Xianzhu
Sign in to reply to this message.
This CL has been committed so feel free to close this issue. https://code.google.com/p/skia/source/detail?r=5866 The CL will be included in Chromium with the next Skia DEPS roll which in all likelihood will be tomorrow morning.
Sign in to reply to this message.
Cool. Thanks! On Tue, Oct 9, 2012 at 12:31 PM, <djsollen@google.com> wrote: > This CL has been committed so feel free to close this issue. > > https://code.google.com/p/**skia/source/detail?r=5866<https://code.google.com... > > The CL will be included in Chromium with the next Skia DEPS roll which > in all likelihood will be tomorrow morning. > > https://codereview.appspot.**com/6572059/<https://codereview.appspot.com/6572... >
Sign in to reply to this message.
|
