Implement Node.contains(node)

RESOLVED FIXED in mozilla9

Status

()

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mounir, Assigned: smaug)

Tracking

({dev-doc-complete})

Trunk
mozilla9
All
Linux
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
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
Created attachment 557890 [details] [diff] [review]
patch

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?
Created attachment 557912 [details] [diff] [review]
patch
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...
Keywords: dev-doc-needed
http://hg.mozilla.org/mozilla-central/rev/3dc687a271e7
Status: NEW → RESOLVED
Last Resolved: 7 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
(Reporter)

Comment 17

7 years ago
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

Comment 22

7 years ago
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.
Keywords: dev-doc-needed → dev-doc-complete
Duplicate of this bug: 698304

Updated

7 years ago
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.