Bug 45532 – Rewrite SVG text layout code

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

This is the master bug tracking the rewrite of the SVG text layout code.

[Nikolas Zimmermann]

Created attachment 69452 [details]
Patch

All preparations are done, welcome the final patch for the new layout engine!
It's already much faster, but there's so much room for improvment!

When RenderSVGText::layout() was called, we used to relayout the whole text tree, including all characters every time.
This is still the case, but it's very easy to cache the results now, as the layout process is now a three phase process. More details later.

This patch doesn't yet contain a ChangeLog nor the build system changes, and it also won't build so far. I'm waiting for my local build.
It's just a merge from my local "WebKit + new layout engine" branch into trunk. Uploading so Dirk can familiarize.

[Nikolas Zimmermann]

Created attachment 69463 [details]
Patch: WebCore

It's not meant to be reviewed yet, as it's still missing a ChangeLog, I just want to trigger EWS.

[Nikolas Zimmermann]

For the curious: I've been working on this patch the last 3 weeks. It fixes all known conceptual problems with the old layout engine - the code is much more readable, the patch is not, agreed.

All excuses to the poor soul Dirk!

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/svg/RenderSVGInlineText.cpp:138:  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/SVGTextLayoutEngine.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/rendering/svg/SVGTextChunkBuilder.cpp:38:  Missing space before {  [whitespace/braces] [5]
WebCore/rendering/svg/SVGTextLayoutAttributes.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/rendering/svg/SVGTextQuery.cpp:537:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/WebCore.pro:3291:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 6 in 35 files


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

[Dirk Schulze]

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

A really huge patch. You should stop working on SVGText. You suck the batteries of reviewers dry :-P Looks goo otherwise. Need to fix the mentioned issues of course.

> WebCore/rendering/SVGRenderTreeAsText.cpp:413
> +    for (unsigned i = 0; i < fragments.size(); ++i) {

save fragment.size() in an extra varaibale

> WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:89
> +        float length(list->getItem(i, ec));

I would take float length = ... and use length(..) just for objects.

> WebCore/rendering/svg/SVGTextLayoutEngine.cpp:212
> +    if (lengthAdjust == SVGTextContentElement::LENGTHADJUST_SPACING)
> +        m_textPathSpacing = (desiredTextLength - totalLength) / totalCharacters;
> +    else
> +        m_textPathScaling = desiredTextLength / totalLength;

you could use:

m_textPathScaling = desiredTextLength / totalLength;
if (lengthAdjust == SVGTextContentElement::LENGTHADJUST_SPACING)
   m_textPathScaling /= totalCharacters;

> WebCore/rendering/svg/SVGTextLayoutEngine.cpp:376
> +    for (; metricsListOffset < textMetricsSize && positionListOffset < xValues.size(); ++metricsListOffset) {

Save xValues.size() in a variable.

> WebCore/rendering/svg/SVGTextQuery.cpp:354
> +    if (startPosition > 0) {

Isn't enough to just use if (startPosition) ?

> WebCore/rendering/svg/SVGTextQuery.cpp:474
> +    if (startPosition > 0) {

ditto.

> WebCore/rendering/svg/SVGTextQuery.cpp:529
> +    for (unsigned i = 0; i < fragment.length; ++i) {

save fragment.length in a variable

[Nikolas Zimmermann]

Created attachment 69467 [details]
Patch: WebCore - v2

This time with an extensive ChangeLog.
The LayoutTests side of this patch follows soon.

NOTE: Dirk has already reviewed my work during the last weeks, otherwhise I wouldn't have asked him to review this beast.
I already splitted the patch up before (see the depends section), but I couldn't split this thing up any further, without landing #if 0 code or lots of commented stuff with FIXMEs.
Text is really a hairy section, and when changing underlying core concepts (like removing text chunks & text chunk parts), this affects several hundred k of code.

[Nikolas Zimmermann]

Created attachment 69468 [details]
Patch: LayoutTests

[Dirk Schulze]

Comment on attachment 69467 [details]
Patch: WebCore - v2

r=me. Discussed a little change in the changelog on IRC.

[Dirk Schulze]

Comment on attachment 69468 [details]
Patch: LayoutTests

r=me. Great explanations.

[Nikolas Zimmermann]

Committed r68976: <http://trac.webkit.org/changeset/68976>

[Nikolas Zimmermann]

(In reply to comment #10)
> Committed r68976: <http://trac.webkit.org/changeset/68976>

Landed qt/win/gtk rebaselines in r68977.

[WebKit Review Bot]

http://trac.webkit.org/changeset/68976 might have broken Chromium Win Release

[Eric Seidel (no email)]

Rewrite again?

[Eric Seidel (no email)]

I'm sad that this was such a huge patch with little (no?) Input from anyone who knows the text system.

[Nikolas Zimmermann]

(In reply to comment #13)
> Rewrite again?

Again? :-)
You are referrring to the last larger text change, which chained the way SVG text is painted, sharing all of the selection logic etc. with InlineTextBox - this stuff has remained unchanged in the new patch.

This patch adressed the logic that deals with SVGs text layout code (parsing x/y/dx/dy/rotate value lists, storing their results) and the actual text-on-path/line layout.

The way HTML/SVG text is integrated hasn't been changed at all.

[Nikolas Zimmermann]

(In reply to comment #14)
> I'm sad that this was such a huge patch with little (no?) Input from anyone who knows the text system.

Right, there was no input at all, but only because of the fact I didn't change anything related to HTML text. The patch is really about laying out a <text> subtree per-character, based on the SVG x/y/dx/dy/rotate values (baselines, glyph-orientation etc.) and the textPath calculations.

A whole load of the code has just been refactored from existing source files, dating back to 2007.
Does that satisfy you?