fontconfig on Linux can change the render preferences on a per strike basis (a strike a combination of face and size). Because of this, we need to query fontconfig each time a new FontPlatformData is created for a new size. This patch adds support for querying this via ChromiumBridge.
Created attachment 45681 [details] patch: two sided so please don't cq+
Attachment 45681 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/graphics/chromium/FontRenderStyle.h:42: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/chromium/FontRenderStyle.h:44: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/chromium/FontRenderStyle.h:45: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/chromium/FontRenderStyle.h:46: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/chromium/FontPlatformDataLinux.cpp:142: One line control clauses should not use braces. [whitespace/braces] [4] WebCore/platform/graphics/chromium/FontPlatformDataLinux.cpp:201: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebKit/chromium/public/linux/WebFontRenderStyle.h:62: #endif line should be "#endif // WebFontRenderStyle_h" [build/header_guard] [5] WebKit/chromium/public/linux/WebFontRenderStyle.h:45: One space before end of line comments [whitespace/comments] [5] WebKit/chromium/public/linux/WebFontRenderStyle.h:47: One space before end of line comments [whitespace/comments] [5] WebKit/chromium/public/linux/WebFontRenderStyle.h:48: One space before end of line comments [whitespace/comments] [5] WebKit/chromium/public/linux/WebFontRenderStyle.h:49: One space before end of line comments [whitespace/comments] [5] WebKit/chromium/src/gtk/WebFontInfo.cpp:110: is_bold is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit/chromium/src/gtk/WebFontInfo.cpp:111: is_italic is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit/chromium/src/gtk/WebFontInfo.cpp:143: Use 0 instead of NULL. [readability/null] [4] WebKit/chromium/src/ChromiumBridge.cpp:313: One line control clauses should not use braces. [whitespace/braces] [4] WebKit/chromium/src/ChromiumBridge.cpp:315: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 16
Created attachment 45682 [details] patch v2: two sided so please don't cq+ fixing up style bot issues. (I think one issue will remain: it's triggering on a mention of 'NULL' in a comment)
Attachment 45681 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/152496
Comment on attachment 45682 [details] patch v2: two sided so please don't cq+ > + paint->setAntiAlias(m_style.useAntiAlias == 2 ? isSkiaAntiAlias : m_style.useAntiAlias); > + paint->setHinting(m_style.useHinting == 2 ? skiaHinting : (SkPaint::Hinting) m_style.hintStyle); 2 => kNoPreference or some such > + m_style.useBitmaps = 2; Here too. > + void queryStyle(); Perhaps this could have a more verbose name that is more self-descriptive. > + FcPatternAdd(pattern, FC_FAMILY, fcvalue, 0); The last param should be FcFalse for this and below. > + // Some versions of fontconfig don't actually write a value into result. Set result = OK at the start, check it at the end? > + FcBool b; One-letter variables make the reviewer man sad.
Created attachment 45685 [details] patch v3: two sided so please don't cq+ > kNoPreference or some such > > > + m_style.useBitmaps = 2; Done > > + void queryStyle(); > > Perhaps this could have a more verbose name that is more self-descriptive. Done > > + FcPatternAdd(pattern, FC_FAMILY, fcvalue, 0); > > The last param should be FcFalse for this and below. Done > > + // Some versions of fontconfig don't actually write a value into result. > > Set result = OK at the start, check it at the end? Not clear that it wouldn't break. fontconfig's own code does this so I'm going to mirror it. > > + FcBool b; > > One-letter variables make the reviewer man sad. reviewer man is unnecessarily sad.
Comment on attachment 45685 [details] patch v3: two sided so please don't cq+ Marking cq- to stop us from marking the patch cq+.
This patch looks formally correct, but I know nothing about fonts or linux. Do you have someone in mind to review this patch?
> Do you have someone in mind to review this patch? Since it touches ChromiumBridge, I figured that fishd should review it. Although I don't know how long he's away for.
I am the reviewer of the Linuxy bits, and it gets my stamp of approval.
Comment on attachment 45685 [details] patch v3: two sided so please don't cq+ There are a bunch of patches blocked on fishd's API review. If you're willing to wait, that probably the best course of action. (I know this will get a complaint from the chromium-ews, but marking it review? will help us keep track of the patch.)
Attachment 45685 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/chromium/src/gtk/WebFontInfo.cpp:143: Use 0 instead of NULL. [readability/null] [4] Total errors found: 1
> WebKit/chromium/src/gtk/WebFontInfo.cpp:143: Use 0 instead of NULL. Note to reviewers: false positive. This mention of NULL is in a comment.
(In reply to comment #13) > > WebKit/chromium/src/gtk/WebFontInfo.cpp:143: Use 0 instead of NULL. > > Note to reviewers: false positive. This mention of NULL is in a comment. fwiw, this isn't a false positive as the script specifically checks for NULL in comments. Several reviewers flag this in reviews. For example, https://bugs.webkit.org/show_bug.cgi?id=22700#c22 "We do not use NULL in code, so using it in comments isn't appropriate either. I suggest lower case "null" or 0."
Created attachment 45949 [details] changing NULL to 0 in a comment Have changed the NULL to a 0 in the comment. I'll see how slammed Darin is at the moment. Since this only adds APIs in the same places as patch he previously reviewed, I might ask someone else to r+ it.
style-queue ran check-webkit-style on attachment 45949 [details] without any errors.
Comment on attachment 45949 [details] changing NULL to 0 in a comment > +++ b/WebCore/platform/graphics/chromium/FontRenderStyle.h ... > +struct FontRenderStyle { > + enum { > + kNoPreference = 2, > + }; nit: indent by 4 spaces. kNoPreference should be NoPreference without the "k" prefix. > +++ b/WebKit/chromium/public/gtk/WebFontInfo.h ... > WEBKIT_API static WebCString familyForChars(const WebUChar* characters, size_t numCharacters); > + // Fill out the given WebFontRenderStyle with the user's preferences for ^^^ nit: please insert a new line after familyForChars. > + WEBKIT_API static void renderStyleForStrike(WebFontRenderStyle* result, const char* family, int sizeAndStyle); nit: isn't it better style to list out parameters last? or perhaps this should just be a return value? renderStyleForStrike(const char* family, int sizeAndStyle, WebFontRenderStyle* result); > +++ b/WebKit/chromium/public/linux/WebFontRenderStyle.h > +++ b/WebKit/chromium/public/linux/WebSandboxSupport.h ... > + virtual void getRenderStyleForStrike(WebFontRenderStyle* style, const char* family, int sizeAndStyle) = 0; ditto... seems like out params should be listed last. > +void WebFontInfo::renderStyleForStrike(WebFontRenderStyle* out, const char* family, int sizeAndStyle) > +{ > + bool isBold = sizeAndStyle & 1; > + bool isItalic = sizeAndStyle & 2; > + int pixelSize = sizeAndStyle >> 2; are there no constants that can be used in place of these magic numbers? > +++ b/WebKit/chromium/src/linux/WebFontRenderStyle.cpp ... > +using WebCore::FontRenderStyle; > + > +namespace WebKit { > + > +void WebFontRenderStyle::toFontRenderStyle(WebCore::FontRenderStyle* out) nit: no need for the WebCore:: prefix above.
Created attachment 46091 [details] Addressing fishd's comments > nit: indent by 4 spaces. > kNoPreference should be NoPreference without the "k" prefix. Done > ^^^ nit: please insert a new line after familyForChars. Done > nit: isn't it better style to list out parameters last? or perhaps this should > just be a return value? I've moved the outarg to the end, although generally I disagree. Destinations usually come first: memcpy, strcpy, '=' etc > are there no constants that can be used in place of these magic numbers? There's a great big comment in the .h file so I'm not too worried. > nit: no need for the WebCore:: prefix above. Done Cheers
style-queue ran check-webkit-style on attachment 46091 [details] without any errors.
I summon Eric, who can hopefully r+ this because it's otherwise stagnating. Cheers
Comment on attachment 46091 [details] Addressing fishd's comments OK, r=me (Sorry for the delays, I lost track of this one. I had an issue with too much bugmail, which is now hopefully a thing of the past.)
> > nit: isn't it better style to list out parameters last? or perhaps this should > > just be a return value? > > I've moved the outarg to the end, although generally I disagree. Destinations > usually come first: memcpy, strcpy, '=' etc Perhaps I am too accustomed to the Google C++ Style Guide, which mandates that out parameters be listed last. That matches my preference as well, but maybe that is due to working in a COM world for too long ;-)
Attachment 46091 [details] was posted by a committer and has review+, assigning to Adam Langley for commit.
r55089
Created attachment 49698 [details] family checking fix in querySystemForRenderStyle In previous patches was a small mistake. Now the feature works correctly. I tested it with these settings: rgba none hinting true hintstyle hintfull antialias true except for [0, 15] sizes Here is my debug output from WebFontInfo::renderStyleForStrike with google.com page: family=arial pixelSize=17 isBold=0, isItalic=0 FC match: useAntiAlias=1, useBitmaps=1, useAutoHint=0, useHinting=1, hintStyle=3 family=arial pixelSize=13 isBold=1, isItalic=0 FC match: useAntiAlias=0, useBitmaps=1, useAutoHint=0, useHinting=1, hintStyle=3 family=arial pixelSize=10 isBold=0, isItalic=0 FC match: useAntiAlias=0, useBitmaps=1, useAutoHint=0, useHinting=1, hintStyle=3 ....
Roman: I added a ChangeLog to your patch and uploaded it at https://bugs.webkit.org/show_bug.cgi?id=35495
Thanks for that. Will review etc on Monday.