Bug 78001 – [Qt] Use QRawFont when building with Qt 5

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Pierre Rossi]

[Qt] Use QRawFont when building with Qt 5

[Pierre Rossi]

Created attachment 125918 [details]
Patch

[WebKit Review Bot]

Attachment 125918 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/qt/FontQt4.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/qt/FontQt4.cpp:70:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/graphics/qt/FontQt4.cpp:115:  Declaration has space between type name and * in QPainter *p  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/qt/FontQt4.cpp:421:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/qt/FontQt4.cpp:423:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 5 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pierre Rossi]

This change depends on the following 2 pending changes in Qt 5:

http://codereview.qt-project.org/#change,13595
http://codereview.qt-project.org/#change,13596

[Pierre Rossi]

*** Bug 67131 has been marked as a duplicate of this bug. ***

[Simon Hausmann]

Comment on attachment 125918 [details]
Patch

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

Yay, this is looking very nice.

As a general suggestion I wonder if it would make sense to get rid of the HAVE(QRAWFONT) altogether and simply distinguish between Qt 4 and Qt 5? (take this with a grain of salt, it might not make the code easier to read, although it could ;)

A few other comments below.

> Source/WebCore/ChangeLog:15
> +        No new tests. (OOPS!)

Line to remove :)

> Source/WebCore/platform/graphics/Font.h:311
> +#if PLATFORM(QT) && !HAVE(QRAWFONT)
> +inline QFont Font::syntheticFont() const
> +{
> +    return font();
> +}
> +#endif

Should this perhaps go into FontQt4.cpp?

> Source/WebCore/platform/graphics/qt/FontQt.cpp:272
> +    if (m_wordSpacing) {
> +        range.format.setFontWordSpacing(m_wordSpacing);
> +        overrides << range;
> +    }
> +    if (m_letterSpacing) {
> +        range.format = QTextCharFormat();
> +        range.format.setFontLetterSpacing(m_letterSpacing);
> +        overrides << range;
> +    }
> +    if (typesettingFeatures() & Kerning) {
> +        range.format = QTextCharFormat();
> +        range.format.setFontKerning(true);
> +        overrides << range;
> +    }

I think for these three overrides you could share the same FormatRange with the same QTextCharFormat and set the spacing/kerning properties on the same QTextCharFormat instance.

> Source/WebCore/platform/graphics/qt/FontQt.cpp:286
> +        for (int i = 0; i <= text.length(); ++i) {
> +            if (!text.at(i).isLower() && previousCharWasLowerCase) {
> +                previousCharWasLowerCase = false;
> +                range.length = i - range.start;
> +                range.format.setFontCapitalization(QFont::SmallCaps);
> +                overrides << range;
> +            } else if (text.at(i).isLower() && !previousCharWasLowerCase) {
> +                previousCharWasLowerCase = true;
> +                range.format = QTextCharFormat();
> +                range.start = i;
> +            }

Is this necessary? Doesn't QTextEngine already take care of the setting smallcaps only on lowercase characters?

If my suspicion is true, then you might be able to get away with one single QTextLayout::FormatRange for all the font features, with start = 0 and length = text.length() and various properties set on the char format.

> Source/WebCore/platform/graphics/qt/FontQt.cpp:350
> +    QFont f = QFont(rawFont.familyName());

Easier to write QFont f(rawFont.familyName()) perhaps? :)

> Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:166
> +    m_fontMetrics.setDescent(qAbs(descent));

This magic piece deserves a comment :)

[Pierre Rossi]

Created attachment 126066 [details]
Patch

[WebKit Review Bot]

Attachment 126066 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/qt/FontQt4.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/qt/FontQt4.cpp:70:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/graphics/qt/FontQt4.cpp:115:  Declaration has space between type name and * in QPainter *p  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/qt/FontQt4.cpp:421:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/qt/FontQt4.cpp:423:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 5 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pierre Rossi]

(In reply to comment #5)
> (From update of attachment 125918 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125918&action=review
> 
> Yay, this is looking very nice.
> 
> As a general suggestion I wonder if it would make sense to get rid of the HAVE(QRAWFONT) altogether and simply distinguish between Qt 4 and Qt 5? (take this with a grain of salt, it might not make the code easier to read, although it could ;)
> 
That would mainly make the diff bigger since HAVE(QRAWFONT) guards were already in place before that. Since this cruft is bound to go away at some point, I didn't really see the need for changing it.
Also note that the fact that we are tied to a specific version of Qt for the switch is not set in stone too, as unlikely as it may sound, we might not have the necessary enablers in Qt 5.0 and need to differentiate between Qt 5.0 and Qt 5.1, having that distinction in one place makes the whole thing easier to revisit (FontQt4.cpp would be a poorly chosen name in that case !).

> A few other comments below.
> 

Addressed those in the updated patch.

[Simon Hausmann]

Comment on attachment 126066 [details]
Patch

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

I think the style issues should be fixed, too, despite them being in old code.

> Source/WebCore/platform/graphics/qt/FontQt.cpp:271
> +    if (isSmallCaps() || typesettingFeatures() & Kerning || m_wordSpacing || m_letterSpacing)
> +        overrides << range;
> +
> +    if (!overrides.isEmpty())
> +        layout->setAdditionalFormats(overrides);

How about combining this to:

if (range.format.propertyCount() == 0)
    return;

layout->setAdditionalProperties(QList<QTextLayout::FormatRange>() << range);

instead of using the same conditions (if isSmallCaps || typesetting...) twice.

> Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:166
> +    m_fontMetrics.setDescent(qAbs(descent));

Reminder: comment ;-)

[Pierre Rossi]

Created attachment 126112 [details]
Patch

[Simon Hausmann]

Comment on attachment 126112 [details]
Patch

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

Rest of the patch LGTM, just one nitpick left :)

> Source/WebCore/platform/graphics/qt/FontQt.cpp:271
> +    if (isSmallCaps() || typesettingFeatures() & Kerning || m_wordSpacing || m_letterSpacing)
> +        overrides << range;
> +
> +    if (!overrides.isEmpty())
> +        layout->setAdditionalFormats(overrides);

I suppose you forgot this part? :) I'm not so particular about the stylistic aspect of this function, but I'd like at least the if () to be simplified. We shouldn't duplicate the if (m_wordSpacing || isSmallCaps || ...) part.

[Pierre Rossi]

Created attachment 126739 [details]
Patch

[Pierre Rossi]

Created attachment 129700 [details]
Patch

removed the +1 for height calculation in FontQt.cpp since this is not correct anymore in Qt5 (see cb8445f in qtbase)

[Pierre Rossi]

Committed r113968: <http://trac.webkit.org/changeset/113968>

[Csaba Osztrogonác]

Reopen, because one test still fails after this change on Qt5-WK1 and on Qt5-WK2:

--- /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/svg/as-image/img-preserveAspectRatio-support-1-expected.txt
+++ /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/svg/as-image/img-preserveAspectRatio-support-1-actual.txt
@@ -10,8 +10,8 @@
               RenderText {#text} at (9,1) size 75x21
                 text run at (9,1) width 75: "viewBox?"
             RenderTableCell {TH} at (97,2) size 163x44 [bgcolor=#DDDD99] [r=0 c=1 rs=1 cs=1]
-              RenderText {#text} at (9,1) size 145x42
-                text run at (9,1) width 145: "preserve\x{AD}Aspect\x{AD}" + hyphen string "\x{2010}"
+              RenderText {#text} at (14,1) size 135x42
+                text run at (14,1) width 135: "preserve\x{AD}Aspect\x{AD}" + hyphen string "-"
                 text run at (61,22) width 41: "Ratio"
             RenderTableCell {TH} at (262,12) size 202x23 [bgcolor=#DDDD99] [r=0 c=2 rs=1 cs=1]
               RenderText {#text} at (78,1) size 46x21


-----------------------------------------------------------
Rebase patches did by Pierre:
http://trac.webkit.org/changeset/113982
http://trac.webkit.org/changeset/113993

[Csaba Osztrogonác]

I skipped the reamining failing test - http://trac.webkit.org/changeset/114237/trunk/LayoutTests/platform/qt-5.0/Skipped

Please unskip it with the proper fix.

[Ryosuke Niwa]

This patch resulted in roughly 15% performance improvements on Parser/html5-full-render.html :D

http://webkit-perf.appspot.com/graph.html#tests=[[3068,2001,957069],[3068,2001,963028]]&sel=1334204056904.4426,1334267563872.0579&displayrange=7&datatype=running

[Pierre Rossi]

(In reply to comment #17)
> This patch resulted in roughly 15% performance improvements on Parser/html5-full-render.html :D
> 
> http://webkit-perf.appspot.com/graph.html#tests=[[3068,2001,957069],[3068,2001,963028]]&sel=1334204056904.4426,1334267563872.0579&displayrange=7&datatype=running

Hehe, nice ! We're finally harvesting the fruits from the fast path it seems, I guess we have a bunch of people to thank for that. :)