Bug 5842 – Fix dynamic style updates for SVG, also first pass at CDF support

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

[Eric Seidel (no email)]

Fix dynamic style updates for SVG, also first pass at CDF support

Patch attached.

[Eric Seidel (no email)]

Created attachment 4816 [details]
Patch fixes dynamic css updates, adds first-pass CDF support

[Eric Seidel (no email)]

Created attachment 4817 [details]
Other updated test results for absolute bbox update. (now respects viewport)

[Eric Seidel (no email)]

Comment on attachment 4817 [details]
Other updated test results for absolute bbox update. (now respects viewport)

I've added these for completeness.  They don't really need a review. 
Previously absoluteTransform did not respect viewport transformations as part
of containers, yet viewport transformations were used when drawing.  This fix
not only "fixed" the DumpRenderTree results, but also fixed nodeForPoint (and
thus hit testing, etc.)

[Eric Seidel (no email)]

This also makes:
http://www.croczilla.com/svg/samples/circles1/circles1.svg
work...

[Eric Seidel (no email)]

Also makes:
http://www.croczilla.com/svg/samples/circles2/circles2.xml
work for some reason...  Even though we don't yet support <foreignObject>!

[Darin Adler]

Comment on attachment 4816 [details]
Patch fixes dynamic css updates, adds first-pass CDF support

I think the RenderStyle:diff comment should be more explicit about what's
"awful" about the hack and what the correct solution is. A simple disclaimer
isn't really clear enough.

Similarly, I'd like the comments about SVGStyledElementImpl::attributeChanged
to be more clear about what is wrong about this and what a better solution will
be. I don't think the header file needs a comment at all, and I don't think
that you need to say "temporary" -- instead you should say what's wrong without
necessarily making a promise about when you'll fix it.

Why have inheritedNotEqual and equals functions with opposite sense? This seems
like a recipe for future confusion. Maybe equals could be changed into an
operator== to match the one in RenderStyle?

Typo: two spaces after the "&&" in render_style.cpp.

Seems to me that the check in containingBlock should be changed to be a virtual
function. Since it's already calling multiple virtual functions, making a new
one for this purpose will speed things up. And it will also give us better
factoring -- we won't need an SVG_SUPPORT ifdef in here. The only trick is
knowing what to name it.

I'm not fond of the "..." in comments -- it seems to add an unnecessary note of
incompleteness and uncertainty. Please leave out the "I don't think" too unless
you have something concrete.

Generally, ASSERT should not use &&. Instead just assert both things
separately. This helps when the assert fires because you don't have to guess
which one failed. In these cases it doesn't seem important, but it's a nice
general style guidelines to follow.

The comment in KCanvasContainer on the call to setReplaced seems unnecessary.
It's true that the canvas container needs to be marked as a replaced element,
but we don't need a comment to record that at one point this was not obvious.
Eventually, however, we should figure out whether KCanvasContainer should
inherit from RenderReplaced instead of RenderContainer -- I suspect that change
will be a good one in the long run.

Why not put a newline at the end of simpleCDF.xml?

This comment needs improvement:

+	 // Transform from our parent (until the rest of the render tree is
updated to use transforms)

I believe what you're saying is "Turn the parent offset that HTML rendering
uses into a transform. If all of HTML used abitrary transforms we wouldn't need
this code." But I'm not sure that the latter "If all HTML used arbitrary
transforms" is a worthwhile thing to mention in the comment. Of course if it
all used transform matrices then we'd remove those parameters -- we wouldn't
need a comment to point out this code was then unnecessary.

Why are you adding a forward declaration of KDOM::QualifiedName to
SVGStyledElementImpl.h? I don't see any use of KDOM::QualifiedName in the
patch.

These are all pretty minor quibbles, but I guess they add up to review-.

[Eric Seidel (no email)]

Created attachment 4829 [details]
Patch addressing darin's comments.

[Eric Seidel (no email)]

Comment on attachment 4829 [details]
Patch addressing darin's comments.

Addressed Darin's comments.  I chose not to create a virtual function for the
containingBlock call for now.

[Eric Seidel (no email)]

Created attachment 4830 [details]
Patch addressing darin's comments.

[Eric Seidel (no email)]

Created attachment 4831 [details]
Patch addressing darin's comments.

[Maciej Stachowiak]

Comment on attachment 4831 [details]
Patch addressing darin's comments.

r=me

quibbles seem sufficiently addressed