Bug 67651

Summary: Move Element.contains to Node
Product: WebKit Reporter: Erik Arvidsson <arv>
Component: DOMAssignee: Erik Arvidsson <arv>
Status: RESOLVED FIXED    
Severity: Minor CC: ap, cdumez, darin, dglazkov, eric, rniwa, sam, timothy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#dom-node-contains
Bug Depends on: 67806    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch with O(1) document.contains
none
Patch
none
Patch DOM Core -> DOM4
none
Patch. Added null check in DeleteSelectionCommand::mergeParagraphs
none
Patch. Expanded tests
none
Patch for landing
none
Patch for landing
none
mac binding fix
none
Patch
none
Patch
none
Patch none

Description Erik Arvidsson 2011-09-06 10:26:26 PDT
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)
Comment 1 Erik Arvidsson 2011-09-06 15:26:37 PDT
Created attachment 106499 [details]
Patch
Comment 2 Erik Arvidsson 2011-09-06 16:51:55 PDT
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?
Comment 3 Erik Arvidsson 2011-09-07 10:47:37 PDT
Created attachment 106598 [details]
Patch with O(1) document.contains
Comment 4 WebKit Review Bot 2011-09-07 11:45:24 PDT
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
Comment 5 Darin Adler 2011-09-07 13:11:50 PDT
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.
Comment 6 Erik Arvidsson 2011-09-07 13:35:29 PDT
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.
Comment 7 Darin Adler 2011-09-07 13:36:43 PDT
(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.
Comment 8 Darin Adler 2011-09-07 13:37:38 PDT
(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!
Comment 9 Erik Arvidsson 2011-09-07 14:15:22 PDT
Created attachment 106635 [details]
Patch
Comment 10 Darin Adler 2011-09-07 14:16:56 PDT
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.
Comment 11 Darin Adler 2011-09-07 14:18:11 PDT
(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.
Comment 12 Erik Arvidsson 2011-09-07 14:18:54 PDT
Created attachment 106636 [details]
Patch DOM Core -> DOM4
Comment 13 Darin Adler 2011-09-07 14:19:35 PDT
Comment on attachment 106636 [details]
Patch DOM Core -> DOM4

review- because of spurious !this check
Comment 14 Erik Arvidsson 2011-09-07 14:20:51 PDT
(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.
Comment 15 Erik Arvidsson 2011-09-07 14:51:12 PDT
Created attachment 106646 [details]
Patch. Added null check in DeleteSelectionCommand::mergeParagraphs
Comment 16 Erik Arvidsson 2011-09-07 15:07:23 PDT
Created attachment 106652 [details]
Patch. Expanded tests
Comment 17 Ryosuke Niwa 2011-09-07 15:35:43 PDT
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?
Comment 18 Ryosuke Niwa 2011-09-07 15:35:43 PDT
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?
Comment 19 Darin Adler 2011-09-07 15:42:36 PDT
I agree with all of Ryosuke’s comments, but I also feel this is landable as-is.
Comment 20 Erik Arvidsson 2011-09-07 15:49:20 PDT
(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
Comment 21 Erik Arvidsson 2011-09-07 15:55:48 PDT
Created attachment 106665 [details]
Patch for landing
Comment 22 Ryosuke Niwa 2011-09-07 15:57:39 PDT
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.
Comment 23 Erik Arvidsson 2011-09-07 16:00:26 PDT
Comment on attachment 106665 [details]
Patch for landing

webkit-patch failure
Comment 24 Erik Arvidsson 2011-09-07 16:11:15 PDT
Created attachment 106669 [details]
Patch for landing
Comment 25 WebKit Review Bot 2011-09-08 12:37:25 PDT
Comment on attachment 106669 [details]
Patch for landing

Clearing flags on attachment: 106669

Committed r94781: <http://trac.webkit.org/changeset/94781>
Comment 26 WebKit Review Bot 2011-09-08 12:37:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 Ryosuke Niwa 2011-09-08 12:56:21 PDT
This broke mac build. We need to keep the entry in Element.idl for objective-c binding.
Comment 28 Ryosuke Niwa 2011-09-08 12:59:44 PDT
(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.
Comment 29 Ryosuke Niwa 2011-09-08 13:04:08 PDT
Created attachment 106775 [details]
mac binding fix
Comment 30 Ryosuke Niwa 2011-09-08 13:21:05 PDT
Reopening the bug because the landed patch broke Mac binding.
Comment 31 Erik Arvidsson 2011-09-08 13:35:22 PDT
Ryosuke: Is it out of the question to change the ObjC public interfaces?
Comment 32 Ryosuke Niwa 2011-09-08 13:36:38 PDT
(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.
Comment 33 Erik Arvidsson 2011-09-08 13:49:00 PDT
Comment on attachment 106775 [details]
mac binding fix

LGTM but I'm not a reviewer
Comment 34 Eric Seidel (no email) 2011-09-08 14:35:33 PDT
Tim used to be the public API guy, but I don't know who is these days.
Comment 35 Ryosuke Niwa 2011-09-08 14:39:09 PDT
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.
Comment 36 Timothy Hatcher 2011-09-08 14:59:00 PDT
What is wrong with "contains"? Does it conflict with some NSObject method?
Comment 37 Ryosuke Niwa 2011-09-08 15:01:27 PDT
(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.
Comment 38 Timothy Hatcher 2011-09-08 16:35:28 PDT
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.
Comment 39 Erik Arvidsson 2011-09-08 16:46:40 PDT
(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?
Comment 40 Timothy Hatcher 2011-09-08 17:28:11 PDT
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.
Comment 41 Timothy Hatcher 2011-09-08 17:30:35 PDT
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.
Comment 42 Ryosuke Niwa 2011-09-08 18:37:06 PDT
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.
Comment 43 Sam Weinig 2011-09-08 18:51:38 PDT
(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.
Comment 44 Ryosuke Niwa 2011-09-08 18:55:43 PDT
(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...
Comment 45 Erik Arvidsson 2011-09-09 09:39:30 PDT
(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?
Comment 46 Timothy Hatcher 2011-09-09 10:32:03 PDT
(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.
Comment 47 Erik Arvidsson 2011-09-09 11:32:10 PDT
Created attachment 106892 [details]
Patch
Comment 48 Eric Seidel (no email) 2011-09-09 11:35:59 PDT
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.
Comment 49 Ryosuke Niwa 2011-09-09 11:46:11 PDT
So the conclusion is to modify PublicDOMInterfaces.h ?
Comment 50 Timothy Hatcher 2011-09-09 11:55:41 PDT
(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
Comment 51 Erik Arvidsson 2011-09-09 11:57:29 PDT
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.
Comment 52 Erik Arvidsson 2011-09-09 12:15:22 PDT
Created attachment 106900 [details]
Patch
Comment 53 WebKit Review Bot 2011-09-09 12:54:41 PDT
Comment on attachment 106900 [details]
Patch

Attachment 106900 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9628463
Comment 54 Erik Arvidsson 2011-09-09 13:43:40 PDT
Created attachment 106913 [details]
Patch
Comment 55 WebKit Review Bot 2011-09-09 20:20:49 PDT
Comment on attachment 106913 [details]
Patch

Clearing flags on attachment: 106913

Committed r94898: <http://trac.webkit.org/changeset/94898>
Comment 56 WebKit Review Bot 2011-09-09 20:20:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 57 Lucas Forschler 2019-02-06 09:03:33 PST
Mass moving XML DOM bugs to the "DOM" Component.