Bug 33065 – [chromium] Linux: add support for per-strike font render preferences

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Adam Langley]

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.

[Adam Langley]

Created attachment 45681 [details]
patch: two sided so please don't cq+

[WebKit Review Bot]

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

[Adam Langley]

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)

[WebKit Review Bot]

Attachment 45681 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/152496

[Evan Martin]

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.

[Adam Langley]

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.

[Adam Barth]

Comment on attachment 45685 [details]
patch v3: two sided so please don't cq+

Marking cq- to stop us from marking the patch cq+.

[Adam Barth]

This patch looks formally correct, but I know nothing about fonts or linux.  Do you have someone in mind to review this patch?

[Adam Langley]

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

[Evan Martin]

I am the reviewer of the Linuxy bits, and it gets my stamp of approval.

[Adam Barth]

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

[WebKit Review Bot]

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

[Adam Langley]

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

[David Levin]

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

[Adam Langley]

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.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45949 [details] without any errors.

[Darin Fisher (:fishd, Google)]

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.

[Adam Langley]

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

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46091 [details] without any errors.

[Adam Langley]

I summon Eric, who can hopefully r+ this because it's otherwise stagnating.

Cheers

[Darin Fisher (:fishd, Google)]

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

[Darin Fisher (:fishd, Google)]

> > 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 ;-)

[Eric Seidel (no email)]

Attachment 46091 [details] was posted by a committer and has review+, assigning to Adam Langley for commit.

[Adam Langley]

r55089

[Roman Tsisyk]

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

[Evan Martin]

Roman: I added a ChangeLog to your patch and uploaded it at https://bugs.webkit.org/show_bug.cgi?id=35495

[Adam Langley]

Thanks for that. Will review etc on Monday.