Created attachment 61445 [details] patch fixing the issue Chromium's WebFontInfo::renderStyleForStrike() method, which is used to get font style settings from fontconfig on Linux, doesn't set the useSubpixel field in the WebFontRenderStyle struct that gets passed into it. As a result, we just use the struct's default value of 0, which disables subpixel rendering. The fix for this is pretty trivial; I'll work on writing a test.
I asked over IM, but for posterity: what about the other valid values in the FC_RGBA_ enum?
This can't have been completely broken because all hell would be broken loose on the bug tracker. Are you sure that the default isn't 'no-preference'? I could believe that. But, in gerneral, LGTM.
Hmm... That's what I suspected (see http://crosbug.com/4638#c1 paragraph 3), but then I thought that it couldn't account for the regression because that code hasn't changed recently. Perhaps, some other parts changed and revealed the problem in WebFontInfo. Anyway, it's good to know that we have a fix.
(In reply to comment #2) > This can't have been completely broken because all hell would be broken loose on the bug tracker. Are you sure that the default isn't 'no-preference'? I could believe that. As far as I read the code, it's set to no-preference (in which case Skia's default will be used for each font), which is another reason I thought not reading FC_RGBA is not a problem. Having written that, I've just found out why it regressed. In r62121 ( http://trac.webkit.org/changeset/62121), useSubpixel is initialized to 0 (OFF) in FontRenderStyle ctor. I think Daniel's patch is still necessary to support controlling per-strike subpiexl control (well...), but I guess we should init useSubpixel and other values to NO_PREFERENCE (2) rather than OFF.
I was about to upload a patch to set FontRenderStyle to NoPreference (except for hintStyle which is set to 0), but then rediscovered that for both sandbox and non-sandbox cases, useSubpixel in WebFontRenderStyle is set to the correct default of NoPreference (by calling setDefaults in renderStyleForStrikes). So, there's no point of touching this and I'm back to the head-scratching state (why this regressed in late June).
I think that the big picture of what's currently going on (probably more for my benefit than anyone else's) is: - Chrome's browser process uses GtkSettings to get the user's "gtk-xft-rgba" setting and maps it one-to-one to a value from RendererPreferencesSubpixelRenderingEnum (with a "system default" value for cases where we don't get back a recognizable value from GTK or the hint style is undefined). - RenderView::UpdateFontRenderingFromRendererPrefs() in renderer/render_view_linux.cc maps the RendererPreferences's enum to a bool (true for everything other than "system default" and "none") and passes that value to WebFontRendering::setSubpixelGlyphs(), which passes it to FontPlatformData::setSubpixelGlyphs(), which sets the global isSkiaSubpixelGlyphs bool in WebCore/platform/graphics/chromium/FontPlatformDataLinux.cpp. - When we eventually call out to WebFontInfo::renderStyleForStrike() to get the per-strike settings, we aren't currently looking at fontconfig's FC_RGBA value, so we leave the WebFontRenderStyle::useSubpixel member at its default value of 2 ("no preference expressed") -- I missed that this wasn't just a bool when looking at the code earlier. - Back in FontPlatformData::setupPaint(), we call SkPaint::setLCDRenderText() using the useSubpixel value if a preference was expressed, or the value from the isSkiaSubpixelGlyphs global otherwise. So I think that the problem is that when subpixel isn't configured correctly in GtkSettings (as is the case for Chrome OS), we use the default instead of using the setting from fontconfig if one is available. My previous patch is incorrect, I think, in that I should be using the "no preference expressed" 2 value in the case where there's no setting in fontconfig, so we'll fall back to whatever value we already have from GtkSettings. Also, it looks like we only use the subpixel order and orientation provided by GtkSettings (the only place I see this getting touched is RenderView::UpdateFontRenderingFromRendererPrefs()). Does it make sense to continue doing that, or do we want to find some way that we can use the order provided by fontconfig? I can't think of any reason why someone would want to configure *differing* subpixel orders in fontconfig, but I think that there may be no way to configure the order and orientation at all currently using only fontconfig.
When you get the chance, don't forget to upload a patch with the new (or amended) layout test.
Created attachment 61835 [details] Patch
+ (I believe that Chromium is always initializing this before using + it, but it scares me all the same.) [...] -static bool isSkiaAntiAlias = true, isSkiaSubpixelGlyphs; +static bool isSkiaAntiAlias = true; +static bool isSkiaSubpixelGlyphs = false; Yes, "static"s are always initialized to false. You might consider changing your changelog to not include "believe" in it. I think the change is still ok for clarity purposes though. + out->useSubpixel = 2; Is there not some enum that we should be using?
Thanks, I updated the changelist description. I couldn't find any enum values. From WebKit/chromium/public/linux/WebFontRenderStyle.h: struct WebFontRenderStyle { // Each of the use* members below can take one of three values: // 0: off // 1: on // 2: no preference expressed char useBitmaps; // use embedded bitmap strike if possible char useAutoHint; // use 'auto' hinting (FreeType specific) char useHinting; // hint glyphs to the pixel grid char hintStyle; // level of hinting, 0..3 char useAntiAlias; // antialias glyph shapes char useSubpixel; // use subpixel antialias ... Should I add some in this change?
No, the change looks good to me. If we do refactoring it should be in a different change anyway.
CC'ing a reviewer, the change is pretty obvious and LGTM to me
Comment on attachment 61835 [details] Patch > +++ b/LayoutTests/ChangeLog > + Layout test addition to check that Chromium Linux honors fontconfig > + settings enabling or disabling subpixel rendering per-strike. > + > + Subpixel rendering always disabled for Chromium Linux > + https://bugs.webkit.org/show_bug.cgi?id=42220 Nit: the detailed description should go below the bug line, i.e.: > + Subpixel rendering always disabled for Chromium Linux > + https://bugs.webkit.org/show_bug.cgi?id=42220 > + > + Layout test addition to check that Chromium Linux honors fontconfig > + settings enabling or disabling subpixel rendering per-strike. > +++ b/WebCore/ChangeLog > @@ -1,3 +1,16 @@ > + Initialize global Chromium Linux isSkiaSubpixelGlyphs flag to false. > + (I believe that Chromium is always initializing this before using > + it, but it scares me all the same.) > + > + Subpixel rendering always disabled for Chromium Linux > + https://bugs.webkit.org/show_bug.cgi?id=42220 Ditto. > + Honor Fontconfig subpixel rendering setting on Chromium Linux. > + > + Subpixel rendering always disabled for Chromium Linux > + https://bugs.webkit.org/show_bug.cgi?id=42220 And same here. :) When you've made these changes, you can post a new patch and any committer can set cq+ to put it in the commit queue. No need for another review.
Created attachment 61863 [details] Patch
Comment on attachment 61863 [details] Patch Rejecting patch 61863 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Ojan Vafai', u'--force']" exit_code: 255 Parsed 9 diffs from patch file(s). patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/platform/chromium-linux/platform/chromium/fast/text/chromium-linux-fontconfig-renderstyle-expected.checksum only literal type is supported now at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply line 248. Full output: http://webkit-commit-queue.appspot.com/results/3348749
I will attempt to land this patch myself on Monday.
r63780