Add SPI for telling WebKit to prefer pictograph glyphs over monochrome ones
Created attachment 213908 [details] Patch
Comment on attachment 213908 [details] Patch Attachment 213908 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/3866073
Comment on attachment 213908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213908&action=review > Source/WebCore/platform/graphics/FontSelector.h:44 > + virtual PassRefPtr<FontData> getFallbackFontData(const FontDescription&, size_t) = 0; A drive-by nitpick: "get" is wrong to use in this function's name: 1. We use "get" for functions with a return argument. 2. A "get" function shouldn't transfer ownership via PassRefPtr, it's more about "create" or "take".
Comment on attachment 213908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213908&action=review > Source/WebKit/mac/WebView/WebView.mm:3161 > +- (void)_setFontFallbackPrefersPictographs:(BOOL)flag > +{ > + if (_private->page) > + _private->page->settings().setFontFallbackPrefersPictographs(flag); > +} It’s clearly wrong for a WebView instance method to update the shared Settings object. Looking at the history of this code, I see that originally I had used WebPreferences for this, but then for some reason external to WebKit I had to change this into an instance method. It‘s OK to keep the code as-is for now, but please add a FIXME here about how this is wrong, and a comment next to the method declaration about how it affects all WebViews sharing the same WebPreferences instance.
Comment on attachment 213908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213908&action=review > Source/WebCore/css/CSSFontSelector.cpp:648 > + ASSERT_ARG(index, !index); I'm changing this to ASSERT_UNUSED(...) to fix the release build.
(In reply to comment #3) > (From update of attachment 213908 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=213908&action=review > > > Source/WebCore/platform/graphics/FontSelector.h:44 > > + virtual PassRefPtr<FontData> getFallbackFontData(const FontDescription&, size_t) = 0; > > A drive-by nitpick: "get" is wrong to use in this function's name: > 1. We use "get" for functions with a return argument. > 2. A "get" function shouldn't transfer ownership via PassRefPtr, it's more about "create" or "take". I agree with Alexey that the name is wrong, but I think it’s more important to keep the interface consistent with the getFontData member function for now, and then fix both at the same time.
Comment on attachment 213908 [details] Patch Attachment 213908 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/3875074
(In reply to comment #6) > (In reply to comment #3) > > (From update of attachment 213908 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=213908&action=review > > > > > Source/WebCore/platform/graphics/FontSelector.h:44 > > > + virtual PassRefPtr<FontData> getFallbackFontData(const FontDescription&, size_t) = 0; > > > > A drive-by nitpick: "get" is wrong to use in this function's name: > > 1. We use "get" for functions with a return argument. > > 2. A "get" function shouldn't transfer ownership via PassRefPtr, it's more about "create" or "take". > > I agree with Alexey that the name is wrong, but I think it’s more important to keep the interface consistent with the getFontData member function for now, and then fix both at the same time. I'll add a FIXME for now. I'm also going to write a test, so I'll post a new patch incorporating all feedback.
Created attachment 213940 [details] Patch
Committed r157265: <http://trac.webkit.org/changeset/157265>