Bug 38799 – Move buildLayoutInformationForTextBox() from SVGRootInlineBox in SVGInlineTextBox where it belongs

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

Move buildLayoutInformationForTextBox() from SVGRootInlineBox in SVGInlineTextBox where it belongs

[Nikolas Zimmermann]

Created attachment 55534 [details]
Patch

[Dirk Schulze]

(In reply to comment #1)
> Created an attachment (id=55534) [details]
> Patch

Please upload a new patch. XCode changes can't be updated.

[Dirk Schulze]

(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.

[Nikolas Zimmermann]

Created attachment 55548 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 55548 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2227109

[Dirk Schulze]

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.

[Nikolas Zimmermann]

(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.

[Dirk Schulze]

(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.

[Nikolas Zimmermann]

Created attachment 55681 [details]
Updated patch

[WebKit Review Bot]

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.

[Nikolas Zimmermann]

(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.

[Dirk Schulze]

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.

[Nikolas Zimmermann]

(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 :-)

[Nikolas Zimmermann]

Committed r59148: <http://trac.webkit.org/changeset/59148>