Bug 5842 - Fix dynamic style updates for SVG, also first pass at CDF support
Summary: Fix dynamic style updates for SVG, also first pass at CDF support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P4 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-11-27 03:26 PST by Eric Seidel (no email)
Modified: 2005-11-29 16:01 PST (History)
0 users

See Also:


Attachments
Patch fixes dynamic css updates, adds first-pass CDF support (23.50 KB, patch)
2005-11-27 04:02 PST, Eric Seidel (no email)
darin: review-
Details | Formatted Diff | Diff
Other updated test results for absolute bbox update. (now respects viewport) (173.19 KB, patch)
2005-11-27 04:03 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch addressing darin's comments. (32.28 KB, patch)
2005-11-27 23:07 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch addressing darin's comments. (32.28 KB, patch)
2005-11-27 23:14 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch addressing darin's comments. (32.28 KB, patch)
2005-11-27 23:15 PST, Eric Seidel (no email)
mjs: 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) 2005-11-27 03:26:26 PST
Fix dynamic style updates for SVG, also first pass at CDF support

Patch attached.
Comment 1 Eric Seidel (no email) 2005-11-27 04:02:05 PST
Created attachment 4816 [details]
Patch fixes dynamic css updates, adds first-pass CDF support
Comment 2 Eric Seidel (no email) 2005-11-27 04:03:00 PST
Created attachment 4817 [details]
Other updated test results for absolute bbox update. (now respects viewport)
Comment 3 Eric Seidel (no email) 2005-11-27 04:04:57 PST
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.)
Comment 4 Eric Seidel (no email) 2005-11-27 04:07:44 PST
This also makes:
http://www.croczilla.com/svg/samples/circles1/circles1.svg
work...
Comment 5 Eric Seidel (no email) 2005-11-27 04:08:53 PST
Also makes:
http://www.croczilla.com/svg/samples/circles2/circles2.xml
work for some reason...  Even though we don't yet support <foreignObject>!
Comment 6 Darin Adler 2005-11-27 19:31:19 PST
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-.
Comment 7 Eric Seidel (no email) 2005-11-27 23:07:16 PST
Created attachment 4829 [details]
Patch addressing darin's comments.
Comment 8 Eric Seidel (no email) 2005-11-27 23:07:52 PST
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.
Comment 9 Eric Seidel (no email) 2005-11-27 23:14:30 PST
Created attachment 4830 [details]
Patch addressing darin's comments.
Comment 10 Eric Seidel (no email) 2005-11-27 23:15:47 PST
Created attachment 4831 [details]
Patch addressing darin's comments.
Comment 11 Maciej Stachowiak 2005-11-29 02:14:06 PST
Comment on attachment 4831 [details]
Patch addressing darin's comments.

r=me

quibbles seem sufficiently addressed