Move buildLayoutInformationForTextBox() from SVGRootInlineBox in SVGInlineTextBox where it belongs
Created attachment 55534 [details] Patch
(In reply to comment #1) > Created an attachment (id=55534) [details] > Patch Please upload a new patch. XCode changes can't be updated.
(In reply to comment #2) > (In reply to comment #1) > > Created an attachment (id=55534) [details] [details] > > Patch > > Please upload a new patch. XCode changes can't be updated. Patch looks good on the first view. Will review the new patch, once it is up.
Created attachment 55548 [details] Patch
Attachment 55548 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/rendering/SVGRootInlineBox.cpp:961: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGInlineTextBox.cpp:686: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGInlineTextBox.cpp:694: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGInlineTextBox.cpp:767: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGInlineTextBox.cpp:770: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGTextLayoutUtilities.cpp:185: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGTextLayoutUtilities.cpp:202: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGTextLayoutUtilities.cpp:221: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGTextLayoutUtilities.cpp:245: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGTextLayoutUtilities.cpp:355: 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: 10 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 55548 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2227109
Comment on attachment 55548 [details] Patch WebCore/rendering/SVGTextLayoutUtilities.cpp:347 + kerning = svgFont->horizontalKerningForPairOfStringsAndGlyphs(lastGlyph.unicode, lastGlyph.glyphName, unicodeString, glyphName); This is a code change, that also changes behavior. I agree, that the Spec should be interpreted that way (either vkern or hkern). But this changes current behavior, where we allow vkern on horizontal texts. Please add a layout-test for this. Shouldn't be hard to make a combination of svg/text/text-vkern ans svg/text/text-hkern. text-vkern already has horizontal texts in it. You may just add hkern in the font defs and update the results. The patch looks great. Please fix the build error on chromium, add/modify the test and I can r+ the test.
(In reply to comment #7) > (From update of attachment 55548 [details]) > WebCore/rendering/SVGTextLayoutUtilities.cpp:347 > + kerning = svgFont->horizontalKerningForPairOfStringsAndGlyphs(lastGlyph.unicode, lastGlyph.glyphName, unicodeString, glyphName); > This is a code change, that also changes behavior. I agree, that the Spec should be interpreted that way (either vkern or hkern). But this changes current behavior, where we allow vkern on horizontal texts. This is wrong, before the code looked like: - if (style->font().isSVGFont()) { - if (lastGlyph.isValid) { - horizontalKerning = svgFont->horizontalKerningForPairOfStringsAndGlyphs(lastGlyph.unicode, lastGlyph.glyphName, unicodeString, glyphName); - if (isVerticalText) - verticalKerning = svgFont->verticalKerningForPairOfStringsAndGlyphs(lastGlyph.unicode, lastGlyph.glyphName, unicodeString, glyphName); You always only grabbed the vkern information for vertical text. The only thing which has changed, and you're right about that: hkern could be applied to vertical text before and this is not possible anymore, so I'm going to update the vkern test, to include a hkern element, to assure it's not applied anywhere. While I'm at it I'll do the same for the hkern test, adding a vkern element, to be sure we're not applying it.
(In reply to comment #8) > (In reply to comment #7) > > (From update of attachment 55548 [details] [details]) > > WebCore/rendering/SVGTextLayoutUtilities.cpp:347 > > + kerning = svgFont->horizontalKerningForPairOfStringsAndGlyphs(lastGlyph.unicode, lastGlyph.glyphName, unicodeString, glyphName); > > This is a code change, that also changes behavior. I agree, that the Spec should be interpreted that way (either vkern or hkern). But this changes current behavior, where we allow vkern on horizontal texts. > This is wrong, before the code looked like: > - if (style->font().isSVGFont()) { > - if (lastGlyph.isValid) { > - horizontalKerning = svgFont->horizontalKerningForPairOfStringsAndGlyphs(lastGlyph.unicode, lastGlyph.glyphName, unicodeString, glyphName); > - if (isVerticalText) > - verticalKerning = svgFont->verticalKerningForPairOfStringsAndGlyphs(lastGlyph.unicode, lastGlyph.glyphName, unicodeString, glyphName); > > You always only grabbed the vkern information for vertical text. > The only thing which has changed, and you're right about that: hkern could be applied to vertical text before and this is not possible anymore, so I'm going to update the vkern test, to include a hkern element, to assure it's not applied anywhere. While I'm at it I'll do the same for the hkern test, adding a vkern element, to be sure we're not applying it. Even if line by line reviewing is a real improvement, sometimes it's better to mark multiple lines :-) I meant the if (isVerticalText) thing, not the horizontalKerning call it self.
Created attachment 55681 [details] Updated patch
Attachment 55681 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/rendering/SVGInlineTextBox.cpp:686: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGInlineTextBox.cpp:694: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGInlineTextBox.cpp:767: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGInlineTextBox.cpp:770: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGRootInlineBox.cpp:962: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGTextLayoutUtilities.cpp:185: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGTextLayoutUtilities.cpp:202: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGTextLayoutUtilities.cpp:221: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGTextLayoutUtilities.cpp:245: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGTextLayoutUtilities.cpp:355: 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: 10 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #11) > Attachment 55681 [details] did not pass style-queue: > > Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 > WebCore/rendering/SVGInlineTextBox.cpp:686: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] > WebCore/rendering/SVGInlineTextBox.cpp:694: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] > WebCore/rendering/SVGInlineTextBox.cpp:767: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] > WebCore/rendering/SVGInlineTextBox.cpp:770: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] > WebCore/rendering/SVGRootInlineBox.cpp:962: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] > WebCore/rendering/SVGTextLayoutUtilities.cpp:185: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] > WebCore/rendering/SVGTextLayoutUtilities.cpp:202: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] > WebCore/rendering/SVGTextLayoutUtilities.cpp:221: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] > WebCore/rendering/SVGTextLayoutUtilities.cpp:245: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] > WebCore/rendering/SVGTextLayoutUtilities.cpp:355: 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: 10 in 21 files Argl, stupid rule. We _do_ want to check spacing != 0.0f, not !spacing.
Comment on attachment 55681 [details] Updated patch WebCore/rendering/SVGInlineTextBox.cpp:718 + if (info.pathTextLength > 0.0f && info.pathChunkLength > 0.0f) { Maybe the style bot was right here (info.pathTextLength && info.pathChunkLength) :-) WebCore/rendering/SVGInlineTextBox.cpp:252 + if (closestOffsetInBox < (int) end() && direction() == RTL) not static_cast<int> here? WebCore/rendering/SVGTextLayoutUtilities.cpp:164 + // Text which is not fullwidth will be set with a glyph-orientation of 90-degrees. IIRC we don't want leading spaces here. WebCore/rendering/SVGTextLayoutUtilities.cpp:322 + if (primitive->primitiveType() == CSSPrimitiveValue::CSS_PERCENTAGE && font.pixelSize() > 0) font.pixelSize() > 0 the same like above Patch looks goo. r=me, but please think about the style hints.
(In reply to comment #13) > (From update of attachment 55681 [details]) > WebCore/rendering/SVGInlineTextBox.cpp:718 > + if (info.pathTextLength > 0.0f && info.pathChunkLength > 0.0f) { > Maybe the style bot was right here (info.pathTextLength && info.pathChunkLength) :-) > > WebCore/rendering/SVGInlineTextBox.cpp:252 > + if (closestOffsetInBox < (int) end() && direction() == RTL) > not static_cast<int> here? > > WebCore/rendering/SVGTextLayoutUtilities.cpp:164 > + // Text which is not fullwidth will be set with a glyph-orientation of 90-degrees. > IIRC we don't want leading spaces here. > > WebCore/rendering/SVGTextLayoutUtilities.cpp:322 > + if (primitive->primitiveType() == CSSPrimitiveValue::CSS_PERCENTAGE && font.pixelSize() > 0) > font.pixelSize() > 0 the same like above > > Patch looks goo. r=me, but please think about the style hints. Fixed those issues, thanks for pointing them out! (I initially did not plan to fix them, as I was just moving code, but as you're fine with it, I'm okay with it too :-)
Committed r59148: <http://trac.webkit.org/changeset/59148>