Bug 38799 - Move buildLayoutInformationForTextBox() from SVGRootInlineBox in SVGInlineTextBox where it belongs
Summary: Move buildLayoutInformationForTextBox() from SVGRootInlineBox in SVGInlineTex...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-08 01:21 PDT by Nikolas Zimmermann
Modified: 2010-05-11 05:12 PDT (History)
3 users (show)

See Also:


Attachments
Patch (71.00 KB, patch)
2010-05-10 03:19 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (71.27 KB, patch)
2010-05-10 06:20 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (109.11 KB, patch)
2010-05-11 02:45 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2010-05-08 01:21:45 PDT
Move buildLayoutInformationForTextBox() from SVGRootInlineBox in SVGInlineTextBox where it belongs
Comment 1 Nikolas Zimmermann 2010-05-10 03:19:00 PDT
Created attachment 55534 [details]
Patch
Comment 2 Dirk Schulze 2010-05-10 04:05:53 PDT
(In reply to comment #1)
> Created an attachment (id=55534) [details]
> Patch

Please upload a new patch. XCode changes can't be updated.
Comment 3 Dirk Schulze 2010-05-10 05:00:36 PDT
(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.
Comment 4 Nikolas Zimmermann 2010-05-10 06:20:23 PDT
Created attachment 55548 [details]
Patch
Comment 5 WebKit Review Bot 2010-05-10 06:24:16 PDT
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.
Comment 6 WebKit Review Bot 2010-05-10 07:23:51 PDT
Attachment 55548 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2227109
Comment 7 Dirk Schulze 2010-05-10 08:54:12 PDT
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.
Comment 8 Nikolas Zimmermann 2010-05-11 01:45:09 PDT
(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.
Comment 9 Dirk Schulze 2010-05-11 01:53:40 PDT
(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.
Comment 10 Nikolas Zimmermann 2010-05-11 02:45:46 PDT
Created attachment 55681 [details]
Updated patch
Comment 11 WebKit Review Bot 2010-05-11 02:47:07 PDT
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.
Comment 12 Nikolas Zimmermann 2010-05-11 02:50:24 PDT
(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 13 Dirk Schulze 2010-05-11 04:21:06 PDT
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.
Comment 14 Nikolas Zimmermann 2010-05-11 05:02:34 PDT
(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 :-)
Comment 15 Nikolas Zimmermann 2010-05-11 05:12:02 PDT
Committed r59148: <http://trac.webkit.org/changeset/59148>