Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1615923004:
Add FontCache API for retrieving prioritized fallback fonts (Closed)
|
Created:
4 years, 11 months ago by drott Modified:
4 years, 10 months ago CC:
blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney, vmpstr+blinkwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd FontCache API for retrieving prioritized fallback fonts
BUG=580536
Committed: https://crrev.com/49d9c1f5fc97e4cedb0518cc3dd40771d8a68bec
Cr-Commit-Position: refs/heads/master@{#373499}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Review comments addressed #
Total comments: 5
Patch Set 3 : newlines removed #Patch Set 4 : Build fix attempt by returning pointer instead of reference #Patch Set 5 : Separate tests per enum value, added fonts on Linux and Win #Patch Set 6 : Column limit, formatting #Patch Set 7 : Additional fallback for Linux #
Total comments: 1
Patch Set 8 : Documenting public method #
Messages
Total messages: 46 (18 generated)
Description was changed from ========== Add FontCache API for retrieving prioritized fallback fonts BUG=580536 ========== to ========== Add FontCache API for retrieving prioritized fallback fonts BUG=580536 ==========
drott@chromium.org changed reviewers: + eae@chromium.org, kojii@chromium.org, wkorman@chromium.org
Currently depends on: https://codereview.chromium.org/1613153002/
https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right): https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/FontCache.cpp:202: if (!fontsListInitialized) { Blink seems to prefer the early-out if (fontsListInitialized) return fontsList; to reduce subsequent indentation and (debatably) readability, I don't feel strongly for short methods like this though. https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/FontCache.cpp:211: rm blank https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/FontCache.h (right): https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/FontCache.h:179: const Vector<AtomicString>& initAndGetfontListForFallbackPriority(const FontDescription); Getfont -> GetFont, and make param a ref? https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/FontCacheTest.cpp (right): https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/FontCacheTest.cpp:38: TEST(FontCache, availablePriorityFallbackFonts) Tests with invalid and text? https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right): https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:56: const char* kColorEmojiFontsMac[] = { "Apple Color Emoji" }; Is there a useful documentation comment we can add here and in Skia below re: what url/doc/approach we used to determine these constants? To help eng in future should they need updating/revisiting. https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:52: // Due to crbug.com/322658 we cannot properly specifiy Android font family names here, specify
Review comments addressed
Thanks for the review! https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right): https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/FontCache.cpp:202: if (!fontsListInitialized) { On 2016/01/25 23:38:16, wkorman wrote: > Blink seems to prefer the early-out Changed to early-return. https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/FontCache.cpp:211: On 2016/01/25 23:38:16, wkorman wrote: > rm blank Done. https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/FontCache.cpp:227: RELEASE_ASSERT_NOT_REACHED(); This would require a EXPECT_DEATH in a gtest checking for Invalid and Text values... https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/FontCacheTest.cpp (right): https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/FontCacheTest.cpp:38: TEST(FontCache, availablePriorityFallbackFonts) On 2016/01/25 23:38:16, wkorman wrote: > Tests with invalid and text? ...however I did not manage to get them to work reliably in Chrome's gtest setup. DEATH tests warn about multithreaded execution and the crashed/assertion-failed process somehow times-out instead of activating and talking to the DEATH test harness correctly. https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right): https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:56: const char* kColorEmojiFontsMac[] = { "Apple Color Emoji" }; On 2016/01/25 23:38:16, wkorman wrote: > Is there a useful documentation comment we can add here and in Skia below re: > what url/doc/approach we used to determine these constants? To help eng in > future should they need updating/revisiting. Added comments on how to determine/update those, to the best of my knowledge. Unfortunately, documentation on this is sparse. https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/1615923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:52: // Due to crbug.com/322658 we cannot properly specifiy Android font family names here, On 2016/01/25 23:38:16, wkorman wrote: > specify Done.
lgtm https://codereview.chromium.org/1615923004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right): https://codereview.chromium.org/1615923004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:228: -1 blank https://codereview.chromium.org/1615923004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:251: -1 blank https://codereview.chromium.org/1615923004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/1615923004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:193: -1 blank https://codereview.chromium.org/1615923004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:199: for (size_t i = 0; i < WTF_ARRAY_LENGTH(kColorEmojiFonts); ++i) Could just set local var to the right array for each case and then do the for loop once below switch on that.
newlines removed
https://codereview.chromium.org/1615923004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/1615923004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:199: for (size_t i = 0; i < WTF_ARRAY_LENGTH(kColorEmojiFonts); ++i) On 2016/02/02 19:04:08, wkorman wrote: > Could just set local var to the right array for each case and then do the for > loop once below switch on that. I've tried doing that but an array cannot be assigned to a local array, only a pointer, and then I lose the ability to figure out the WTF_ARRAY_LENGTH, so I would have to store the length as well. I don't think it would become much prettier in that case.
The CQ bit was checked by drott@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/1615923004/#ps40001 (title: "newlines removed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1615923004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1615923004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Build fix attempt by returning pointer instead of reference
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1615923004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1615923004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
Patchset #4 (id:60001) has been deleted
Build fix attempt by returning pointer instead of reference
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1615923004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1615923004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Separate tests per enum value, added fonts on Linux and Win
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1615923004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1615923004/100001
column limit, formatting
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1615923004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1615923004/120001
Additional fallback for Linux
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1615923004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1615923004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1615923004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/FontCache.h (right): https://codereview.chromium.org/1615923004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/FontCache.h:90: const Vector<AtomicString>* fontListForFallbackPriority(const FontDescription&, FontFallbackPriority); As a public method worth documenting what this does in brief, mentioning lazy initialization and that it will not return null (can it return an empty list?).
Documenting public method
The CQ bit was checked by drott@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/1615923004/#ps160001 (title: "Documenting public method")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1615923004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1615923004/160001
Message was sent while issue was closed.
Description was changed from ========== Add FontCache API for retrieving prioritized fallback fonts BUG=580536 ========== to ========== Add FontCache API for retrieving prioritized fallback fonts BUG=580536 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add FontCache API for retrieving prioritized fallback fonts BUG=580536 ========== to ========== Add FontCache API for retrieving prioritized fallback fonts BUG=580536 Committed: https://crrev.com/49d9c1f5fc97e4cedb0518cc3dd40771d8a68bec Cr-Commit-Position: refs/heads/master@{#373499} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/49d9c1f5fc97e4cedb0518cc3dd40771d8a68bec Cr-Commit-Position: refs/heads/master@{#373499}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:160001) has been created in https://codereview.chromium.org/1750213003/ by drott@chromium.org. The reason for reverting is: This will have to be done differently to be compatible with Android, where we cannot search by family name.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
