770814 - Make nsEditor::GetNodeLocation return already_AddRefed<nsIDOMNode> instead of having it as an out param

Status: RESOLVED FIXED

Description

[Aryeh Gregor (:ayg) (next working August 2-22)]

See bug 769967 comment 9, and my response in bug 769967 comment 13.  I don't see the value in having it return one param when the other is still an out param.  If we are going to return one of the params, I'd prefer it be the node, because "getter_AddRefs()" is longer than "&".

Comment 1

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

Returning an already_AddRefed here makes sense to me.

Comment 2

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

Let's return the parent, and keep the offset as an outparam.

Comment 3

[Luqman Aden [:Luqman]]

Created attachment 639536 [details] [diff] [review]
Return parent as already_AddRefed<nsIDOMNode> instead of having it as an out param

Comment 4

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

Comment on attachment 639536 [details] [diff] [review]
Return parent as already_AddRefed<nsIDOMNode> instead of having it as an out param

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

The patch looks really good, thanks!  I have a few comments on it, most of them are just nits and similar to one another (the first one is a serious problem though).  I'd appreciate if you address these comments and attach a new version of the patch.  Thanks!

Also, do you have access to the try server so that you can test the patch?  If not, I can help you get access to it if you want.  :-)  See https://wiki.mozilla.org/ReleaseEngineering/TryServer if you're not familiar with the try server.

::: editor/libeditor/base/nsEditor.cpp
@@ +2960,5 @@
>  
>      PRUint32 firstNodeLength;
>      result = GetLengthOfDOMNode(leftNode, firstNodeLength);
>      NS_ENSURE_SUCCESS(result, result);
> +    GetNodeLocation(aNodeToJoin, &joinOffset);

This will return an already_AddRefed, but will not assign it to anything, which causes us to leak a reference to the interface (because AddRef was called on it, but no matching Release was ever called.)  You need to assign the return value to the parent variable, even though it is immediately overwritten on the next line.

::: editor/libeditor/html/nsHTMLEditRules.cpp
@@ +759,5 @@
>    nsCOMPtr<nsIDOMElement> rootElem = do_QueryInterface(mHTMLEditor->GetRoot());
>    NS_ENSURE_TRUE(rootElem, NS_ERROR_FAILURE);
>  
>    PRInt32 offset, rootOffset;
> +  parent = nsEditor::GetNodeLocation(rootElem, &rootOffset);

Nit: please remove the parent declaration above and declare and initialize it here in one go.

@@ +2852,5 @@
>      if (isEmpty)
>      {
>        nsCOMPtr<nsIDOMNode> parent, brNode;
>        PRInt32 offset;
> +      parent = nsEditor::GetNodeLocation(citeNode, &offset);

Nit: please move the declaration of parent to this line.

@@ +4146,5 @@
>    PRInt32 startOffset, endOffset, offset;
>    nsresult res;
>  
>    // get split point location
> +  startParent = nsEditor::GetNodeLocation(aStartChild, &startOffset);

Nit: please move the initialization of startParent to this line.

@@ +4159,5 @@
>    if (aLeftNode) 
>      *aLeftNode = leftNode;
>  
>    // get split point location
> +  endParent = nsEditor::GetNodeLocation(aEndChild, &endOffset);

Same thing for endParent.

@@ +7053,5 @@
>    // caller responsible for:
>    //   left & right node are same type
>    PRInt32 parOffset;
>    nsCOMPtr<nsIDOMNode> parent, rightParent;
> +  parent = nsEditor::GetNodeLocation(aNodeLeft, &parOffset);

Nit: please move the declaration of parent to this line.

@@ +7329,1 @@
>        tmp = tmp2;

Nit: there is no reason for us to have tmp2 here and then assign it to tmp.  Please assign the return value directly to tmp.

@@ +7343,1 @@
>        tmp = tmp2;

Same here.

@@ +8704,5 @@
>          NS_ENSURE_SUCCESS(res, res);
>          if (!curPositionedDiv) {
>            PRInt32 parentOffset;
>            nsCOMPtr<nsIDOMNode> curParentParent;
> +          curParentParent = nsEditor::GetNodeLocation(curParent, &parentOffset);

Nit: please move the declaration to the same line.

@@ +8753,5 @@
>            NS_ENSURE_SUCCESS(res, res);
>            if (!curPositionedDiv) {
>            PRInt32 parentOffset;
>            nsCOMPtr<nsIDOMNode> curParentParent;
> +          curParentParent = nsEditor::GetNodeLocation(curParent, &parentOffset);

Same here.

::: editor/libeditor/html/nsWSRunObject.cpp
@@ +117,5 @@
>    NS_ENSURE_TRUE(aNode && aHTMLEd, NS_ERROR_NULL_POINTER);
>    
>    nsCOMPtr<nsIDOMNode> parent;
>    PRInt32 offset;
> +  parent = aHTMLEd->GetNodeLocation(aNode, &offset);

Nit: please move the declaration to the same line.

::: editor/libeditor/text/nsPlaintextEditor.cpp
@@ +430,5 @@
>      nsCOMPtr<nsIDOMNode> tmp;
>      PRInt32 offset;
>      PRUint32 len;
>      nodeAsText->GetLength(&len);
> +    tmp = GetNodeLocation(node, &offset);

Nit: please move the declaration to the same line.

Comment 5

[Luqman Aden [:Luqman]]

Created attachment 639679 [details] [diff] [review]
Return parent as already_AddRefed<nsIDOMNode> instead of having it as an out param

Comment 6

[Luqman Aden [:Luqman]]

>Also, do you have access to the try server so that you can test the patch?  If not, I can help you get access to it if you want.  :-)  See https://wiki.mozilla.org/ReleaseEngineering/TryServer if you're not familiar with the try server.

No, I don't have access to the try server.

Comment 7

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

Comment on attachment 639679 [details] [diff] [review]
Return parent as already_AddRefed<nsIDOMNode> instead of having it as an out param

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

Looks great, r=me.  Thanks a lot for your patch!

Comment 8

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

It would be great if you can push your patch to the try server first (when you get access) before we check in this patch, to make sure that it doesn't break anything.  Please put a TBPL link to your try push once it's done, and I'll watch it, and will then land your patch.  Thanks!

Comment 9

[Luqman Aden [:Luqman]]

Finally figured out how to push to try server. Here's the link: https://tbpl.mozilla.org/?tree=Try&rev=59f968bfbf6f

Comment 10

[Mozilla RelEng Bot]

Try run for fb98204ac07d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fb98204ac07d
Results (out of 19 total builds):
    exception: 18
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fb98204ac07d

Comment 11

[Luqman Aden [:Luqman]]

(In reply to Mozilla RelEng Bot from comment #10)
> Try run for fb98204ac07d is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=fb98204ac07d
> Results (out of 19 total builds):
>     exception: 18
>     success: 1
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/laden@csclub.
> uwaterloo.ca-fb98204ac07d

This would be the wrong one i pushed initially.

Comment 12

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/392e0ec0088e

Thanks for your patch!

Comment 13

[Ed Morley (Away 27th-2nd) [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/392e0ec0088e