Bug 122608 – Add SPI for telling WebKit to prefer pictograph glyphs over monochrome ones

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

Add SPI for telling WebKit to prefer pictograph glyphs over monochrome ones

[Andy Estes]

Created attachment 213908 [details]
Patch

[Build Bot]

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 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.

[Andy Estes]

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.

[Build Bot]

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]

(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]

Created attachment 213940 [details]
Patch

[Andy Estes]

Committed r157265: <http://trac.webkit.org/changeset/157265>