[webkit-reviews] review denied: [Bug 5842] Fix dynamic style updates for SVG, also first pass at CDF support : [Attachment 4816] Patch fixes dynamic css updates, adds first-pass CDF support

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Eric Seidel <macdome at opendarwin.org>'s
request for review:
Bug 5842: Fix dynamic style updates for SVG, also first pass at CDF support
http://bugzilla.opendarwin.org/show_bug.cgi?id=5842

Attachment 4816: Patch fixes dynamic css updates, adds first-pass CDF support
http://bugzilla.opendarwin.org/attachment.cgi?id=4816&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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-.