The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla16

Status

()

Core
Editor
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ayg, Assigned: Luqman)

Tracking

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=ehsan][lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

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 "&".
Returning an already_AddRefed here makes sense to me.
Whiteboard: [mentor=ehsan][lang=c++]
Let's return the parent, and keep the offset as an outparam.
Summary: Make nsEditor::GetNodeLocation return offset instead of having it as an out param → Make nsEditor::GetNodeLocation return already_AddRefed<nsIDOMNode> instead of having it as an out param
(Assignee)

Comment 3

5 years ago
Created attachment 639536 [details] [diff] [review]
Return parent as already_AddRefed<nsIDOMNode> instead of having it as an out param
Attachment #639536 - Flags: review?(ehsan)
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.
Attachment #639536 - Flags: review?(ehsan)

Updated

5 years ago
Assignee: nobody → laden
(Assignee)

Comment 5

5 years ago
Created attachment 639679 [details] [diff] [review]
Return parent as already_AddRefed<nsIDOMNode> instead of having it as an out param
Attachment #639536 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #639679 - Flags: review?(ehsan)
(Assignee)

Comment 6

5 years ago
>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 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!
Attachment #639679 - Flags: review?(ehsan) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
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!
Keywords: checkin-needed
(Assignee)

Comment 9

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

Comment 10

5 years ago
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
(Assignee)

Comment 11

5 years ago
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/392e0ec0088e

Thanks for your patch!
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/392e0ec0088e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.