Last Comment Bug 683852 - Implement Node.contains(node)
: Implement Node.contains(node)
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: mozilla9
Assigned To: Olli Pettay [:smaug] (vacation Aug 25-28)
:
Mentors:
http://dvcs.w3.org/hg/domcore/raw-fil...
: 685086 698304 (view as bug list)
Depends on: 684458
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-01 02:39 PDT by Mounir Lamouri (:mounir)
Modified: 2011-10-30 15:00 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (10.83 KB, patch)
2011-09-02 10:57 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
patch (11.25 KB, patch)
2011-09-02 12:31 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
bzbarsky: review+
Details | Diff | Splinter Review
patch (11.16 KB, patch)
2011-09-02 13:36 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-09-01 02:39:14 PDT

    
Comment 1 Alex Vincent [:WeirdAl] 2011-09-01 08:36:56 PDT
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)?
Comment 2 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-01 12:37:09 PDT
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).
Comment 3 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-02 05:10:46 PDT
Sorry Alex. I was in the train and started to write patch for this...
Taking.
Comment 4 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-02 10:57:36 PDT
Created attachment 557890 [details] [diff] [review]
patch

Trying to avoid virtual calls, but GetBindingParent is there, and in
some cases IsNodeOfType.
Comment 5 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-02 11:56:37 PDT
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()
Comment 6 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-02 11:57:17 PDT
Er, GetCurrentDoc != aOther->GetCurrentDoc()
Comment 7 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-02 12:03:39 PDT
Actually both.
Comment 8 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-02 12:07:15 PDT
Bah, new patch coming.
Comment 9 Boris Zbarsky [:bz] 2011-09-02 12:13:08 PDT
Does that mean you don't want review on that one?
Comment 10 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-02 12:31:01 PDT
Created attachment 557912 [details] [diff] [review]
patch
Comment 11 Boris Zbarsky [:bz] 2011-09-02 12:50:37 PDT
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.
Comment 12 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-02 13:01:20 PDT
Ah, I could reuse nsContentUtils::ContentIsDescendantOf, but 
nsContentUtils::IsInSameAnonymousTree would be bit slower.
Comment 13 Boris Zbarsky [:bz] 2011-09-02 13:02:46 PDT
Yeah, ok.  Let's just reuse nsContentUtils::ContentIsDescendantOf.

And maybe we should add more booleans like IsElement() on nsINode...
Comment 14 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-02 13:36:04 PDT
Created attachment 557929 [details] [diff] [review]
patch
Comment 15 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-02 13:47:20 PDT
http://hg.mozilla.org/mozilla-central/rev/3dc687a271e7
Comment 16 Justin Wood (:Callek) (Away until Aug 29) 2011-09-02 21:11:54 PDT
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
Comment 17 Mounir Lamouri (:mounir) 2011-09-05 03:46:04 PDT
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?
Comment 18 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-05 03:54:43 PDT
IsNodeOfType(nsINode::eDOCUMENT) but that is virtual call.
Comment 19 :Ms2ger (⌚ UTC+1/+2) 2011-09-05 04:46:11 PDT
I guess NodeType() == nsIDOMNode::DOCUMENT_NODE is fast now.
Comment 20 Boris Zbarsky [:bz] 2011-09-05 07:08:32 PDT
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.
Comment 21 :Ms2ger (⌚ UTC+1/+2) 2011-09-07 04:15:37 PDT
*** Bug 685086 has been marked as a duplicate of this bug. ***
Comment 22 bugzilla33 2011-09-07 05:55:15 PDT
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
Comment 23 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-07 06:28:49 PDT
*** Bug 685086 has been marked as a duplicate of this bug. ***
Comment 24 Eric Shepherd [:sheppy] 2011-10-14 10:07:16 PDT
Docs updated:

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

Mentioned on Firefox 9 for developers.
Comment 25 :Ms2ger (⌚ UTC+1/+2) 2011-10-30 14:54:01 PDT
*** Bug 698304 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.