WebCore+SVG fails to report errors, fails to load external scripts This patch fixes both (simple) issues.
Created attachment 4845 [details] Fixes reporting errors, loading external scripts
Comment on attachment 4845 [details] Fixes reporting errors, loading external scripts Simple patch. Also does a bit of cleanup to the <script> handling code in xml_tokenizer.cpp
Comment on attachment 4845 [details] Fixes reporting errors, loading external scripts 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.
Created attachment 4868 [details] Addresses Darin's comments (adds XLinkNames)
Created attachment 4869 [details] Addresses Darin's comments (XLinkNames broken out)
Comment on attachment 4869 [details] Addresses Darin's comments (XLinkNames broken out) I addressed Darin's concerns regarding making the xlink qualified name (now depends on 5887) as well as using SharedPtr to make sure that the ElementImpls we create don't disappear out from underneath us. After discussing with mjs and darin, I did not attempt to abstract the ScriptElement logic into a common baseclass for SVG & HTML at this time. We may still chose to do so in the future, but doing so is not critical at this moment. Either Darin our Maciej should be able to review this.
Comment on attachment 4869 [details] Addresses Darin's comments (XLinkNames broken out) The name xhtmlParseErrorHeader makes it sound like "parse" is a verb, so I think it should be improved. The "h3" and "fixed" are both appended immediately after being created, so the SharedPtr is not needed in those cases. Do we have layout tests that test these parse error cases? r=me
(In reply to comment #7) > (From update of attachment 4869 [details] [edit]) > The name xhtmlParseErrorHeader makes it sound like "parse" is a verb, so I > think it should be improved. I changed it to createXHTMLParserErrorHeader. > The "h3" and "fixed" are both appended immediately after being created, so the > SharedPtr is not needed in those cases. I left these as is, I felt that the ref count churn was OK, and made for cleaner (more defensive) coding, if someone else were to change that code, or the append were to fail (and not release the child), etc. > Do we have layout tests that test these parse error cases? I added 7 layout tests: 2 to cover xml parse errors 3 to cover xslt related errors and 2 more to cover svg parse errors.