Last Comment Bug 770814 - Make nsEditor::GetNodeLocation return already_AddRefed<nsIDOMNode> instead of having it as an out param
: Make nsEditor::GetNodeLocation return already_AddRefed<nsIDOMNode> instead of...
Status: RESOLVED FIXED
[mentor=ehsan][lang=c++]
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla16
Assigned To: Luqman Aden [:Luqman]
:
: Makoto Kato [:m_kato]
Mentors:
Depends on:
Blocks: 769967
  Show dependency treegraph
 
Reported: 2012-07-04 02:00 PDT by Aryeh Gregor (:ayg) (next working March 28-April 26)
Modified: 2012-07-09 04:50 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Return parent as already_AddRefed<nsIDOMNode> instead of having it as an out param (67.34 KB, patch)
2012-07-05 17:47 PDT, Luqman Aden [:Luqman]
no flags Details | Diff | Splinter Review
Return parent as already_AddRefed<nsIDOMNode> instead of having it as an out param (68.47 KB, patch)
2012-07-06 08:15 PDT, Luqman Aden [:Luqman]
ehsan: review+
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-04 02:00:16 PDT
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 2012-07-04 12:52:11 PDT
Returning an already_AddRefed here makes sense to me.
Comment 2 :Ehsan Akhgari 2012-07-05 13:07:57 PDT
Let's return the parent, and keep the offset as an outparam.
Comment 3 Luqman Aden [:Luqman] 2012-07-05 17:47:58 PDT
Created attachment 639536 [details] [diff] [review]
Return parent as already_AddRefed<nsIDOMNode> instead of having it as an out param
Comment 4 :Ehsan Akhgari 2012-07-06 07:42:28 PDT
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] 2012-07-06 08:15:35 PDT
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] 2012-07-06 08:18:13 PDT
>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 2012-07-06 08:43:48 PDT
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 2012-07-06 11:54:00 PDT
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] 2012-07-08 01:15:30 PDT
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 2012-07-08 01:30:23 PDT
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
Comment 11 Luqman Aden [:Luqman] 2012-07-08 01:32:00 PDT
(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 2012-07-08 17:46:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/392e0ec0088e

Thanks for your patch!
Comment 13 Ed Morley [:emorley] 2012-07-09 04:50:16 PDT
https://hg.mozilla.org/mozilla-central/rev/392e0ec0088e

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