Closed
Bug 770814
Opened 9 years ago
Closed 9 years ago
Make nsEditor::GetNodeLocation return already_AddRefed<nsIDOMNode> instead of having it as an out param
Categories
(Core :: DOM: Editor, enhancement)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: ayg, Assigned: Luqman)
References
Details
(Whiteboard: [mentor=ehsan][lang=c++])
Attachments
(1 file, 1 obsolete file)
68.47 KB,
patch
|
ehsan
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Returning an already_AddRefed here makes sense to me.
Whiteboard: [mentor=ehsan][lang=c++]
Comment 2•9 years ago
|
||
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•9 years ago
|
||
Attachment #639536 -
Flags: review?(ehsan)
Comment 4•9 years ago
|
||
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•9 years ago
|
Assignee: nobody → laden
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #639536 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #639679 -
Flags: review?(ehsan)
Assignee | ||
Comment 6•9 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 7•9 years ago
|
||
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•9 years ago
|
Keywords: checkin-needed
Comment 8•9 years ago
|
||
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•9 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•9 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•9 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.
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/392e0ec0088e Thanks for your patch!
Target Milestone: --- → mozilla16
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/392e0ec0088e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•