Bug 25532 - Clean up SVG rendering tree more
Summary: Clean up SVG rendering tree more
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 25568
Blocks: 14019 16939 23112
  Show dependency treegraph
 
Reported: 2009-05-03 17:16 PDT by Eric Seidel (no email)
Modified: 2009-05-05 07:45 PDT (History)
2 users (show)

See Also:


Attachments
Fix RenderForeignObject::paint() (5.08 KB, patch)
2009-05-03 21:01 PDT, Eric Seidel (no email)
staikos: review+
Details | Formatted Diff | Diff
Remove redundant disableLayoutState() calls (3.86 KB, patch)
2009-05-03 21:01 PDT, Eric Seidel (no email)
staikos: review+
Details | Formatted Diff | Diff
Share layout code between RenderSVGViewportContainer and RenderSVGContainer (4.77 KB, patch)
2009-05-03 21:01 PDT, Eric Seidel (no email)
staikos: review+
Details | Formatted Diff | Diff
Remove dead code from RenderPath (1.84 KB, patch)
2009-05-03 21:01 PDT, Eric Seidel (no email)
staikos: review+
Details | Formatted Diff | Diff
Remove broken absoluteTransform() code from RenderSVGInlineText (12.19 KB, patch)
2009-05-03 21:01 PDT, Eric Seidel (no email)
staikos: review+
Details | Formatted Diff | Diff
Move RenderSVGText off of localToAbsolute() (3.50 KB, patch)
2009-05-03 21:01 PDT, Eric Seidel (no email)
staikos: review+
Details | Formatted Diff | Diff
Move absoluteRects and absoluteQuads into RenderSVGInline and remove absoluteTransform() usage (8.13 KB, patch)
2009-05-03 21:01 PDT, Eric Seidel (no email)
staikos: review+
Details | Formatted Diff | Diff
Add SVGRenderBase to share logic between SVG renderers (16.31 KB, patch)
2009-05-03 21:01 PDT, Eric Seidel (no email)
simon.fraser: review+
Details | Formatted Diff | Diff
Move more code into SVGRenderBase (13.29 KB, patch)
2009-05-03 21:01 PDT, Eric Seidel (no email)
simon.fraser: review+
Details | Formatted Diff | Diff
Remove the vestigial calculateLocalTransform() (12.53 KB, patch)
2009-05-03 21:01 PDT, Eric Seidel (no email)
simon.fraser: review+
Details | Formatted Diff | Diff
Remove m_absoluteBounds hack from RenderSVGText (2.04 KB, patch)
2009-05-03 21:02 PDT, Eric Seidel (no email)
simon.fraser: review+
Details | Formatted Diff | Diff
ASSERT(useTransforms) in SVG mapLocalToContainer implementations (3.15 KB, patch)
2009-05-03 21:02 PDT, Eric Seidel (no email)
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-05-03 17:16:45 PDT
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.
Comment 1 Eric Seidel (no email) 2009-05-03 21:01:16 PDT
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(-)
Comment 2 Eric Seidel (no email) 2009-05-03 21:01:21 PDT
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(-)
Comment 3 Eric Seidel (no email) 2009-05-03 21:01:25 PDT
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(-)
Comment 4 Eric Seidel (no email) 2009-05-03 21:01:29 PDT
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(-)
Comment 5 Eric Seidel (no email) 2009-05-03 21:01:34 PDT
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(-)
Comment 6 Eric Seidel (no email) 2009-05-03 21:01:39 PDT
Created attachment 29977 [details]
Move RenderSVGText off of localToAbsolute()

 WebCore/ChangeLog                   |   11 +++++++++++
 WebCore/rendering/RenderSVGText.cpp |   20 ++++++--------------
 2 files changed, 17 insertions(+), 14 deletions(-)
Comment 7 Eric Seidel (no email) 2009-05-03 21:01:43 PDT
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(-)
Comment 8 Eric Seidel (no email) 2009-05-03 21:01:49 PDT
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(-)
Comment 9 Eric Seidel (no email) 2009-05-03 21:01:53 PDT
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(-)
Comment 10 Eric Seidel (no email) 2009-05-03 21:01:57 PDT
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(-)
Comment 11 Eric Seidel (no email) 2009-05-03 21:02:03 PDT
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(-)
Comment 12 Eric Seidel (no email) 2009-05-03 21:02:08 PDT
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(-)
Comment 13 Eric Seidel (no email) 2009-05-03 21:03:21 PDT
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.
Comment 14 Simon Fraser (smfr) 2009-05-04 17:32:37 PDT
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.
Comment 15 Simon Fraser (smfr) 2009-05-04 17:37:23 PDT
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?
Comment 16 Simon Fraser (smfr) 2009-05-04 17:41:36 PDT
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?
Comment 17 Eric Seidel (no email) 2009-05-04 18:04:30 PDT
(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.
Comment 18 Eric Seidel (no email) 2009-05-05 01:11:39 PDT
(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.

Comment 19 Eric Seidel (no email) 2009-05-05 01:14:56 PDT
Yay, these are all landed now.
Comment 20 Eric Seidel (no email) 2009-05-05 01:16:35 PDT
A test case for some of this is up for review at:
https://bugs.webkit.org/show_bug.cgi?id=23112
Comment 21 Eric Seidel (no email) 2009-05-05 02:11:00 PDT
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
Comment 22 Eric Seidel (no email) 2009-05-05 02:11:20 PDT
Re-opening since I rolled out the last change.
Comment 23 Eric Seidel (no email) 2009-05-05 07:45:37 PDT
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. :)