Bug 106013 - [Qt] Support kerning in fast path font rendering
Summary: [Qt] Support kerning in fast path font rendering
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks: 112704
  Show dependency treegraph
 
Reported: 2013-01-03 06:10 PST by Allan Sandfeld Jensen
Modified: 2013-03-19 09:09 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.40 KB, patch)
2013-01-03 06:14 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (5.93 KB, patch)
2013-03-14 08:36 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (7.58 KB, patch)
2013-03-19 02:28 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (7.78 KB, patch)
2013-03-19 03:06 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (7.74 KB, patch)
2013-03-19 03:28 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (5.25 KB, patch)
2013-03-19 07:00 PDT, Allan Sandfeld Jensen
jturcotte: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2013-01-03 06:10:30 PST
The fast path font rendering has support for kerning, but to do so requires the SimpleFontData::applyTransforms method. Fortunately the method is available in QRawFont under the name advancesForGlyphIndexes. By tweaking the GlyphBufferGlyph and GlyphBufferAdvance types similar to what Safari does, this method can be called directly.
Comment 1 Allan Sandfeld Jensen 2013-01-03 06:14:41 PST
Created attachment 181169 [details]
Patch
Comment 2 Allan Sandfeld Jensen 2013-01-03 06:25:07 PST
Comment on attachment 181169 [details]
Patch

This patch is missing a change to ensure kerning is set on QRawFont.
Comment 3 Allan Sandfeld Jensen 2013-01-03 08:43:54 PST
Actually it seems I have tested this incorrectly last time, or there have been a regression in Qt. QRawFont::advancesForGlyphIndexes does not currently perform kerning.
Comment 4 Allan Sandfeld Jensen 2013-03-14 08:36:21 PDT
Created attachment 193128 [details]
Patch

Updated to new proposed Qt 5.1 API
Comment 5 Allan Sandfeld Jensen 2013-03-19 02:28:34 PDT
Created attachment 193769 [details]
Patch
Comment 6 EFL EWS Bot 2013-03-19 02:50:29 PDT
Comment on attachment 193769 [details]
Patch

Attachment 193769 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17238256
Comment 7 Early Warning System Bot 2013-03-19 02:51:12 PDT
Comment on attachment 193769 [details]
Patch

Attachment 193769 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17116699
Comment 8 Early Warning System Bot 2013-03-19 02:56:07 PDT
Comment on attachment 193769 [details]
Patch

Attachment 193769 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17247024
Comment 9 WebKit Review Bot 2013-03-19 02:56:16 PDT
Comment on attachment 193769 [details]
Patch

Attachment 193769 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17194461
Comment 10 WebKit Review Bot 2013-03-19 03:03:01 PDT
Comment on attachment 193769 [details]
Patch

Attachment 193769 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17194463
Comment 11 Allan Sandfeld Jensen 2013-03-19 03:06:13 PDT
Created attachment 193772 [details]
Patch
Comment 12 EFL EWS Bot 2013-03-19 03:14:24 PDT
Comment on attachment 193772 [details]
Patch

Attachment 193772 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17160655
Comment 13 Allan Sandfeld Jensen 2013-03-19 03:28:50 PDT
Created attachment 193774 [details]
Patch

Do not use QT_VERSION_CHECK() in parts that other ports needs to parse
Comment 14 Pierre Rossi 2013-03-19 04:22:37 PDT
Comment on attachment 193774 [details]
Patch

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

LGTM

> Source/WebCore/platform/graphics/GlyphBuffer.h:95
> +struct GlyphBufferAdvance : public QPointF {

Just out of curiosity, why not use QSizeF here ? Wouldn't a simple typedef do the trick even since the function signatures match already ?
Comment 15 Allan Sandfeld Jensen 2013-03-19 04:34:28 PDT
(In reply to comment #14)
> (From update of attachment 193774 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193774&action=review
> 
> LGTM
> 
> > Source/WebCore/platform/graphics/GlyphBuffer.h:95
> > +struct GlyphBufferAdvance : public QPointF {
> 
> Just out of curiosity, why not use QSizeF here ? Wouldn't a simple typedef do the trick even since the function signatures match already ?

The problem is that QRawFont::advancesForGlyphIndexes takes a pointer to an QPointF array as an argument. So I want a GlyphBufferAdvance class that is also makes for a valid QPointF pointer.
Comment 16 Pierre Rossi 2013-03-19 06:16:47 PDT
Comment on attachment 193774 [details]
Patch

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

>>> Source/WebCore/platform/graphics/GlyphBuffer.h:95
>>> +struct GlyphBufferAdvance : public QPointF {
>> 
>> Just out of curiosity, why not use QSizeF here ? Wouldn't a simple typedef do the trick even since the function signatures match already ?
> 
> The problem is that QRawFont::advancesForGlyphIndexes takes a pointer to an QPointF array as an argument. So I want a GlyphBufferAdvance class that is also makes for a valid QPointF pointer.

Right. So there is no way around it indeed. so now we could also save a line in Font::drawGlyphs in FontQt.cpp ;)
Comment 17 Allan Sandfeld Jensen 2013-03-19 07:00:44 PDT
Created attachment 193818 [details]
Patch

Moving enabling kerning by default to a separate patch
Comment 18 Jocelyn Turcotte 2013-03-19 08:59:06 PDT
Comment on attachment 193818 [details]
Patch

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

> Source/WebCore/platform/graphics/SimpleFontData.h:209
> +#elif PLATFORM(QT) && QT_VERSION >= 0x050100

QT_VERSION_CHECK?

> Source/WebCore/platform/graphics/WidthIterator.h:69
> +#elif PLATFORM(QT) && QT_VERSION >= 0x050100

ditto

LGTM too for the rest, r=me.
Comment 19 Allan Sandfeld Jensen 2013-03-19 09:03:08 PDT
Committed r146209: <http://trac.webkit.org/changeset/146209>