88806 2012-06-11 14:14:53 -0700 [Chromium-Android] Remove Android-specific code in FontPlatformDataHarfBuzz.cpp 2012-06-11 16:18:23 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 66687 1 wangxianzhu webkit-unassigned abarth peter reed oldest_to_newest 646249 0 wangxianzhu 2012-06-11 14:14:53 -0700 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. 646313 1 peter 2012-06-11 15:14:42 -0700 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 646377 2 wangxianzhu 2012-06-11 16:07:15 -0700 (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. 646381 3 peter 2012-06-11 16:09:44 -0700 Are you proposing to switch Android back to getAdvancedTypefaceMetrics(), and thus removing the compile-time branch, until emSizeInFontUnits() is available for all platforms? 646392 4 wangxianzhu 2012-06-11 16:18:23 -0700 (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.