Bug 88263 - Add support for enabling text subpixel positioning in Skia
Summary: Add support for enabling text subpixel positioning in Skia
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Normal
Assignee: Daniel Erat
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-04 15:55 PDT by Daniel Erat
Modified: 2012-06-14 15:01 PDT (History)
12 users (show)

See Also:


Attachments
Patch (15.59 KB, patch)
2012-06-04 17:46 PDT, Daniel Erat
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (720.26 KB, application/zip)
2012-06-05 06:16 PDT, WebKit Review Bot
no flags Details
Patch (15.65 KB, patch)
2012-06-05 09:45 PDT, Daniel Erat
no flags Details | Formatted Diff | Diff
Patch (53.73 KB, patch)
2012-06-05 15:32 PDT, Daniel Erat
no flags Details | Formatted Diff | Diff
Patch (49.29 KB, patch)
2012-06-05 16:10 PDT, Daniel Erat
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (789.67 KB, application/zip)
2012-06-06 07:52 PDT, WebKit Review Bot
no flags Details
Patch (52.72 KB, patch)
2012-06-06 15:04 PDT, Daniel Erat
no flags Details | Formatted Diff | Diff
Patch (4.71 KB, patch)
2012-06-13 09:13 PDT, Daniel Erat
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Erat 2012-06-04 15:55:26 PDT
The Skia backend supports subpixel-positioned text (via SkPaint::setSubpixelText()), but we currently lack a way to enable it.  I have a change that adds a useSubpixelPositioning field to FontRenderStyle and WebFontRenderStyle and makes FontPlatformDataHarfBuzz apply it to the SkPaint.  It also renames the existing useSubpixel fields to useSubpixelRendering.
Comment 1 Daniel Erat 2012-06-04 17:46:49 PDT
Created attachment 145665 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-04 17:48:03 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Behdad Esfahbod 2012-06-04 18:23:46 PDT
+    // FontConfig doesn't provide parameters to configure whether subpixel
+    // positioning should be used or not.
+    out->useSubpixelPositioning = 2;

A better default may be to enable this if hinting is off?
Comment 4 WebKit Review Bot 2012-06-05 06:16:28 PDT
Comment on attachment 145665 [details]
Patch

Attachment 145665 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12901496

New failing tests:
fonts/fantasy.html
fast/css/font-face-synthetic-bold-italic.html
fast/text/fake-italic.html
fonts/cursive.html
fast/css/font-face-multiple-faces.html
Comment 5 WebKit Review Bot 2012-06-05 06:16:32 PDT
Created attachment 145778 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 6 Daniel Erat 2012-06-05 08:05:19 PDT
#3: Not sure.  That seems like a fairly substantial change in behavior; I'd prefer for this change to be a no-op for now except in the case where we explicitly request subpixel positioning.
Comment 7 Behdad Esfahbod 2012-06-05 08:06:45 PDT
Fair enough.  I'll go request an element for this in Fontconfig...
Comment 8 Daniel Erat 2012-06-05 09:45:32 PDT
Created attachment 145823 [details]
Patch
Comment 9 Daniel Erat 2012-06-05 09:50:22 PDT
The patch from #8 makes SimpleFontData::platformWidthForGlyph() again round widths, but now only when subpixel positioning is disabled.  It looks like SkPaint::measureText() is returning fractional widths for fake-italics (and maybe also fake-bold?).  Behdad suggests that Skia should be doing the rounding itself when subpixel positioning is disabled; I think that ensuring that this is the case within WebKit is safe.
Comment 10 Tony Chang 2012-06-05 14:20:10 PDT
Comment on attachment 145823 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145823&action=review

Code change looks fine.  You'll have to wait for an API reviewer to take a look at this change.

> Source/WebCore/ChangeLog:13
> +        Pass setting to SkPaint in FontPlatformDataHarfBuzz and remove
> +        round() call when computing glyph widths in SimpleFontDataSkia.
> +

Please add a sentence about why there are no new tests.  E.g., No new tests because this shouldn't change page rendering.  It might be possible to write a gtest style test (see the Source/WebKit/chromium/tests directory) for what is exposed via WebFontRenderStyle.h, but I'm not sure if you can verify that the value was properly set.

> Source/WebCore/platform/graphics/chromium/FontRenderStyle.h:49
> -          useSubpixel(0) { }
> +          useSubpixelRendering(0),
> +          useSubpixelPositioning(0) { }

Nit: WebKit style normally puts the , in front (lines up below the :).
Comment 11 Daniel Erat 2012-06-05 15:32:45 PDT
Created attachment 145881 [details]
Patch
Comment 12 Tony Chang 2012-06-05 15:41:22 PDT
Comment on attachment 145881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145881&action=review

> LayoutTests/platform/chromium-linux/fast/text/chromium-linux-text-subpixel-positioning.html:3
> +    <p>This test requires Chromium Linux <tt>test_shell</tt> in

Nit: I would change this text to "This test requires DumpRenderTree, as that ..."
Comment 13 Daniel Erat 2012-06-05 16:10:29 PDT
Created attachment 145890 [details]
Patch
Comment 14 James Robinson 2012-06-05 16:34:19 PDT
Comment on attachment 145890 [details]
Patch

Public WebKit API lgtm
Comment 15 WebKit Review Bot 2012-06-06 07:51:54 PDT
Comment on attachment 145890 [details]
Patch

Attachment 145890 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12897824

New failing tests:
platform/chromium-linux/fast/text/chromium-linux-text-subpixel-positioning.html
Comment 16 WebKit Review Bot 2012-06-06 07:52:13 PDT
Created attachment 146030 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 17 Daniel Erat 2012-06-06 15:04:55 PDT
Created attachment 146119 [details]
Patch
Comment 18 Tony Chang 2012-06-06 15:09:23 PDT
Comment on attachment 146119 [details]
Patch

Adding a new entry to fonts.conf seems fine.
Comment 19 WebKit Review Bot 2012-06-07 09:59:00 PDT
Comment on attachment 146119 [details]
Patch

Clearing flags on attachment: 146119

Committed r119732: <http://trac.webkit.org/changeset/119732>
Comment 20 WebKit Review Bot 2012-06-07 09:59:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Daniel Erat 2012-06-13 09:12:58 PDT
Reopening to attach new patch.
Comment 22 Daniel Erat 2012-06-13 09:13:02 PDT
Created attachment 147334 [details]
Patch
Comment 23 Daniel Erat 2012-06-13 09:13:43 PDT
The new patch removes a field that was deprecated by this change.
Comment 24 Adam Barth 2012-06-13 09:17:48 PDT
(In reply to comment #21)
> Reopening to attach new patch.

Normally we use a new bug for each patch.
Comment 25 WebKit Review Bot 2012-06-14 15:01:13 PDT
Comment on attachment 147334 [details]
Patch

Clearing flags on attachment: 147334

Committed r120363: <http://trac.webkit.org/changeset/120363>
Comment 26 WebKit Review Bot 2012-06-14 15:01:19 PDT
All reviewed patches have been landed.  Closing bug.