337543 - Use nsINode more!

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Another patch to simplify code by using nsINode in more places comming up.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Btw, I didn't update the interface iids since the new signatures are binary compatible with the old.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Btw, I ended up not doing the nsresult-as-out-parameter thing to save addrefs in the treewalker. I noticed that we addref and release all over the place anyway. Unfortunately we have to do that since scripts can run at any point (in the filter) so the tree could mutate under us causing nodes to go away otherwise.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Merge to tip

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 222233 [details] [diff] [review]
Merge to tip

>Index: content/base/src/nsTreeWalker.cpp
>===================================================================

> nsTreeWalker::nsTreeWalker(nsIDOMNode *aRoot,
>                            PRUint32 aWhatToShow,
>                            nsIDOMNodeFilter *aFilter,
>                            PRBool aExpandEntityReferences) :
>-    mRoot(aRoot),
>+    mRoot(do_QueryInterface(aRoot)),
>     mWhatToShow(aWhatToShow),
>     mExpandEntityReferences(aExpandEntityReferences),
>-    mCurrentNode(aRoot),
>+    mCurrentNode(do_QueryInterface(aRoot)),

Hmm, those QIs can fail. You should make this constructor take nsINode if you want to do this (it's not clear to me that this is an obvious improvement) and do the QI in NS_NewTreeWalker.

> NS_IMETHODIMP nsTreeWalker::FirstChild(nsIDOMNode **_retval)

>+                                mPossibleIndexesPos+1,

Add some spaces around the + while you're here.

> NS_IMETHODIMP nsTreeWalker::LastChild(nsIDOMNode **_retval)

>+                                mPossibleIndexesPos+1,

Ditto.

>@@ -601,127 +591,83 @@ nsTreeWalker::ChildOf(nsIDOMNode* aNode,

>+nsresult nsTreeWalker::TestNode(nsINode* aNode, PRInt16* _filtered)

>+    nsCOMPtr<nsIDOMNode> domNode;
>+    if (!nodeType) {

nodeType == 0

>Index: content/html/content/src/nsGenericHTMLElement.cpp
>===================================================================

>@@ -3126,21 +3126,19 @@ nsGenericHTMLFormElement::UnbindFromTree

>-    nsCOMPtr<nsIFormControl> thisControl;
>+    nsIFormControl* thisControl = this;

Is this safe (it's not a strong ref anymore)? If so, just drop thisControl and replace it with this wherever it's used.

>@@ -3167,18 +3165,17 @@ nsGenericHTMLFormElement::BeforeSetAttr(

>-    nsCOMPtr<nsIFormControl> thisControl =
>-      do_QueryInterface(NS_STATIC_CAST(nsIContent*, this));
>+    nsIFormControl* thisControl = this;

See above.

Comment 6

[Peter van der Woude [:Peter6]]

could this have caused Bug 338541 ?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This has been fixed for some time now.