Bug 88806
| Summary: | [Chromium-Android] Remove Android-specific code in FontPlatformDataHarfBuzz.cpp | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Xianzhu Wang <wangxianzhu> |
| Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED WONTFIX | ||
| Severity: | Normal | CC: | abarth, peter, reed |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | |||
| Bug Blocks: | 66687 | ||
Xianzhu Wang
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
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
(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
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
(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.