[webkit-reviews] review denied: [Bug 5875] WebCore+SVG fails to report errors, fails to load external scripts : [Attachment 4845] Fixes reporting errors, loading external scripts

[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 5875: WebCore+SVG fails to report errors, fails to load external scripts
http://bugzilla.opendarwin.org/show_bug.cgi?id=5875

Attachment 4845: Fixes reporting errors, loading external scripts
http://bugzilla.opendarwin.org/attachment.cgi?id=4845&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
There's no need to call doc->removeChild(root). Putting a child in a new parent
with appendChild automatically removes it from its old location.

This line:

    body->appendChild(root,exceptioncode)

is missing a space after a comma.

The new code and some of the existing code in this function is just working by
"luck" because it doesn't ref the newly created nodes even though in some cases
it makes calls to set them up before calling appendChild to put them into the
tree. Those calls are allowed to ref/deref the node, which would then be
deleted. One way to fix this is to do always do the appendChild first. Another
alternative is to use SharedPtr for some of the local variables. A third is to
add explicit ref/deref calls.

The name "newRoot" is a confusing name for the local variable to hold the newly
created HTML element, since "root" is used as the name for a body element and
indeed "root" gets set to "body" not "newRoot".

The code in XMLTokenizer::executeScripts seems like it belongs in the script
element classes themselves somehow rather than here in the tokenizer. If there
is some common base class for HTML and SVG script elements then they could have
a virtual function that returns the script's source URL. It seems that there's
logic that would need to be shared between HTML and SVG script elements to
support dynamic loading of scripts. It also seems not so good have the xlink
and href attribute names as string literals here, meaning atomic strings will
be created each time this code is run.