Bug 12207 - RenderSVGContainer should be split into multiple classes
Summary: RenderSVGContainer should be split into multiple classes
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 5940 9752 10526 11976 12221 13981 14051
  Show dependency treegraph
Reported: 2007-01-11 00:56 PST by Eric Seidel (no email)
Modified: 2007-06-13 16:26 PDT (History)
2 users (show)

See Also:

First attempt (16.57 KB, patch)
2007-01-12 12:45 PST, Rob Buis
mjs: review-
Details | Formatted Diff | Diff
Work in progress (31.63 KB, patch)
2007-06-12 15:57 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Split out RenderSVGRoot (38.88 KB, patch)
2007-06-12 17:08 PDT, Rob Buis
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) 2007-01-11 00:56:15 PST
RenderSVGContainer should be split into two classes, one for SVG and one for G

The current implementation is confusing at times and has lead to bugs like
Comment 1 Rob Buis 2007-01-12 12:45:39 PST
Created attachment 12397 [details]
First  attempt

Thi is not the final patch, and lots of test results will change (I guess all SVG results). I hope I get some feedback on the code and names I picked.

Comment 2 Eric Seidel (no email) 2007-01-14 22:00:42 PST
We may need more than 2.

Here are all the SVG containers I can think of, grouped by behavior:

Have a viewBox and establish a viewPort:
<svg> - new SVGRenderRoot
<image> - RenderSVGImage
<foreignObject> - RenderSVGForeignObject

No viewbox, sometimes establishes a viewport:
<symbol> - only when referenced by <use>, the logic should probably be in the <use> renderer(s)

Has viewbox, but does not establish a viewport:
Neither of these draw their content directly.

Has no viewport:
<defs> - does not draw its content directly, but probably should have a renderer to allow proper style resolution of its kids (right now it has no renderer)
<g> - should probably be RenderSVGContainer

<view> also has a viewBox, but it's never drawn, so we don't need a renderer for it.

I propose adding 3 new container types in this bug:

RenderSVGRoot - for <svg>, is a RenderBlock, only trickiness occurs when inside another <svg>
RenderSVGGroup - for <g>, only supports localTransform
RenderSVGUseable - for <symbol>, <pattern>, <marker>, viewbox is simply ignored for <symbol>, does not draw content via normal paint() method.

Optionally, you could also make <defs> a RenderSVGUsable, although then maybe it should be called RenderSVGHidden, not sure.
Comment 3 Eric Seidel (no email) 2007-01-14 22:15:48 PST
I forgot <mask> as well, that should also be part of the <symbol>, <pattern>, <marker> group, IMO.
Comment 4 Eric Seidel (no email) 2007-01-14 23:10:28 PST
Now that bug 12210, I should note that only the new RenderSVGRoot class will need to have the changes introduced in 12210.
Comment 5 Maciej Stachowiak 2007-01-15 01:52:09 PST
Comment on attachment 12397 [details]
First  attempt

r- for now based on Eric's comments
Comment 6 Rob Buis 2007-06-05 00:43:36 PDT
(In reply to comment #4)
> Now that bug 12210, I should note that only the new RenderSVGRoot class will
> need to have the changes introduced in 12210.

I'll give this another go, on the feature_branch. If anybody was working on it already, let me know, we should coordinate it.

Comment 7 Eric Seidel (no email) 2007-06-06 03:10:40 PDT
Anyone working on this bug (i.e. rob) should be aware of the patch attached to:

That patch includes some small changes which try to make it obvious what sections of RenderSVGContainer already have special-case code for the <svg> root element.  The goal of splitting out a RenderSVGRoot class would be to remove those special cases from RenderSVGGroup (or RenderSVGContainer) so that RenderSVGContainer can only worry about being inside other SVG content, and RenderSVGRoot can worry about all the issues of mixing with other non-SVG content.
Comment 8 Rob Buis 2007-06-12 15:56:00 PDT
(In reply to comment #7)
> Anyone working on this bug (i.e. rob) should be aware of the patch attached to:
> http://bugs.webkit.org/show_bug.cgi?id=14012

I am assuming your remark is about trunk. The work will go to feature_branch and there the patch seems to be applied, so no problem.

Comment 9 Rob Buis 2007-06-12 15:57:48 PDT
Created attachment 14987 [details]
Work in progress

This is just work in progress, a first split to RenderSVGRoot, no regressions.

Comment 10 Rob Buis 2007-06-12 17:08:16 PDT
Created attachment 14988 [details]
Split out RenderSVGRoot

A good first start.

Comment 11 Eric Seidel (no email) 2007-06-13 16:09:08 PDT
Comment on attachment 14988 [details]
Split out RenderSVGRoot

Eventually we'll need a better solution to this:

+        // FIXME: All this setup should be done after attributesChanged, not here.
+        rootContainer->setViewBox(viewBox());
+        rootContainer->setAlign(SVGPreserveAspectRatio::SVGPreserveAspectRatioType(preserveAspectRatio()->align()));
+        rootContainer->setSlice(preserveAspectRatio()->meetOrSlice() == SVGPreserveAspectRatio::SVG_MEETORSLICE_SLICE);

We'll need to find a way to abstract a bunch of this code so it's not so copy/paste.

It looks fine for a start.  We'll need follow up patches to clean up these newly split classes.
Comment 12 Rob Buis 2007-06-13 16:26:17 PDT
Landed in r23509.