Bug 73885 – DOM4 remove method

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Erik Arvidsson]

DOM4 remove method

[Erik Arvidsson]

Created attachment 117978 [details]
Patch

[Adam Barth]

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.

[Adam Barth]

@Anne: Would you prefer DOM4 to be implemented with vendor prefixes or without?

[Ojan Vafai]

(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.

[Adam Barth]

> 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.

[Anne van Kesteren]

No prefixes works for me. Everyone agrees on this method as far as I can tell. Maybe message [e-mail] to that effect so people can complain?

[Erik Arvidsson]

Created attachment 119631 [details]
Patch

[Darin Adler]

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.

[Erik Arvidsson]

(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.

[Erik Arvidsson]

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.

[Alexey Proskuryakov]

Erik, why did you change component? This is core DOM, not HTML.

[Erik Arvidsson]

(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 :-)

[Alexey Proskuryakov]

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>.

[Erik Arvidsson]

Created attachment 119647 [details]
Patch

[Erik Arvidsson]

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?

[Early Warning System Bot]

Comment on attachment 119647 [details]
Patch

Attachment 119647 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10914781

[Darin Adler]

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&)'

[Darin Adler]

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.

[Ojan Vafai]

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.

[Erik Arvidsson]

Created attachment 119888 [details]
Patch

[Early Warning System Bot]

Comment on attachment 119888 [details]
Patch

Attachment 119888 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10944010

[Erik Arvidsson]

Created attachment 119895 [details]
Patch

[Erik Arvidsson]

Created attachment 165396 [details]
Patch

[Alexey Proskuryakov]

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?

[Erik Arvidsson]

Created attachment 165403 [details]
Patch

[Erik Arvidsson]

(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

[Ojan Vafai]

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).

[Alexey Proskuryakov]

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?

[Erik Arvidsson]

(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.

[Ojan Vafai]

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).

[Alexey Proskuryakov]

I see. It feels inconsistent since removeChild exists on all nodes, but I guess that shouldn't affect web developers in practice.

[Erik Arvidsson]

(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

[Erik Arvidsson]

Created attachment 165425 [details]
Patch

[WebKit Review Bot]

Comment on attachment 165425 [details]
Patch

Clearing flags on attachment: 165425

Committed r129400: <http://trac.webkit.org/changeset/129400>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.