Bug 46972 – Add two new helper files for the new SVGTextLayoutEngine

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

[Nikolas Zimmermann]

Add SVGTextLayoutEngineBaseline & SVGTextLayoutEngineSpacing, which will be used & owned by the new SVGTextLayoutEngine.

[Nikolas Zimmermann]

Created attachment 69446 [details]
Patch

[WebKit Review Bot]

Attachment 69446 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/svg/SVGTextLayoutEngineBaseline.cpp:158:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
WebCore/rendering/svg/SVGTextLayoutEngineBaseline.cpp:181:  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/svg/SVGTextLayoutEngineBaseline.cpp:199:  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/svg/SVGTextLayoutEngineBaseline.cpp:210:  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/svg/SVGTextLayoutEngineBaseline.cpp:226:  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/svg/SVGTextLayoutEngineSpacing.cpp:82:  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: 6 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Created attachment 69447 [details]
Patch v2

Fix style issues.

[Dirk Schulze]

Comment on attachment 69447 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=69447&action=review

> WebCore/rendering/svg/SVGTextLayoutEngineBaseline.cpp:146
> +        return m_font.ascent() * 8.0f / 10.0f;
> +    case AB_MATHEMATICAL:
> +        return m_font.ascent() / 2;
> +    default:

On one line you use / 2, on the other line / 10.0f. Please use a common style here. I think it's more save to use / 2.f instead of / 2, but it's your decision.

> WebCore/rendering/svg/SVGTextLayoutEngineBaseline.cpp:166
> +    case GO_AUTO:
> +        {
> +            // Spec: Fullwidth ideographic and fullwidth Latin text will be set with a glyph-orientation of 0-degrees.
> +            // Text which is not fullwidth will be set with a glyph-orientation of 90-degrees.
> +            unsigned int unicodeRange = findCharUnicodeRange(character);
> +            if (unicodeRange == cRangeSetLatin || unicodeRange == cRangeArabic)
> +                return 90;
> +
> +            return 0;
> +        }

The indention looks wrong here.

[Nikolas Zimmermann]

(In reply to comment #4)
> (From update of attachment 69447 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69447&action=review
> 
> > WebCore/rendering/svg/SVGTextLayoutEngineBaseline.cpp:146
> > +        return m_font.ascent() * 8.0f / 10.0f;
> > +    case AB_MATHEMATICAL:
> > +        return m_font.ascent() / 2;
> > +    default:
> 
> On one line you use / 2, on the other line / 10.0f. Please use a common style here. I think it's more save to use / 2.f instead of / 2, but it's your decision.
Well the common style is to avoid .f where possible. But as 8 / 10 gives me an integer, I need to use these postfixes.

> 
> > WebCore/rendering/svg/SVGTextLayoutEngineBaseline.cpp:166
> > +    case GO_AUTO:
> > +        {
> > +            // Spec: Fullwidth ideographic and fullwidth Latin text will be set with a glyph-orientation of 0-degrees.
> > +            // Text which is not fullwidth will be set with a glyph-orientation of 90-degrees.
> > +            unsigned int unicodeRange = findCharUnicodeRange(character);
> > +            if (unicodeRange == cRangeSetLatin || unicodeRange == cRangeArabic)
> > +                return 90;
> > +
> > +            return 0;
> > +        }
> 
> The indention looks wrong here.
If you check the last patch, I did that on purpose, otherwhise the style bot moans.

[Nikolas Zimmermann]

Created attachment 69448 [details]
Patch v3

We agreed to ignore the style warning regarding indentation in a switch, it looks awkward, and the coding style guidelines don't say a word about it.

[Dirk Schulze]

Comment on attachment 69448 [details]
Patch v3

LGTM. r=me

[Nikolas Zimmermann]

Landed in r68878.