Bug 67651 – Move Element.contains to Node

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

[Erik Arvidsson]

We currently expose contains on Element. The new spec moves this to Node to allow the check if a node is in the document (and other use cases)

[Erik Arvidsson]

Created attachment 106499 [details]
Patch

[Erik Arvidsson]

One of the reasons we are moving contains to Node is so that we can do efficient tests whether a node is in the document. contains for Document now only need to check inDocument and compare the ownerDocument which is O(1) instead of O(depth). However, to add this I either need to make contains virtual or add code for special casing Document nodes inside Node::contains. I'm unsure which one (if any) would be preferred?

[Erik Arvidsson]

Created attachment 106598 [details]
Patch with O(1) document.contains

[WebKit Review Bot]

Comment on attachment 106598 [details]
Patch with O(1) document.contains

Attachment 106598 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9608269

New failing tests:
editing/deleting/5272440.html

[Darin Adler]

Comment on attachment 106598 [details]
Patch with O(1) document.contains

View in context: https://bugs.webkit.org/attachment.cgi?id=106598&action=review

> Source/WebCore/dom/Node.cpp:1341
> +    if (nodeType() == DOCUMENT_NODE)

The nodeType call is a slow way, involving a virtual function call, to check if a node is a document. I believe the fast way is node->document() == this, but there may be an even faster way.

[Erik Arvidsson]

Comment on attachment 106598 [details]
Patch with O(1) document.contains

I'll update to use document() instead of nodeType() which is non virtual so it should also prevent the crash when this is null.

[Darin Adler]

(In reply to comment #6)
> I'll update to use document() instead of nodeType() which is non virtual so it should also prevent the crash when this is null.

It’s not legal to call a member function on an pointer that is null even if it’s non-virtual. We should not write code that depends on that. If we want a function that works with a null pointer then it needs to be a non-member function.

[Darin Adler]

(In reply to comment #6)
> I'll update to use document() instead of nodeType() which is non virtual so it should also prevent the crash when this is null.

Calling document() will still crash when this is null, because it will try to read the document pointer out of the object!

[Erik Arvidsson]

Created attachment 106635 [details]
Patch

[Darin Adler]

Comment on attachment 106635 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106635&action=review

> Source/WebCore/dom/Node.cpp:1340
> +    if (!node || !this)
>          return false;

It's not legal to call a member function on a null pointer. So it's never correct to check this for 0. This change should not be made. We don’t want this to be the one function in the entire WebKit project doing this.

[Darin Adler]

(In reply to comment #10)
> > Source/WebCore/dom/Node.cpp:1340
> > +    if (!node || !this)
> >          return false;
> 
> It's not legal to call a member function on a null pointer. So it's never correct to check this for 0. This change should not be made. We don’t want this to be the one function in the entire WebKit project doing this.

If this change was needed, we’d need test coverage too, and I think that is not possible because it’s not called that way.

[Erik Arvidsson]

Created attachment 106636 [details]
Patch DOM Core -> DOM4

[Darin Adler]

Comment on attachment 106636 [details]
Patch DOM Core -> DOM4

review- because of spurious !this check

[Erik Arvidsson]

(In reply to comment #13)
> (From update of attachment 106636 [details])
> review- because of spurious !this check

The invalid case come from the editing code. I'll add the check there.

[Erik Arvidsson]

Created attachment 106646 [details]
Patch. Added null check in DeleteSelectionCommand::mergeParagraphs

[Erik Arvidsson]

Created attachment 106652 [details]
Patch. Expanded tests

[Ryosuke Niwa]

Comment on attachment 106652 [details]
Patch. Expanded tests

View in context: https://bugs.webkit.org/attachment.cgi?id=106652&action=review

> Source/WebCore/ChangeLog:7
> +

You should explain the change and refer to the relevant parts of the spec.

> Source/WebCore/dom/Node.cpp:1340
>      if (!node)
>          return false;

Should we also optimize the case where two nodes do not belong to the same document?

> Source/WebCore/editing/DeleteSelectionCommand.cpp:608
> -    if (!startOfParagraphToMove.deepEquivalent().deprecatedNode() || !endBlock->contains(startOfParagraphToMove.deepEquivalent().deprecatedNode())) {
> +    if (!endBlock || !endBlock->contains(startOfParagraphToMove.deepEquivalent().deprecatedNode()) || !startOfParagraphToMove.deepEquivalent().deprecatedNode()) {

Is there a test that catches this crash?

[Ryosuke Niwa]

Comment on attachment 106652 [details]
Patch. Expanded tests

View in context: https://bugs.webkit.org/attachment.cgi?id=106652&action=review

> Source/WebCore/ChangeLog:7
> +

You should explain the change and refer to the relevant parts of the spec.

> Source/WebCore/dom/Node.cpp:1340
>      if (!node)
>          return false;

Should we also optimize the case where two nodes do not belong to the same document?

> Source/WebCore/editing/DeleteSelectionCommand.cpp:608
> -    if (!startOfParagraphToMove.deepEquivalent().deprecatedNode() || !endBlock->contains(startOfParagraphToMove.deepEquivalent().deprecatedNode())) {
> +    if (!endBlock || !endBlock->contains(startOfParagraphToMove.deepEquivalent().deprecatedNode()) || !startOfParagraphToMove.deepEquivalent().deprecatedNode()) {

Is there a test that catches this crash?

[Darin Adler]

I agree with all of Ryosuke’s comments, but I also feel this is landable as-is.

[Erik Arvidsson]

(In reply to comment #18)
> (From update of attachment 106652 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106652&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +
> 
> You should explain the change and refer to the relevant parts of the spec.

OK. Will do.

> > Source/WebCore/dom/Node.cpp:1340
> >      if (!node)
> >          return false;
> 
> Should we also optimize the case where two nodes do not belong to the same document?

I don't think that is a common case but I'm willing to be convinced.
 
> > Source/WebCore/editing/DeleteSelectionCommand.cpp:608
> > -    if (!startOfParagraphToMove.deepEquivalent().deprecatedNode() || !endBlock->contains(startOfParagraphToMove.deepEquivalent().deprecatedNode())) {
> > +    if (!endBlock || !endBlock->contains(startOfParagraphToMove.deepEquivalent().deprecatedNode()) || !startOfParagraphToMove.deepEquivalent().deprecatedNode()) {
> 
> Is there a test that catches this crash?

Yup, editing/deleting/5272440.html

[Erik Arvidsson]

Created attachment 106665 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 106665 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=106665&action=review

> LayoutTests/ChangeLog:11
> +        Reviewed by Darin Adler.

This line should appear before the comment but after the bug titles & urls.

> Source/WebCore/ChangeLog:11
> +        Reviewed by Darin Adler.

Ditto.

[Erik Arvidsson]

Comment on attachment 106665 [details]
Patch for landing

webkit-patch failure

[Erik Arvidsson]

Created attachment 106669 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 106669 [details]
Patch for landing

Clearing flags on attachment: 106669

Committed r94781: <http://trac.webkit.org/changeset/94781>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This broke mac build. We need to keep the entry in Element.idl for objective-c binding.

[Ryosuke Niwa]

(In reply to comment #27)
> This broke mac build. We need to keep the entry in Element.idl for objective-c binding.

I have a build fix locally and will land shortly. Just making sure it actually builds.

[Ryosuke Niwa]

Created attachment 106775 [details]
mac binding fix

[Ryosuke Niwa]

Reopening the bug because the landed patch broke Mac binding.

[Erik Arvidsson]

Ryosuke: Is it out of the question to change the ObjC public interfaces?

[Ryosuke Niwa]

(In reply to comment #31)
> Ryosuke: Is it out of the question to change the ObjC public interfaces?

Apparently we can't because they're exposed in the WebKit framework.

[Erik Arvidsson]

Comment on attachment 106775 [details]
mac binding fix

LGTM but I'm not a reviewer

[Eric Seidel (no email)]

Tim used to be the public API guy, but I don't know who is these days.

[Ryosuke Niwa]

We have a concern about defining contains on both Node.idl and Element.idl in Objective-C and we're rolling out the patch while we're figuring out how to fix that.

[Timothy Hatcher]

What is wrong with "contains"? Does it conflict with some NSObject method?

[Ryosuke Niwa]

(In reply to comment #36)
> What is wrong with "contains"? Does it conflict with some NSObject method?

So there's an Objective-C binding for bool Element::contains(Element*) but since we're moving it to Node, we now have bool Node::contains(Node*) and the binding code is giving us an error.

I attempted to fix this by adding back Element::contains(Element*) to Element.idl but we weren't sure if that'll cause problems given there already is Node::contains(Node*) in Node.idl.

[Timothy Hatcher]

It shouldn't cause problems to have the method in both places. But I'm curious what the error is, since it would be better to fix that without it being in both places.

[Erik Arvidsson]

(In reply to comment #38)
> It shouldn't cause problems to have the method in both places. But I'm curious what the error is, since it would be better to fix that without it being in both places.

Source/WebCore/bindings/objc/PublicDOMInterfaces.h has 

@interface DOMElement : DOMNode WEBKIT_VERSION_1_3
...
- (BOOL)contains:(DOMElement *)element AVAILABLE_WEBKIT_VERSION_3_0_AND_LATER;


and my patch moved contains to Node which made DOMElement not have a contains any more.

I'm not sure what errors Ryosuke saw with his patch though?

[Timothy Hatcher]

PublicDOMInterfaces.h is designed to prevent accidental changes to public API. It isn't smart enough to know when methods move up in the inheritance chain, like from DOMElement to DOMNode.

In this case it is safe to move "contains" from Element.idl to Node.idl and update PublicDOMInterfaces.h to reflect the move.

[Timothy Hatcher]

Actually it isn't smart enough to know when methods move at all. It is up to the patch author to update PublicDOMInterfaces.h when it is a binary compatible safe change or when the API has been approved to become public by Apple.

[Ryosuke Niwa]

By the way, I'd consider this as a feature addition since it adds new IDL attribute on Node. You should probably be making a post on webkit-dev.

[Sam Weinig]

(In reply to comment #42)
> By the way, I'd consider this as a feature addition since it adds new IDL attribute on Node. You should probably be making a post on webkit-dev.

I don't think that is necessary.  It is moving code around to match a new spec, let's not go to overboard on these kind of posts to webkit-dev.

[Ryosuke Niwa]

(In reply to comment #43)
> (In reply to comment #42)
> > By the way, I'd consider this as a feature addition since it adds new IDL attribute on Node. You should probably be making a post on webkit-dev.
> 
> I don't think that is necessary.  It is moving code around to match a new spec, let's not go to overboard on these kind of posts to webkit-dev.

Okay, sounds good to me. I just wanted to make sure nobody will later object to this change and complain, etc...

[Erik Arvidsson]

(In reply to comment #41)
> Actually it isn't smart enough to know when methods move at all. It is up to the patch author to update PublicDOMInterfaces.h when it is a binary compatible safe change or when the API has been approved to become public by Apple.

Timothy: I looked at PublicDOMInterfaces.h and it isn't clear to me how I need to update it. Do you mind giving me a helping hand?

[Timothy Hatcher]

(In reply to comment #45)
> (In reply to comment #41)
> > Actually it isn't smart enough to know when methods move at all. It is up to the patch author to update PublicDOMInterfaces.h when it is a binary compatible safe change or when the API has been approved to become public by Apple.
> 
> Timothy: I looked at PublicDOMInterfaces.h and it isn't clear to me how I need to update it. Do you mind giving me a helping hand?

You just edit it like you would a normal ObjC header. Remove the method from DOMElement and add it to DOMNode, and make the argument take a DOMNode instead of DOMElement.

[Erik Arvidsson]

Created attachment 106892 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 106892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106892&action=review

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:310
> +- (BOOL)contains:(DOMNode t *)node AVAILABLE_WEBKIT_VERSION_3_0_AND_LATER;

't' Looks like a typo.

[Ryosuke Niwa]

So the conclusion is to modify PublicDOMInterfaces.h ?

[Timothy Hatcher]

(In reply to comment #49)
> So the conclusion is to modify PublicDOMInterfaces.h ?

Yes. Since this change will not break existing users of the API since it just moves down in the class inheritance. We have done this a few times, for example when tabIndex, blur and focus became shared.

http://trac.webkit.org/changeset/32664/trunk/WebCore/bindings/objc/PublicDOMInterfaces.h

[Erik Arvidsson]

Thanks Timothy.

Thanks Eric. 

I'm rebuilding and re testing to ensure I didn't do anything else stupid before I'll upload a new patch.

[Erik Arvidsson]

Created attachment 106900 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106900 [details]
Patch

Attachment 106900 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9628463

[Erik Arvidsson]

Created attachment 106913 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106913 [details]
Patch

Clearing flags on attachment: 106913

Committed r94898: <http://trac.webkit.org/changeset/94898>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

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