Bug 45532 - Rewrite SVG text layout code
Summary: Rewrite SVG text layout code
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
Depends on: 46887 46888 46889 46964 46969 46971 46972
  Show dependency treegraph
Reported: 2010-09-10 05:01 PDT by Nikolas Zimmermann
Modified: 2010-10-04 22:46 PDT (History)
8 users (show)

See Also:

Patch (205.61 KB, patch)
2010-10-01 05:02 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch: WebCore (296.15 KB, patch)
2010-10-01 06:36 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch: WebCore - v2 (298.15 KB, patch)
2010-10-01 07:50 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff
Patch: LayoutTests (deleted)
2010-10-01 08:09 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-09-10 05:01:07 PDT
This is the master bug tracking the rewrite of the SVG text layout code.
Comment 1 Nikolas Zimmermann 2010-10-01 05:02:49 PDT
Created attachment 69452 [details]

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.
Comment 2 Nikolas Zimmermann 2010-10-01 06:36:43 PDT
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.
Comment 3 Nikolas Zimmermann 2010-10-01 06:38:46 PDT
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!
Comment 4 WebKit Review Bot 2010-10-01 06:41:07 PDT
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.
Comment 5 Dirk Schulze 2010-10-01 07:06:02 PDT
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) {


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

save fragment.length in a variable
Comment 6 Nikolas Zimmermann 2010-10-01 07:50:36 PDT
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.
Comment 7 Nikolas Zimmermann 2010-10-01 08:09:27 PDT
Created attachment 69468 [details]
Patch: LayoutTests
Comment 8 Dirk Schulze 2010-10-01 08:18:32 PDT
Comment on attachment 69467 [details]
Patch: WebCore - v2

r=me. Discussed a little change in the changelog on IRC.
Comment 9 Dirk Schulze 2010-10-01 08:19:10 PDT
Comment on attachment 69468 [details]
Patch: LayoutTests

r=me. Great explanations.
Comment 10 Nikolas Zimmermann 2010-10-02 01:09:28 PDT
Committed r68976: <http://trac.webkit.org/changeset/68976>
Comment 11 Nikolas Zimmermann 2010-10-02 02:28:36 PDT
(In reply to comment #10)
> Committed r68976: <http://trac.webkit.org/changeset/68976>

Landed qt/win/gtk rebaselines in r68977.
Comment 12 WebKit Review Bot 2010-10-04 04:44:34 PDT
http://trac.webkit.org/changeset/68976 might have broken Chromium Win Release
Comment 13 Eric Seidel (no email) 2010-10-04 08:25:27 PDT
Rewrite again?
Comment 14 Eric Seidel (no email) 2010-10-04 08:27:16 PDT
I'm sad that this was such a huge patch with little (no?) Input from anyone who knows the text system.
Comment 15 Nikolas Zimmermann 2010-10-04 22:45:04 PDT
(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.
Comment 16 Nikolas Zimmermann 2010-10-04 22:46:59 PDT
(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?