New issue
Advanced search Search tips

Issue 607906 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

SVG text layout attributes cleanup/TLC

Project Member Reported by f...@opera.com, Apr 29 2016

Issue description

Some cleanups of text layout attributes related code on the long, winding, road to cleaner (and better) <text> layout code.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7f48ba3851fc5c37187da843a4ffe3b9fe9a4045

commit 7f48ba3851fc5c37187da843a4ffe3b9fe9a4045
Author: fs <fs@opera.com>
Date: Fri Apr 29 15:07:10 2016

Move isEmptyValue and emptyValue to SVGCharacterData

These two helpers have a stronger tie to SVGCharacterData (on which they
operate) than to SVGTextLayoutAttributes - where they are currently
defined.
Move them as described, and also add simple query helpers to make code
using them simpler and more readable.

BUG=607906

Review-Url: https://codereview.chromium.org/1933183002
Cr-Commit-Position: refs/heads/master@{#390649}

[modify] https://crrev.com/7f48ba3851fc5c37187da843a4ffe3b9fe9a4045/third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp
[modify] https://crrev.com/7f48ba3851fc5c37187da843a4ffe3b9fe9a4045/third_party/WebKit/Source/core/layout/svg/SVGTextLayoutAttributes.h
[modify] https://crrev.com/7f48ba3851fc5c37187da843a4ffe3b9fe9a4045/third_party/WebKit/Source/core/layout/svg/SVGTextLayoutAttributesBuilder.cpp
[modify] https://crrev.com/7f48ba3851fc5c37187da843a4ffe3b9fe9a4045/third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp

Project Member

Comment 2 by bugdroid1@chromium.org, Apr 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/731c92d1821ff04b324eb334f484337005d713c9

commit 731c92d1821ff04b324eb334f484337005d713c9
Author: fs <fs@opera.com>
Date: Fri Apr 29 17:45:36 2016

Restructure LayoutSVGText::layout

"Uncascade" LayoutSVGText::layout by separating the handling of the two
flags (m_needsTextMetricsUpdate, m_needsPositioningValuesUpdate) into
sequential blocks. Add assert to verify consistency.

BUG=607906

Review-Url: https://codereview.chromium.org/1933193002
Cr-Commit-Position: refs/heads/master@{#390691}

[modify] https://crrev.com/731c92d1821ff04b324eb334f484337005d713c9/third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp

Project Member

Comment 3 by bugdroid1@chromium.org, Apr 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cdd3f0264799c6af210ce9e7596b803df92d3cea

commit cdd3f0264799c6af210ce9e7596b803df92d3cea
Author: fs <fs@opera.com>
Date: Fri Apr 29 20:17:02 2016

Refactor SVGTextLayoutEngine::currentLogicalCharacterMetrics

The two methods currentLogicalCharacterAttributes and
currentLogicalCharacterMetrics on SVGTextLayoutEngine are very
interdependent, since after calling the former, the latter will be
called.
So fold most of the former into the latter, keeping the bits of the
former that advances to the next layout attribute entry, while
renaming it to nextLogicalAttributes.
The methods are also changed from returning a bool and using out-
variables to return the active SVGTextLayoutAttributes structure
instead.

BUG=607906

Review-Url: https://codereview.chromium.org/1935493002
Cr-Commit-Position: refs/heads/master@{#390736}

[modify] https://crrev.com/cdd3f0264799c6af210ce9e7596b803df92d3cea/third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp
[modify] https://crrev.com/cdd3f0264799c6af210ce9e7596b803df92d3cea/third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.h

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/23b6e12f968b96fa35d612b0779144fb62a1cf86

commit 23b6e12f968b96fa35d612b0779144fb62a1cf86
Author: fs <fs@opera.com>
Date: Fri Apr 29 20:54:45 2016

Simplify SVG layout attribute reordering

findFirstAndLastAttributesInVector is an identity transform, since it
only search for first/lastContext in the layout attributes vector and
return that in the out variable. Remove it. This in turn means that the
vector of layout attributes is unused, and hence also removed. Finally
tidy up the reversing loop by moving more code into the swapping helper
function, and merge the two identical sequences of item swapping code.
Drop the ASSERT that disallows having no (nullptr) user-data for the
closure to collectLeafBoxesInLogicalOrder.

BUG=607906

Review-Url: https://codereview.chromium.org/1931303002
Cr-Commit-Position: refs/heads/master@{#390751}

[modify] https://crrev.com/23b6e12f968b96fa35d612b0779144fb62a1cf86/third_party/WebKit/Source/core/layout/api/LineLayoutSVGInlineText.h
[modify] https://crrev.com/23b6e12f968b96fa35d612b0779144fb62a1cf86/third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp
[modify] https://crrev.com/23b6e12f968b96fa35d612b0779144fb62a1cf86/third_party/WebKit/Source/core/layout/line/InlineFlowBox.h
[modify] https://crrev.com/23b6e12f968b96fa35d612b0779144fb62a1cf86/third_party/WebKit/Source/core/layout/svg/line/SVGRootInlineBox.cpp
[modify] https://crrev.com/23b6e12f968b96fa35d612b0779144fb62a1cf86/third_party/WebKit/Source/core/layout/svg/line/SVGRootInlineBox.h

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/312abc7136a25ea6fc67504d07faa6e3c71dab1c

commit 312abc7136a25ea6fc67504d07faa6e3c71dab1c
Author: fs <fs@opera.com>
Date: Fri Apr 29 20:56:41 2016

Minor tweaks to m_needsReordering in LayoutSVGText::layout

Nit fixes from https://codereview.chromium.org/1933193002/.

BUG=607906

Review-Url: https://codereview.chromium.org/1933413002
Cr-Commit-Position: refs/heads/master@{#390753}

[modify] https://crrev.com/312abc7136a25ea6fc67504d07faa6e3c71dab1c/third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp

Project Member

Comment 6 by bugdroid1@chromium.org, May 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/62d08faa9793d36f12b0844cdbcdbcec6a6526b9

commit 62d08faa9793d36f12b0844cdbcdbcec6a6526b9
Author: fs <fs@opera.com>
Date: Mon May 02 21:22:25 2016

Remove the LayoutSVGInlineText* context in SVGTextLayoutAttributes

Most uses of the SVGTextLayoutAttributes* in vector owned by
LayoutSVGText, actually ends up doing ...->context() and dereferencing
the owning LayoutSVGInlineText rather than the attributes object itself.
It's also slightly more obvious what's going on when considering the
iteration over "text nodes" rather than their associated attributes.
Make LayoutSVGText collect the descendant LayoutSVGInlineTexts rather
than the "parts object" SVGTextLayoutAttributes, and rename as
appropriate to reflect that change.
Since removing the context pointer makes SVGTextLayoutAttributes a simple
wrapper around a SVGCharacterDataMap, just store the latter directly.
Rename SVGTextLayoutAttributes.h to SVGCharacterData.h
Also replace a HashMap::find+conditional copy with a HashMap::get.

BUG=607906

Review-Url: https://codereview.chromium.org/1937043002
Cr-Commit-Position: refs/heads/master@{#391052}

[modify] https://crrev.com/62d08faa9793d36f12b0844cdbcdbcec6a6526b9/third_party/WebKit/Source/core/core.gypi
[modify] https://crrev.com/62d08faa9793d36f12b0844cdbcdbcec6a6526b9/third_party/WebKit/Source/core/layout/api/LineLayoutSVGInlineText.h
[modify] https://crrev.com/62d08faa9793d36f12b0844cdbcdbcec6a6526b9/third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp
[modify] https://crrev.com/62d08faa9793d36f12b0844cdbcdbcec6a6526b9/third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.h
[modify] https://crrev.com/62d08faa9793d36f12b0844cdbcdbcec6a6526b9/third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp
[modify] https://crrev.com/62d08faa9793d36f12b0844cdbcdbcec6a6526b9/third_party/WebKit/Source/core/layout/svg/LayoutSVGText.h
[rename] https://crrev.com/62d08faa9793d36f12b0844cdbcdbcec6a6526b9/third_party/WebKit/Source/core/layout/svg/SVGCharacterData.h
[delete] https://crrev.com/a8e9e2c4ca588971ed8f3b9209a33ad3c563c586/third_party/WebKit/Source/core/layout/svg/SVGTextLayoutAttributes.cpp
[modify] https://crrev.com/62d08faa9793d36f12b0844cdbcdbcec6a6526b9/third_party/WebKit/Source/core/layout/svg/SVGTextLayoutAttributesBuilder.cpp
[modify] https://crrev.com/62d08faa9793d36f12b0844cdbcdbcec6a6526b9/third_party/WebKit/Source/core/layout/svg/SVGTextLayoutAttributesBuilder.h
[modify] https://crrev.com/62d08faa9793d36f12b0844cdbcdbcec6a6526b9/third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp
[modify] https://crrev.com/62d08faa9793d36f12b0844cdbcdbcec6a6526b9/third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.h
[modify] https://crrev.com/62d08faa9793d36f12b0844cdbcdbcec6a6526b9/third_party/WebKit/Source/core/layout/svg/line/SVGRootInlineBox.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, May 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2f349013b11f8906316b41de2867f253a81ad147

commit 2f349013b11f8906316b41de2867f253a81ad147
Author: fs <fs@opera.com>
Date: Tue May 03 17:30:49 2016

Simplify logical iteration in SVGTextLayoutEngine

By replacing m_logicalCharacterOffset == logicalTextNode->textLength()
by the corresponding metrics list equivalent, it becomes obvious that
we're just checking the same thing twice in succession. Remove the first
check and block of code.
Also reverse the test in the loop and refactor to avoid the 'continue'
when skipping whitespace.
The second part of the disjunction (w/ logicalMetrics.isEmpty()) does
not do anything useful, so is removed. This makes the condition match
what SVGTextLayoutAttributesBuilder does (which is the intention.)

BUG=607906

Review-Url: https://codereview.chromium.org/1941303003
Cr-Commit-Position: refs/heads/master@{#391281}

[modify] https://crrev.com/2f349013b11f8906316b41de2867f253a81ad147/third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, May 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7730dd28f370e4f0614b31af83fd600e81f1c18c

commit 7730dd28f370e4f0614b31af83fd600e81f1c18c
Author: fs <fs@opera.com>
Date: Wed May 18 14:20:24 2016

Use SVGInlineTextMetricsIterator in updateLayoutAttributes

This reuses a pre-existing piece to do the "dual offset/variable"
iteration, hiding (and sharing) the complexity.

BUG=607906

Review-Url: https://codereview.chromium.org/1988063002
Cr-Commit-Position: refs/heads/master@{#394412}

[modify] https://crrev.com/7730dd28f370e4f0614b31af83fd600e81f1c18c/third_party/WebKit/Source/core/layout/api/LineLayoutSVGInlineText.h
[modify] https://crrev.com/7730dd28f370e4f0614b31af83fd600e81f1c18c/third_party/WebKit/Source/core/layout/svg/SVGTextLayoutAttributesBuilder.cpp

Comment 9 by tkent@chromium.org, Dec 9 2016

Components: Blink>SVG

Comment 10 by f...@opera.com, Feb 12 2018

Labels: -Type-Bug Type-Task

Sign in to add a comment