RESOLVED FIXED 122608
Add SPI for telling WebKit to prefer pictograph glyphs over monochrome ones
https://bugs.webkit.org/show_bug.cgi?id=122608
Summary Add SPI for telling WebKit to prefer pictograph glyphs over monochrome ones
Andy Estes
Reported 2013-10-10 11:06:35 PDT
Add SPI for telling WebKit to prefer pictograph glyphs over monochrome ones
Attachments
Patch (12.41 KB, patch)
2013-10-10 11:36 PDT, Andy Estes
no flags
Patch (16.55 KB, patch)
2013-10-10 15:53 PDT, Andy Estes
mitz: review+
Andy Estes
Comment 1 2013-10-10 11:36:51 PDT
Build Bot
Comment 2 2013-10-10 11:56:41 PDT
Alexey Proskuryakov
Comment 3 2013-10-10 12:00:32 PDT
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".
mitz
Comment 4 2013-10-10 12:04:06 PDT
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.
Andy Estes
Comment 5 2013-10-10 12:05:46 PDT
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.
mitz
Comment 6 2013-10-10 12:06:59 PDT
(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.
Build Bot
Comment 7 2013-10-10 12:07:53 PDT
Andy Estes
Comment 8 2013-10-10 12:44:14 PDT
(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.
Andy Estes
Comment 9 2013-10-10 15:53:27 PDT
Andy Estes
Comment 10 2013-10-10 16:04:27 PDT
Note You need to log in before you can comment on or make changes to this bug.