WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(16.55 KB, patch)
2013-10-10 15:53 PDT
,
Andy Estes
mitz: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Andy Estes
Comment 1
2013-10-10 11:36:51 PDT
Created
attachment 213908
[details]
Patch
Build Bot
Comment 2
2013-10-10 11:56:41 PDT
Comment on
attachment 213908
[details]
Patch
Attachment 213908
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/3866073
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
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
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
Created
attachment 213940
[details]
Patch
Andy Estes
Comment 10
2013-10-10 16:04:27 PDT
Committed
r157265
: <
http://trac.webkit.org/changeset/157265
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug