Closed Bug 683852 Opened 8 years ago Closed 8 years ago

Implement Node.contains(node)

Categories

(Core :: DOM: Core & HTML, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: mounir, Assigned: smaug)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

No description provided.
This is the kind of bug I could do pretty quickly.  Would we want anonymous content to show up as true?  What about native anonymous content (a la the video element's controls)?
Per spec anonymous content shows up as true only within the same anonymous content (since the spec
relies on parent-to-child chain, not child-to-parent chain).
Sorry Alex. I was in the train and started to write patch for this...
Taking.
Assignee: nobody → Olli.Pettay
Attached patch patch (obsolete) — Splinter Review
Trying to avoid virtual calls, but GetBindingParent is there, and in
some cases IsNodeOfType.
Attachment #557890 - Flags: review?(bzbarsky)
Comment on attachment 557890 [details] [diff] [review]
patch


>+nsINode::Contains(const nsINode* aOther) const
>+{
>+  if (!aOther ||
>+      aOther == this ||
>+      !(aOther->IsElement() ||
>+        aOther->IsNodeOfType(nsINode::eCONTENT)) ||
>+      !GetFirstChild()) {
This is missing still one thing
.. ||
GetOwnerDoc != aOther->GetOwnerDoc()
Er, GetCurrentDoc != aOther->GetCurrentDoc()
Actually both.
Bah, new patch coming.
Does that mean you don't want review on that one?
Attached patch patchSplinter Review
Attachment #557890 - Attachment is obsolete: true
Attachment #557890 - Flags: review?(bzbarsky)
Attachment #557912 - Flags: review?(bzbarsky)
Comment on attachment 557912 [details] [diff] [review]
patch

Once you do the document check, is there a reason to not just use a combination of ContentIsDescendantOf and nsContentUtils::IsInSameAnonymousTree?

Certainly reimplementing ContentIsDescendantOf here is not something we want, I think.

Other than that, looks good.

r=me with the above issue addressed.
Attachment #557912 - Flags: review?(bzbarsky) → review+
Ah, I could reuse nsContentUtils::ContentIsDescendantOf, but 
nsContentUtils::IsInSameAnonymousTree would be bit slower.
Yeah, ok.  Let's just reuse nsContentUtils::ContentIsDescendantOf.

And maybe we should add more booleans like IsElement() on nsINode...
Attached patch patchSplinter Review
http://hg.mozilla.org/mozilla-central/rev/3dc687a271e7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
FYI, I landed a followup fix [on comm-central] to break bustage for Thunderbird and SeaMonkey.

(Admitedly the comm-* code that depends on this stuff is very very ugly) http://hg.mozilla.org/comm-central/rev/a5f596801e38
Depends on: 684458
Comment on attachment 557929 [details] [diff] [review]
patch

Review of attachment 557929 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsGenericElement.cpp
@@ +5544,5 @@
> +    return PR_FALSE;
> +  }
> +
> +  const nsIContent* other = static_cast<const nsIContent*>(aOther);
> +  if (this == GetOwnerDoc()) {

This is *very* confusing I think. If I understand it correctly, we want to check if |this| is a document. right? Isn't there more explicit way to do so?
IsNodeOfType(nsINode::eDOCUMENT) but that is virtual call.
I guess NodeType() == nsIDOMNode::DOCUMENT_NODE is fast now.
We should consider adding IsDocument() on nsINode and backing it with either a boolean flag (we have a bunch of free ones) or the NodeType() check Ms2ger suggests.
Duplicate of this bug: 685086
Not fixed,
Pleasre REOPEN, because:

http://host0001.webd.pl/bugs/firefox/contains.html?

theSameElem.contains(theSameElem) should returns TRUE

IE 9, Opera 11.5, Konqueror 4.5, Chrome 13, Safari 5.1 return TRUE
FF 9a1 returns FALSE
Docs updated:

https://developer.mozilla.org/en/DOM/Node.contains

Mentioned on Firefox 9 for developers.
Duplicate of this bug: 698304
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.