Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
447233003:
Use font manager for fallback on Android. (Closed)
|
Created:
6 years, 4 months ago by bungeman-chromium Modified:
6 years, 4 months ago CC:
blink-reviews, jamesr, krit, jbroman, danakj, Rik, Stephen Chennney, pdr., rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionUse font manager for fallback on Android.
The corresponding Chromium side change is https://codereview.chromium.org/450733002/ . These two can be committed in any order.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179753
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove extra parens. #
Total comments: 2
Messages
Total messages: 15 (0 generated)
Adding fmalita as an OWNER.
https://codereview.chromium.org/447233003/diff/1/Source/platform/fonts/androi... File Source/platform/fonts/android/FontCacheAndroid.cpp (right): https://codereview.chromium.org/447233003/diff/1/Source/platform/fonts/androi... Source/platform/fonts/android/FontCacheAndroid.cpp:66: RefPtr<SkFontMgr> fm = adoptRef((SkFontMgr::RefDefault())); drive-by nit: why the double parentheses for both adoptRef calls?
https://codereview.chromium.org/447233003/diff/1/Source/platform/fonts/androi... File Source/platform/fonts/android/FontCacheAndroid.cpp (right): https://codereview.chromium.org/447233003/diff/1/Source/platform/fonts/androi... Source/platform/fonts/android/FontCacheAndroid.cpp:66: RefPtr<SkFontMgr> fm = adoptRef((SkFontMgr::RefDefault())); On 2014/08/07 16:36:40, jbroman wrote: > drive-by nit: why the double parentheses for both adoptRef calls? Thanks, for pointing that out, will fix. This was due to moving things about, I suspect.
lgtm https://codereview.chromium.org/447233003/diff/20001/Source/platform/fonts/an... File Source/platform/fonts/android/FontCacheAndroid.cpp (right): https://codereview.chromium.org/447233003/diff/20001/Source/platform/fonts/an... Source/platform/fonts/android/FontCacheAndroid.cpp:73: return skiaFamilyName.c_str(); Unrelated nit: for the sake of the next person looking at this and not spilling their coffee, we should instantiate the AtomicString explicitly :)
https://codereview.chromium.org/447233003/diff/20001/Source/platform/fonts/an... File Source/platform/fonts/android/FontCacheAndroid.cpp (right): https://codereview.chromium.org/447233003/diff/20001/Source/platform/fonts/an... Source/platform/fonts/android/FontCacheAndroid.cpp:73: return skiaFamilyName.c_str(); On 2014/08/07 17:41:07, Florin Malita wrote: > Unrelated nit: for the sake of the next person looking at this and not spilling > their coffee, we should instantiate the AtomicString explicitly :) OMG, yes. This is what it was doing, so I'm leaving it for now. This happens to work because the underlying Rec in the String is just being reffed and then unreffed here but is still owned by the typeface. So in theory this pointer is good for as long as the typeface exists (forever, essentially). Even though this code never should have relied on that (we certainly don't guarantee it). I'm not really into fixing it right now because the idea that you can do fallback using family names is wrong (even with the extra ttcindex hack) because it isn't necessarily the case that requesting the family name from the font and then looking that name up is going to get you the same typeface anyway (especially with styles and such). So this all needs to change in the future in any event to use the looked up typeface directly.
The CQ bit was checked by bungeman@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bungeman@chromium.org/447233003/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by bungeman@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bungeman@chromium.org/447233003/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
Message was sent while issue was closed.
Change committed as 179753 |
