We should implement WebKit's fast font path using the new QGlyphs API in Qt 4.8.
Created attachment 76646 [details] Patch (work in progress) Uploading my current WIP for feedback.
Comment on attachment 76646 [details] Patch (work in progress) View in context: https://bugs.webkit.org/attachment.cgi?id=76646&action=review > WebCore/WebCore.pro:3399 > +greaterThan(QT_MAJOR_VERSION,3):greaterThan(QT_MINOR_VERSION, 7):HAVE_QGLYPHS=1 Hmm, Qt > 3.7 ? > WebCore/platform/graphics/qt/FontQt.cpp:119 > + textFillPen = fillPenForContext(ctx); Tab? > WebCore/platform/graphics/qt/FontQt.cpp:314 > + float x1 = line.cursorToX(from); > + float x2 = line.cursorToX(to); > + if (x2 < x1) > + qSwap(x1, x2); Why not taking the min and max?
> > WebCore/WebCore.pro:3399 > > +greaterThan(QT_MAJOR_VERSION,3):greaterThan(QT_MINOR_VERSION, 7):HAVE_QGLYPHS=1 > > Hmm, Qt > 3.7 ? >= 4.8 actually. Don't get me started on qmake syntax... > Tab? grmbl.. oops :) > > WebCore/platform/graphics/qt/FontQt.cpp:314 > > + float x1 = line.cursorToX(from); > > + float x2 = line.cursorToX(to); > > + if (x2 < x1) > > + qSwap(x1, x2); > > Why not taking the min and max? This code was just moved in this patch.
Comment on attachment 76646 [details] Patch (work in progress) View in context: https://bugs.webkit.org/attachment.cgi?id=76646&action=review >> WebCore/WebCore.pro:3399 >> +greaterThan(QT_MAJOR_VERSION,3):greaterThan(QT_MINOR_VERSION, 7):HAVE_QGLYPHS=1 > > Hmm, Qt > 3.7 ? a more readable version check: !lessThan(QT_MAJOR_VERSION,4):!lessThan(QT_MINOR_VERSION, 8):HAVE_QGLYPHS=1
Created attachment 78118 [details] Patch (work in progress) Rebased patch.
Created attachment 86212 [details] Proposed patch
Created attachment 86213 [details] Proposed patch
Attachment 86213 [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/FontQt.cpp:329: 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/FontQt.cpp:331: 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: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 86213 [details] did not build on qt: Build output: http://queues.webkit.org/results/8203370
Comment on attachment 86213 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=86213&action=review Would be nice if this compiled on Qt 4.7 as well, which is what I suppose the buildbots are running? > Source/WebCore/WebCore.pro:3086 > +!lessThan(QT_MAJOR_VERSION,4):!lessThan(QT_MINOR_VERSION, 8):HAVE_QGLYPHS=1 why no space before 4? > Source/WebCore/platform/graphics/Font.cpp:151 > +#if PLATFORM(QT) && HAVE(QGLYPHS) > + if (context->textDrawingMode() & TextModeStroke || context->contextShadow()->m_type != ContextShadow::NoShadow) > + codePathToUse = Complex; > +#endif Why is this not in the codePath method? any particular reason? > Source/WebCore/platform/graphics/qt/FontQt.cpp:353 > + // Shadowed text should always take the complex path. Text with shadow? Shadowed sounds a bit strange to me > Source/WebCore/platform/graphics/qt/FontQt.cpp:359 > + QVector<quint32> glyphIndexes; Indices? > Source/WebCore/platform/graphics/qt/GlyphPageTreeNodeQt.cpp:54 > +\ What is this for?
(In reply to comment #10) > Would be nice if this compiled on Qt 4.7 as well, which is what I suppose the buildbots are running? That's a must-have obviously. > > Source/WebCore/WebCore.pro:3086 > > +!lessThan(QT_MAJOR_VERSION,4):!lessThan(QT_MINOR_VERSION, 8):HAVE_QGLYPHS=1 > > why no space before 4? Will fix. Though now that I think of it, we could just use QT_VERSION_CHECK and check for 4.8 instead of using a HAVE(QGLYPHS) macro.. > > Source/WebCore/platform/graphics/Font.cpp:151 > > +#if PLATFORM(QT) && HAVE(QGLYPHS) > > + if (context->textDrawingMode() & TextModeStroke || context->contextShadow()->m_type != ContextShadow::NoShadow) > > + codePathToUse = Complex; > > +#endif > > Why is this not in the codePath method? any particular reason? The codePath method doesn't have access to a GraphicsContext. > > Source/WebCore/platform/graphics/qt/FontQt.cpp:353 > > + // Shadowed text should always take the complex path. > > Text with shadow? Shadowed sounds a bit strange to me Agree, will change. > > Source/WebCore/platform/graphics/qt/FontQt.cpp:359 > > + QVector<quint32> glyphIndexes; > > Indices? Both are valid words, I would normally call it Indices myself, but in this case the Qt API calls them glyphIndexes so I used the same name for the variable. > > Source/WebCore/platform/graphics/qt/GlyphPageTreeNodeQt.cpp:54 > > +\ > > What is this for? It improves performance about 200%. :)
> > > +\ > > > > What is this for? > > It improves performance about 200%. :) Then let's add some more of those \ in the code :-)
Comment on attachment 86213 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=86213&action=review > Source/WebCore/platform/graphics/qt/GlyphPageTreeNodeQt.cpp:67 > + for (unsigned i = 0; i < length; ++i) { > + Glyph glyph = getGlyphForCharacterAndFont(qstring.at(i), font); Wouldn't it be nice to get the glyphs for the page in one shot, instead of creating a QTextLayout (that'll heap alloc in the impl) for each character? :)
(In reply to comment #13) > (From update of attachment 86213 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=86213&action=review > > > Source/WebCore/platform/graphics/qt/GlyphPageTreeNodeQt.cpp:67 > > + for (unsigned i = 0; i < length; ++i) { > > + Glyph glyph = getGlyphForCharacterAndFont(qstring.at(i), font); > > Wouldn't it be nice to get the glyphs for the page in one shot, instead of creating a QTextLayout (that'll heap alloc in the impl) for each character? :) Most definitely. For anyone passing by, this bug is currently pending new API in Qt before deciding how to continue. :)
Created attachment 91928 [details] Proposed patch v2 This patch uses the new QRawFont APIs in the fire-team branch of Qt 4.8.
Attachment 91928 [details] did not build on qt: Build output: http://queues.webkit.org/results/8534073
Created attachment 91933 [details] Proposed patch v3 Let's try to make that build on Qt 4.7..
Attachment 91933 [details] did not build on qt: Build output: http://queues.webkit.org/results/8531707
(In reply to comment #18) > Attachment 91933 [details] did not build on qt: > Build output: http://queues.webkit.org/results/8531707 Sigh. I'll leave this here for Simon to look at, since it will likely need multiple iterations anyway.
Created attachment 92225 [details] Proposed patch v4 Updated patch after IRL review with Simon. Renamed HAVE(QGLYPHS) to HAVE(QRAWFONT) for inexplicable aesthetic reasons. The GlyphPage::fill() friend hack is removed in favor of implementing SimpleFontData::platformWidthForGlyph(). As a side-effect, advances for zero-size fonts are now 0 (as they should be.) I have requested an API to disable unneeded caching in Qt's font engines, but this is not yet available, and will be used in a follow-up patch.
Attachment 92225 [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/FontCacheQt.cpp:101: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #21) > Attachment 92225 [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/FontCacheQt.cpp:101: Should have a space between // and comment [whitespace/comments] [4] Whoops, that's my bad. Would not land with this, obviously. :)
Comment on attachment 92225 [details] Proposed patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=92225&action=review r=me Follow-up stuff: 1) Implement stroking and shadowing for drawGlyphs (maybe in Qt) 2) Extend QRawFont APIs so that we can pass const QChar* str, int len and get glyphs/advances in out-arrays, so that we can use QVarLengthArray on the WebKit side 3) Other stuff I forgot ;) >> Source/WebCore/platform/graphics/qt/FontCacheQt.cpp:101 >> + //return getCachedFontData(fontDescription, fallbackFamily); > > Should have a space between // and comment [whitespace/comments] [4] Remove this before landing :) > Source/WebCore/platform/graphics/qt/FontQt.cpp:337 > + QVector<quint32> glyphIndexes; > + QVector<QPointF> positions; These should be QVarLengthArrays. Fix before landing I'd say. > Source/WebCore/platform/graphics/qt/FontQt.cpp:441 > +bool Font::primaryFontHasGlyphForCharacter(UChar32) const > +{ > + notImplemented(); > + return true; > +} This should be pretty easy to implement now, no? > Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:138 > + QVector<quint32> indexes = rawFont.glyphIndexesForString(QLatin1String(" ")); Urgh, I think we really need QRawFont::glyphIndexForCharacter, or better: an overload that takes a const QChar* and int len.
(In reply to comment #23) > > Source/WebCore/platform/graphics/qt/FontQt.cpp:441 > > +bool Font::primaryFontHasGlyphForCharacter(UChar32) const > > +{ > > + notImplemented(); > > + return true; > > +} > > This should be pretty easy to implement now, no? Forget this comment please :)
Committed r85853: <http://trac.webkit.org/changeset/85853>
With the latest Qt 4.8 I got this fail: ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp: In member function ‘void WebCore::SimpleFontData::platformInit()’: ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:135: error: ‘class QRawFont’ has no member named ‘xHeight’ ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:136: error: ‘class QRawFont’ has no member named ‘leading’ ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp: In member function ‘void WebCore::SimpleFontData::platformCharWidthInit()’: ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:178: error: ‘class QRawFont’ has no member named ‘averageCharWidth’ ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:179: error: ‘class QRawFont’ has no member named ‘maxCharWidth’ What kind of Qt do you use? :o Is it a hyper secret internal version or what?
(In reply to comment #26) > With the latest Qt 4.8 I got this fail: > > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp: In member function ‘void WebCore::SimpleFontData::platformInit()’: > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:135: error: ‘class QRawFont’ has no member named ‘xHeight’ > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:136: error: ‘class QRawFont’ has no member named ‘leading’ > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp: In member function ‘void WebCore::SimpleFontData::platformCharWidthInit()’: > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:178: error: ‘class QRawFont’ has no member named ‘averageCharWidth’ > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:179: error: ‘class QRawFont’ has no member named ‘maxCharWidth’ > > > What kind of Qt do you use? :o Is it a hyper secret internal version or what? I'm using the fire-team pre-integration repository, not sure when it will be merged into mainline Qt. Jiang?
(In reply to comment #27) > (In reply to comment #26) > > With the latest Qt 4.8 I got this fail: > > > > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp: In member function ‘void WebCore::SimpleFontData::platformInit()’: > > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:135: error: ‘class QRawFont’ has no member named ‘xHeight’ > > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:136: error: ‘class QRawFont’ has no member named ‘leading’ > > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp: In member function ‘void WebCore::SimpleFontData::platformCharWidthInit()’: > > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:178: error: ‘class QRawFont’ has no member named ‘averageCharWidth’ > > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:179: error: ‘class QRawFont’ has no member named ‘maxCharWidth’ > > > > > > What kind of Qt do you use? :o Is it a hyper secret internal version or what? > > I'm using the fire-team pre-integration repository, not sure when it will be merged into mainline Qt. Jiang? It seems that last integration attempt was blocked by some configuration problem of our internal CI server, my guess is that it will be integrated to mainline Qt in the next round, which means a couple of days or so.
I absolutely confused ... Is there a policy somewhere for what can be landed in WebKit trunk or not? I thought Qt 4.8 means 4.8 branch on gitorious. But now we can't get regressions on trunk until QRawFont changes goes to the 4.8 branch and I rebuild Qt and install it to the bot. It would be better for buildbot if we require that WebKit trunk should be build with Qt 4.8 weekly build from 4.8 branch.
kling++! woot!