DOM4 remove method
Created attachment 117978 [details] Patch
Comment on attachment 117978 [details] Patch Do we need to use a prefix here? Given Henri's recent post about prefixes, http://hsivonen.iki.fi/vendor-prefixes/, we probably want to avoid them whenever possible.
@Anne: Would you prefer DOM4 to be implemented with vendor prefixes or without?
(In reply to comment #3) > @Anne: Would you prefer DOM4 to be implemented with vendor prefixes or without? This bug is probably not the best place to discuss this. There's a variety of strong opinions WRT Henri's post. We should start a thread on webkit-dev.
> This bug is probably not the best place to discuss this. There's a variety of strong opinions WRT Henri's post. We should start a thread on webkit-dev. Yes, or in the WhatWG. Ideally we'd involve Henri and Anne in the discussion so everyone is on the same page.
No prefixes works for me. Everyone agrees on this method as far as I can tell. Maybe message www-dom@w3.org to that effect so people can complain?
Created attachment 119631 [details] Patch
Comment on attachment 119631 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119631&action=review Patch looks pretty good, but there is a problem with exception code handling. > Source/WebCore/dom/Node.h:178 > - void remove(ExceptionCode&); > + void remove(ExceptionCode& = ASSERT_NO_EXCEPTION); This change doesn’t makes sense. If the DOM does not want an exception from this function, then there is no reason for any caller to want an exception. We have these exceptions only for the benefit of the DOM. If there are any exceptions that could happen when called from the DOM, an assert is inappropriate. Nothing that a webpage does should be able to cause an assertion to fire.
(In reply to comment #8) > (From update of attachment 119631 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119631&action=review > > Patch looks pretty good, but there is a problem with exception code handling. > > > Source/WebCore/dom/Node.h:178 > > - void remove(ExceptionCode&); > > + void remove(ExceptionCode& = ASSERT_NO_EXCEPTION); > > This change doesn’t makes sense. If the DOM does not want an exception from this function, then there is no reason for any caller to want an exception. We have these exceptions only for the benefit of the DOM. > > If there are any exceptions that could happen when called from the DOM, an assert is inappropriate. Nothing that a webpage does should be able to cause an assertion to fire. removeChild calls Node::remove and removeChild may throw an exception.
Correction not removeChild but others. I'll go through all callers and let them handle the exception in the few cases where it is needed.
Erik, why did you change component? This is core DOM, not HTML.
(In reply to comment #11) > Erik, why did you change component? This is core DOM, not HTML. Alexey, just like you said; This is Core DOM, not XML :-)
But there is no other XML DOM besides Core DOM, is there? On the other hand, HTML DOM has a number of additions, and we also use the component for things like parsing, see <https://bugs.webkit.org/describecomponents.cgi?product=WebKit>.
Created attachment 119647 [details] Patch
Darin, this removes some asserts. I can add back asserts that asserts that the node has a parentNode before calling remove if you feel like those asserts are important?
Comment on attachment 119647 [details] Patch Attachment 119647 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10914781
Qt: ../../../../Source/WebCore/xml/parser/XMLDocumentParserQt.cpp: In member function 'void WebCore::XMLDocumentParser::parseEndElement()': ../../../../Source/WebCore/xml/parser/XMLDocumentParserQt.cpp:497: error: no matching function for call to 'WebCore::ContainerNode::remove(WebCore::ExceptionCode&)'
Comment on attachment 119647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119647&action=review Please fix the Qt build before landing. > Source/WebCore/dom/Node.h:29 > +#include "ExceptionCodePlaceholder.h" No need to add this include.
Comment on attachment 119647 [details] Patch This patch is fine. A couple thoughts: -It seems that, in the presence of sync mutation events, we could get an exception in basically all these cases. This patch doesn't make this worse, but we might want to consider adding an EventScope to Node::remove. -All the error checks in ContainerNode::removeChild don't apply when remove is called from Element. Would be a nice future patch to restructure the code so that remove and removeChild can call a helper function that does the removal without the error checking. This should be a significant performance improvement.
Created attachment 119888 [details] Patch
Comment on attachment 119888 [details] Patch Attachment 119888 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10944010
Created attachment 119895 [details] Patch
Created attachment 165396 [details] Patch
Comment on attachment 165396 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165396&action=review > Source/WebCore/ChangeLog:23 > + (WebCore::Node::remove): Remove ExceptionCode parameter. Can you please change this comment to a "why" one?
Created attachment 165403 [details] Patch
(In reply to comment #24) > (From update of attachment 165396 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165396&action=review > > > Source/WebCore/ChangeLog:23 > > + (WebCore::Node::remove): Remove ExceptionCode parameter. > > Can you please change this comment to a "why" one? That is good advice. I'll try to keep that in mind
Comment on attachment 165403 [details] Patch I think you can get exceptions in all these cases due to sync firing of mutation events and blur/focus events. All the places in the existing code that ASSERT(!ec) are just wrong. Some day, I'll like for us to try an experiment where we EventScope blur/focus events as well and see if pages break, but until then, remove can throw exceptions. You obviously don't need to fixup the existing code, but I think we need to make the DOM4 method throw in the case that the child has been moved (e.g. during a blur event).
Comment on attachment 165403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165403&action=review > Source/WebCore/dom/CharacterData.idl:50 > + // DOM 4 > + void remove(); Are remove() methods actually needed everywhere, not just in Node.idl?
(In reply to comment #28) > (From update of attachment 165403 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165403&action=review > > > Source/WebCore/dom/CharacterData.idl:50 > > + // DOM 4 > > + void remove(); > > Are remove() methods actually needed everywhere, not just in Node.idl? Yes. Not all Nodes have remove.
Comment on attachment 165403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165403&action=review >> Source/WebCore/dom/CharacterData.idl:50 >> + void remove(); > > Are remove() methods actually needed everywhere, not just in Node.idl? DOM4 puts them int these three places specifically to avoid having them in non-sensical places (e.g. Document doesn't need a remove method).
I see. It feels inconsistent since removeChild exists on all nodes, but I guess that shouldn't affect web developers in practice.
(In reply to comment #31) > I see. It feels inconsistent since removeChild exists on all nodes, but I guess that shouldn't affect web developers in practice. For nodes that cannot have a parentNode remove makes no sense. Similar rules apply to before, after, prepend and append
Created attachment 165425 [details] Patch
Comment on attachment 165425 [details] Patch Clearing flags on attachment: 165425 Committed r129400: <http://trac.webkit.org/changeset/129400>
All reviewed patches have been landed. Closing bug.
Mass moving XML DOM bugs to the "DOM" Component.