Bug 25532 – Clean up SVG rendering tree more

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

[Eric Seidel (no email)]

Clean up SVG rendering tree more

I wrote 16 patches on my flight from LAX->SYD cleaning up the SVG rendering tree more.  They're all small (some of them tiny) and don't really need their own bugs, so I'm going to attach the ones which don't already have other bugs here.

[Eric Seidel (no email)]

Created attachment 29972 [details]
Fix RenderForeignObject::paint()

 .../text/foreignObject-repaint-expected.checksum   |    1 +
 .../svg/text/foreignObject-repaint-expected.png    |  Bin 0 -> 32329 bytes
 .../svg/text/foreignObject-repaint-expected.txt    |   10 +++++++
 LayoutTests/svg/text/foreignObject-repaint.xml     |   15 +++++++++++
 WebCore/ChangeLog                                  |   11 ++++++++
 WebCore/rendering/RenderForeignObject.cpp          |   26 ++++++++-----------
 6 files changed, 48 insertions(+), 15 deletions(-)

[Eric Seidel (no email)]

Created attachment 29973 [details]
Remove redundant disableLayoutState() calls

 WebCore/ChangeLog                                |   14 ++++++++++++++
 WebCore/rendering/RenderForeignObject.cpp        |    9 ++-------
 WebCore/rendering/RenderSVGContainer.cpp         |    7 +------
 WebCore/rendering/RenderSVGViewportContainer.cpp |   12 ++++--------
 4 files changed, 21 insertions(+), 21 deletions(-)

[Eric Seidel (no email)]

Created attachment 29974 [details]
Share layout code between RenderSVGViewportContainer and RenderSVGContainer

 WebCore/ChangeLog                                |   15 ++++++++++++++
 WebCore/rendering/RenderSVGContainer.cpp         |    4 ++-
 WebCore/rendering/RenderSVGContainer.h           |    3 +-
 WebCore/rendering/RenderSVGViewportContainer.cpp |   23 ----------------------
 WebCore/rendering/RenderSVGViewportContainer.h   |    3 +-
 5 files changed, 21 insertions(+), 27 deletions(-)

[Eric Seidel (no email)]

Created attachment 29975 [details]
Remove dead code from RenderPath

 WebCore/ChangeLog                |   10 ++++++++++
 WebCore/rendering/RenderPath.cpp |   10 ----------
 WebCore/rendering/RenderPath.h   |    2 --
 3 files changed, 10 insertions(+), 12 deletions(-)

[Eric Seidel (no email)]

Created attachment 29976 [details]
Remove broken absoluteTransform() code from RenderSVGInlineText

 .../mac/svg/custom/focus-ring-expected.checksum    |    2 +-
 .../mac/svg/custom/focus-ring-expected.png         |  Bin 40658 -> 43386 bytes
 .../svg/custom/svgsvgelement-ctm-expected.txt      |    8 ++--
 WebCore/ChangeLog                                  |   33 ++++++++++++++++++++
 WebCore/rendering/RenderSVGInlineText.cpp          |   30 ++++++++----------
 WebCore/rendering/RenderSVGInlineText.h            |    1 +
 WebCore/rendering/RenderSVGRoot.cpp                |   14 ++++++++
 WebCore/rendering/RenderSVGRoot.h                  |    2 +
 WebCore/rendering/RenderSVGText.cpp                |   14 ++++++++
 WebCore/rendering/RenderSVGText.h                  |    2 +
 WebCore/svg/SVGSVGElement.cpp                      |    4 +-
 11 files changed, 86 insertions(+), 24 deletions(-)

[Eric Seidel (no email)]

Created attachment 29977 [details]
Move RenderSVGText off of localToAbsolute()

 WebCore/ChangeLog                   |   11 +++++++++++
 WebCore/rendering/RenderSVGText.cpp |   20 ++++++--------------
 2 files changed, 17 insertions(+), 14 deletions(-)

[Eric Seidel (no email)]

Created attachment 29978 [details]
Move absoluteRects and absoluteQuads into RenderSVGInline and remove absoluteTransform() usage

 WebCore/ChangeLog                       |   16 +++++++++++
 WebCore/rendering/RenderSVGInline.cpp   |   43 ++++++++++++++++++++++++++++-
 WebCore/rendering/RenderSVGInline.h     |    4 +++
 WebCore/rendering/RenderSVGTSpan.cpp    |   45 -------------------------------
 WebCore/rendering/RenderSVGTSpan.h      |    2 -
 WebCore/rendering/RenderSVGTextPath.cpp |   40 ---------------------------
 WebCore/rendering/RenderSVGTextPath.h   |    2 -
 7 files changed, 62 insertions(+), 90 deletions(-)

[Eric Seidel (no email)]

Created attachment 29979 [details]
Add SVGRenderBase to share logic between SVG renderers

 WebCore/ChangeLog                               |   39 ++++++++++++++++++
 WebCore/rendering/RenderSVGBlock.h              |    3 +-
 WebCore/rendering/RenderSVGImage.h              |    3 +-
 WebCore/rendering/RenderSVGModelObject.h        |    7 ++-
 WebCore/rendering/RenderSVGRoot.h               |    3 +-
 WebCore/rendering/SVGRenderSupport.cpp          |   20 ++++-----
 WebCore/rendering/SVGRenderSupport.h            |   50 ++++++++++++++--------
 WebCore/rendering/SVGRootInlineBox.cpp          |    9 ++--
 WebCore/svg/SVGMaskElement.cpp                  |    2 +-
 WebCore/svg/SVGPatternElement.cpp               |    4 +-
 WebCore/svg/graphics/SVGPaintServerGradient.cpp |    5 +-
 11 files changed, 100 insertions(+), 45 deletions(-)

[Eric Seidel (no email)]

Created attachment 29980 [details]
Move more code into SVGRenderBase

 WebCore/ChangeLog                          |   35 +++++++++++++++++++++++++++
 WebCore/rendering/RenderSVGImage.cpp       |   19 +++++---------
 WebCore/rendering/RenderSVGImage.h         |    4 ++-
 WebCore/rendering/RenderSVGModelObject.cpp |   27 ++++-----------------
 WebCore/rendering/RenderSVGText.cpp        |   25 ++-----------------
 WebCore/rendering/SVGRenderSupport.cpp     |   36 +++++++++++++++++++++++++++-
 WebCore/rendering/SVGRenderSupport.h       |    5 ++++
 7 files changed, 93 insertions(+), 58 deletions(-)

[Eric Seidel (no email)]

Created attachment 29981 [details]
Remove the vestigial calculateLocalTransform()

 .../mac/svg/custom/focus-ring-expected.checksum    |    2 +-
 .../mac/svg/custom/focus-ring-expected.png         |  Bin 43386 -> 50898 bytes
 WebCore/ChangeLog                                  |   33 ++++++++++++++++++++
 WebCore/rendering/RenderForeignObject.cpp          |    9 +-----
 WebCore/rendering/RenderForeignObject.h            |    4 +-
 WebCore/rendering/RenderPath.cpp                   |   14 ++------
 WebCore/rendering/RenderPath.h                     |    1 -
 WebCore/rendering/RenderSVGContainer.cpp           |    6 ---
 WebCore/rendering/RenderSVGContainer.h             |    3 +-
 WebCore/rendering/RenderSVGImage.cpp               |   20 +++++-------
 WebCore/rendering/RenderSVGImage.h                 |    1 -
 WebCore/rendering/RenderSVGText.cpp                |   13 ++------
 WebCore/rendering/RenderSVGText.h                  |    2 -
 .../rendering/RenderSVGTransformableContainer.cpp  |    4 +--
 .../rendering/RenderSVGTransformableContainer.h    |    4 +-
 15 files changed, 56 insertions(+), 60 deletions(-)

[Eric Seidel (no email)]

Created attachment 29982 [details]
Remove m_absoluteBounds hack from RenderSVGText

 WebCore/ChangeLog                   |   13 +++++++++++++
 WebCore/rendering/RenderSVGText.cpp |    6 +-----
 WebCore/rendering/RenderSVGText.h   |    1 -
 3 files changed, 14 insertions(+), 6 deletions(-)

[Eric Seidel (no email)]

Created attachment 29983 [details]
ASSERT(useTransforms) in SVG mapLocalToContainer implementations

 WebCore/ChangeLog                      |   12 ++++++++++++
 WebCore/rendering/RenderSVGRoot.cpp    |    9 +++------
 WebCore/rendering/SVGRenderSupport.cpp |    9 ++-------
 3 files changed, 17 insertions(+), 13 deletions(-)

[Eric Seidel (no email)]

Oops.  I need to fix my get-send-bugzilla script to be shorter. :(

Also, I know this is a lot of patches for one bug.  I expect that pretty much all of these will be simple r+'s.  If any of them need discussion I'm happy to break them out into other bugs.

[Simon Fraser (smfr)]

Comment on attachment 29979 [details]
Add SVGRenderBase to share logic between SVG renderers

>  // Most renderers in the SVG rendering tree will inherit from this class
>  // but not all. (e.g. RenderSVGForeignObject, RenderSVGBlock, RenderSVGImage) thus methods
> -// required by SVG renders need to be declared on RenderObject, but some shared
> -// logic can go in this class.
> +// required by SVG renders need to be declared on RenderObject, but shared
> +// logic can go in this class or in SVGRenderBase.

You should give some direction about why one or the other is appropriate.

[Simon Fraser (smfr)]

Comment on attachment 29980 [details]
Move more code into SVGRenderBase

> +IntRect SVGRenderBase::clippedOverflowRectForRepaint(RenderObject* object, RenderBoxModelObject* repaintContainer)
> +{

Maybe assert that repaintContainer != this to make sure that someone isn't trying to do a container-relative
repaint inside of SVG content?

> +    // Return early for any cases where we don't actually paint
> +    if (object->style()->visibility() != VISIBLE && !object->enclosingLayer()->hasVisibleContent())
> +        return IntRect();
> +
> +    // Pass our local paint rect to computeRectForRepaint() which will
> +    // map to parent coords and recurse up the parent chain.
> +    IntRect repaintRect = enclosingIntRect(object->repaintRectInLocalCoordinates());
> +    object->computeRectForRepaint(repaintContainer, repaintRect);
> +    return repaintRect;


> +void SVGRenderBase::computeRectForRepaint(RenderObject* object, RenderBoxModelObject* repaintContainer, IntRect& repaintRect, bool fixed)
> +{

Assert that this != repaintContainer ?

> +    // Translate to coords in our parent renderer, and then call computeRectForRepaint on our parent
> +    repaintRect = object->localToParentTransform().mapRect(repaintRect);
> +    object->parent()->computeRectForRepaint(repaintContainer, repaintRect, fixed);

Is object->parent() always non-null?

> +void SVGRenderBase::mapLocalToContainer(const RenderObject* object, RenderBoxModelObject* repaintContainer, bool fixed , bool useTransforms, TransformState& transformState)
> +{
> +    ASSERT(!fixed); // We should have no fixed content in the SVG rendering tree.
> +
> +    // FIXME: If we don't respect useTransforms we break SVG text rendering.
> +    // Seems RenderSVGInlineText has some own broken translation hacks which depend useTransforms=false
> +    // This should instead be ASSERT(useTransforms) once we fix RenderSVGInlineText
> +    if (useTransforms)
> +        transformState.applyTransform(object->localToParentTransform());
> +
> +    object->parent()->mapLocalToContainer(repaintContainer, fixed, useTransforms, transformState);

Is object->parent() always non-null?

[Simon Fraser (smfr)]

Comment on attachment 29982 [details]
Remove m_absoluteBounds hack from RenderSVGText

> +        No functional changes (SVGs inside CSS transformed HTML should theoretically repaint better)

Can we have a testcase pls?

[Eric Seidel (no email)]

(In reply to comment #16)
> (From update of attachment 29982 [details] [review])
> > +        No functional changes (SVGs inside CSS transformed HTML should theoretically repaint better)
> 
> Can we have a testcase pls?

It's possible that the test cases from bug 23112 or bug 23113 may already be fixed by these changes.

[Eric Seidel (no email)]

(In reply to comment #15)
> (From update of attachment 29980 [details] [review])

object()->parent() is alwasy non-null, yes.  Unless you're the root of the render tree... which is only RenderCanvas iirc.

And few SVG renderers are RenderBoxModelObject's so checking this != repaintContainer seems overkill to me.

[Eric Seidel (no email)]

Yay, these are all landed now.

[Eric Seidel (no email)]

A test case for some of this is up for review at:
https://bugs.webkit.org/show_bug.cgi?id=23112

[Eric Seidel (no email)]

Comment on attachment 29983 [details]
ASSERT(useTransforms) in SVG mapLocalToContainer implementations

Had to roll this one out, I must not have run the tests after this change... I thought I did.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderSVGRoot.cpp
	M	WebCore/rendering/SVGRenderSupport.cpp
Committed r43218

[Eric Seidel (no email)]

Re-opening since I rolled out the last change.

[Eric Seidel (no email)]

I'll land this last patch as part of:
https://bugs.webkit.org/show_bug.cgi?id=25568

We can go ahead and close this bug again. :)