Summary: | Support kerning with SVG web fonts | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Allan Sandfeld Jensen <allan.jensen> | ||||||||||||||||||
Component: | Text | Assignee: | Allan Sandfeld Jensen <allan.jensen> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, d-r, efidler, eflews.bot, esprehn+autocc, fmalita, glenn, gyuyoung.kim, mitz, pdr, philn, rego+ews, rniwa, schenney, tonyiiitm, xan.lopez, zimmermann | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | 100050 | ||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||
Attachments: |
|
Description
Allan Sandfeld Jensen
2013-06-12 06:32:33 PDT
Created attachment 204433 [details]
Patch
Comment on attachment 204433 [details] Patch Attachment 204433 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/809185 Comment on attachment 204433 [details] Patch Attachment 204433 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/652919 Created attachment 204437 [details]
Patch
Comment on attachment 204437 [details] Patch Attachment 204437 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/760450 Comment on attachment 204437 [details] Patch Attachment 204437 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/827091 Created attachment 204440 [details]
Patch
Add handling of Cairo glyphs
Comment on attachment 204433 [details] Patch Mid-air collision so not sure on line numbers etc. Where are the tests? View in context: https://bugs.webkit.org/attachment.cgi?id=204433&action=review > Source/WebCore/platform/graphics/WidthIterator.cpp:103 > +static inline float applyFontTransforms(GlyphBuffer* glyphBuffer, bool ltr, int& lastGlyphCount, const SimpleFontData* fontData, WidthIterator& iterator, TypesettingFeatures typesettingFeatures, CharactersTreatedAsSpace& charactersTreatedAsSpace) Why not make this non-static since you now need the "this" object? > Source/WebCore/svg/SVGFontElement.cpp:243 > + && !stringMatchesUnicodeRange(u2, svgKerning.unicodeRange2)) return (a || b || c); > Source/WebCore/svg/SVGFontElement.cpp:256 > + && !stringMatchesUnicodeRange(u2, svgKerning.unicodeRange2)) return d && !(a | b | c) (In reply to comment #8) > (From update of attachment 204433 [details]) > Mid-air collision so not sure on line numbers etc. Where are the tests? > I haven't figured out a way to test this reliably cross-port. A pixel-test is obviously possible, but I would prefer a layout or reference test. > View in context: https://bugs.webkit.org/attachment.cgi?id=204433&action=review > > > Source/WebCore/platform/graphics/WidthIterator.cpp:103 > > +static inline float applyFontTransforms(GlyphBuffer* glyphBuffer, bool ltr, int& lastGlyphCount, const SimpleFontData* fontData, WidthIterator& iterator, TypesettingFeatures typesettingFeatures, CharactersTreatedAsSpace& charactersTreatedAsSpace) > > Why not make this non-static since you now need the "this" object? > That was my original plan, but would require moving the definitions of OriginalAdvancesForCharacterTreatedAsSpace and CharactersTreatedAsSpace to the header file, so the end result wouldn't be any cleaner netto. > > Source/WebCore/svg/SVGFontElement.cpp:243 > > + && !stringMatchesUnicodeRange(u2, svgKerning.unicodeRange2)) > > return (a || b || c); > > > Source/WebCore/svg/SVGFontElement.cpp:256 > > + && !stringMatchesUnicodeRange(u2, svgKerning.unicodeRange2)) > > return d && !(a | b | c) That would make it cleaner I agree. Is this the same as bug 92949? (In reply to comment #10) > Is this the same as bug 92949? Yes, would be the same bug. (In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 204433 [details] [details]) > > Mid-air collision so not sure on line numbers etc. Where are the tests? > > > I haven't figured out a way to test this reliably cross-port. A pixel-test is obviously possible, but I would prefer a layout or reference test. In my experience it's almost impossible to test this kind of thing using a ref test, because different platforms will still render things at slightly different locations, enough to make it hard to generate a reliable reference. I think you'll need a pixel test, much as it pains me to say it. Someone from an actual WebKit using team should weigh in (I'm on Blink these days). > > View in context: https://bugs.webkit.org/attachment.cgi?id=204433&action=review > > > > > Source/WebCore/platform/graphics/WidthIterator.cpp:103 > > > +static inline float applyFontTransforms(GlyphBuffer* glyphBuffer, bool ltr, int& lastGlyphCount, const SimpleFontData* fontData, WidthIterator& iterator, TypesettingFeatures typesettingFeatures, CharactersTreatedAsSpace& charactersTreatedAsSpace) > > > > Why not make this non-static since you now need the "this" object? > > > > That was my original plan, but would require moving the definitions of OriginalAdvancesForCharacterTreatedAsSpace and CharactersTreatedAsSpace to the header file, so the end result wouldn't be any cleaner netto. Fair enough. > > > Source/WebCore/svg/SVGFontElement.cpp:243 > > > + && !stringMatchesUnicodeRange(u2, svgKerning.unicodeRange2)) > > > > return (a || b || c); > > > > > Source/WebCore/svg/SVGFontElement.cpp:256 > > > + && !stringMatchesUnicodeRange(u2, svgKerning.unicodeRange2)) > > > > return d && !(a | b | c) > > That would make it cleaner I agree. *** Bug 92949 has been marked as a duplicate of this bug. *** Created attachment 204572 [details]
Patch
Comment on attachment 204572 [details] Patch Attachment 204572 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/846103 Comment on attachment 204572 [details] Patch Attachment 204572 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/849118 Created attachment 204574 [details]
Patch
Sign of iterator again
Comment on attachment 204574 [details] Patch Attachment 204574 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/858194 New failing tests: fast/text/svg-font-face-with-kerning.html Created attachment 204645 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Comment on attachment 204574 [details] Patch Attachment 204574 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/851261 New failing tests: fast/text/svg-font-face-with-kerning.html Created attachment 204681 [details]
Archive of layout-test-results from APPLE-EWS-2 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-2 Port: win-future Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit
(In reply to comment #18) > (From update of attachment 204574 [details]) > Attachment 204574 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/858194 > > New failing tests: > fast/text/svg-font-face-with-kerning.html I should note, that the fails are missing baselines. The baseline is intentional missing and not marked as missing, because I will let the bots created the baseline and land that using garden-o-matic, but the bots will not create new pixel baselines for tests marked 'missing', so I will have to land the test failing in order to get the correct baselines after landing. Comment on attachment 204574 [details]
Patch
Fails due to missing baseline is intentional.
Created attachment 205488 [details]
Patch
Uploaded basic test results. Hopefully it will match since it is SVG rendering and not system font rendering.
With test and test expections, is there somebody who want to review the patch now? Or are there still issues to be solved? Ping for review? Comment on attachment 205488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205488&action=review One clarification requested. Otherwise good to go. > Source/WebCore/platform/graphics/WidthIterator.cpp:122 > + if (fontData->isSVGFont()) { Shouldn't this be "if (fontData->isSVGFont() && (typesettingFeatures & Kerning))" so that the applyTransforms gets called like it was previously? Or is that what the comment implies? As it is SimpleFontData::applyTransforms just returns false, and a code search in my checkout does not reveal any other implementations. (In reply to comment #27) > (From update of attachment 205488 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205488&action=review > > One clarification requested. Otherwise good to go. > > > Source/WebCore/platform/graphics/WidthIterator.cpp:122 > > + if (fontData->isSVGFont()) { > > Shouldn't this be "if (fontData->isSVGFont() && (typesettingFeatures & Kerning))" so that the applyTransforms gets called like it was previously? Or is that what the comment implies? > applySVGKerning is supposed to replace applyTransforms, but should not be called if only ligatures but not kerning is enabled. > As it is SimpleFontData::applyTransforms just returns false, and a code search in my checkout does not reveal any other implementations. ApplyTransforms used to have an ASSERT against handling SVG fonts but now just bails out. I would prefer not to calling it rather than relying on the method bailing out when asked to do something it can not handle. (In reply to comment #27) > (From update of attachment 205488 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205488&action=review > > Or is that what the comment implies? > The comment is just an explaination for why ligatures are not handled here. Since ligatures are handled as part of the glyph selection algorithm, when we have list of glyphe instead of a list of characters, any ligature transformation should already have been handled. Committed r156393: <http://trac.webkit.org/changeset/156393> Comment on attachment 205488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205488&action=review > Source/WebCore/platform/graphics/WidthIterator.cpp:125 > + iterator.run().renderingContext()->applySVGKerning(fontData, iterator, glyphBuffer, lastGlyphCount); It broke the !ENALBE(SVG_FONTS) build, because applySVGKerning is inside in this block: /ramdisk/qt-linux-release-minimal/build/Source/WebCore/platform/graphics/WidthIterator.cpp: In function 'float WebCore::applyFontTransforms(WebCore::GlyphBuffer*, bool, int&, const WebCore::SimpleFontData*, WebCore::WidthIterator&, WebCore::TypesettingFeatures, WebCore::CharactersTreatedAsSpace&)': /ramdisk/qt-linux-release-minimal/build/Source/WebCore/platform/graphics/WidthIterator.cpp:129:48: error: 'class WebCore::TextRun::RenderingContext' has no member named 'applySVGKerning' (In reply to comment #31) > (From update of attachment 205488 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205488&action=review > > > Source/WebCore/platform/graphics/WidthIterator.cpp:125 > > + iterator.run().renderingContext()->applySVGKerning(fontData, iterator, glyphBuffer, lastGlyphCount); > > It broke the !ENALBE(SVG_FONTS) build, because applySVGKerning is inside in this block: > /ramdisk/qt-linux-release-minimal/build/Source/WebCore/platform/graphics/WidthIterator.cpp: In function 'float WebCore::applyFontTransforms(WebCore::GlyphBuffer*, bool, int&, const WebCore::SimpleFontData*, WebCore::WidthIterator&, WebCore::TypesettingFeatures, WebCore::CharactersTreatedAsSpace&)': > /ramdisk/qt-linux-release-minimal/build/Source/WebCore/platform/graphics/WidthIterator.cpp:129:48: error: 'class WebCore::TextRun::RenderingContext' has no member named 'applySVGKerning' Right, fix commited in https://trac.webkit.org/r156399 |