RESOLVED WONTFIX 88806
[Chromium-Android] Remove Android-specific code in FontPlatformDataHarfBuzz.cpp 
https://bugs.webkit.org/show_bug.cgi?id=88806
Summary [Chromium-Android] Remove Android-specific code in FontPlatformDataHarfBuzz.cpp
Xianzhu Wang
Reported 2012-06-11 14:14:53 PDT
In FontPlatformDataHarfBuzz.cpp, FontPlatformData::emSizeInFontUnits(), the Android-specific code was added when skia's Android port didn't support Typeface::getAdvancedTypefaceMetrics(). Skia added Android support of the method in http://codereview.appspot.com/5396045, so the Android-specific code can be removed.
Attachments
Add attachment proposed patch, testcase, etc.
Peter Beverloo
Comment 1 2012-06-11 15:14:42 PDT
Since getAdvancedTypefaceMetrics() is only called to get the em size in font units (the caller disregards all other returned information), I don't think this would be the right approach. See the following bugs for more context (also referred to by a TODO in the code): Bug 75702 ([Chromium] Use SkFontHost::GetUnitsPerEm instead of advanced type metrics for Android in FontPlatformData) Bug 75961 ([Chromium] Switch to using the new SkTypeface::GetUnitsPerEm API) http://code.google.com/p/skia/issues/detail?id=441 (Add SkTypeface::GetUnitsPerEm()) +reed
Xianzhu Wang
Comment 2 2012-06-11 16:07:15 PDT
(In reply to comment #1) > Since getAdvancedTypefaceMetrics() is only called to get the em size in font units (the caller disregards all other returned information), I don't think this would be the right approach. See the following bugs for more context (also referred to by a TODO in the code): > > Bug 75702 ([Chromium] Use SkFontHost::GetUnitsPerEm instead of advanced type metrics for Android in FontPlatformData) > Bug 75961 ([Chromium] Switch to using the new SkTypeface::GetUnitsPerEm API) > > http://code.google.com/p/skia/issues/detail?id=441 (Add SkTypeface::GetUnitsPerEm()) > > +reed Yes, I saw those bugs, but this one just removes the Android-specific code with the bugs (to switch to SkTypeface::GetUnitsPerEm()) still open. That is, it combines two different 'incorrect' ways into one :) Found this by the way when cleaning up Android-specific code in the file in downstream. I'm OK to delay this until we fix 75961.
Peter Beverloo
Comment 3 2012-06-11 16:09:44 PDT
Are you proposing to switch Android back to getAdvancedTypefaceMetrics(), and thus removing the compile-time branch, until emSizeInFontUnits() is available for all platforms?
Xianzhu Wang
Comment 4 2012-06-11 16:18:23 PDT
(In reply to comment #3) > Are you proposing to switch Android back to getAdvancedTypefaceMetrics(), and thus removing the compile-time branch, until emSizeInFontUnits() is available for all platforms? Yes. But now I think this is unnecessary as there's no diff about this in downstream/upstream. Closing this bug.
Note You need to log in before you can comment on or make changes to this bug.