[Qt] Use QRawFont when building with Qt 5
Created attachment 125918 [details] Patch
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.
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
*** Bug 67131 has been marked as a duplicate of this bug. ***
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 :)
Created attachment 126066 [details] Patch
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.
(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.
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 ;-)
Created attachment 126112 [details] Patch
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.
Created attachment 126739 [details] Patch
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)
Committed r113968: <http://trac.webkit.org/changeset/113968>
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
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.
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
(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. :)