1284963 - Make CreateBR return already_AddRefed

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Andrew McCreight [:mccr8]]

Bug 1266882 has demonstrated that returning a raw pointer is error-prone, so let's just revert part 5 of bug 1156062. I'll wait to land this until bug 1260651 has landed, to avoid rebasing trouble for that patch.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Make CreateBR return already_AddRefed.

Returning already_AddRefed rather than a raw pointer makes it harder to make mistakes with refcounting like the one seen in bug 1266882.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8769879 [details] [diff] [review]
Make CreateBR return already_AddRefed.

>diff --git a/editor/libeditor/WSRunObject.h b/editor/libeditor/WSRunObject.h
>index a7cbeb50..be6c7c5 100644
>--- a/editor/libeditor/WSRunObject.h
>+++ b/editor/libeditor/WSRunObject.h
>@@ -210,19 +210,20 @@ public:
>   static nsresult PrepareToSplitAcrossBlocks(HTMLEditor* aHTMLEditor,
>                                              nsCOMPtr<nsINode>* aSplitNode,
>                                              int32_t* aSplitOffset);
> 
>   // InsertBreak inserts a br node at {aInOutParent,aInOutOffset}
>   // and makes any needed adjustments to ws around that point.
>   // example of fixup: normalws after {aInOutParent,aInOutOffset}
>   //                   needs to begin with nbsp.
>-  dom::Element* InsertBreak(nsCOMPtr<nsINode>* aInOutParent,
>-                            int32_t* aInOutOffset,
>-                            nsIEditor::EDirection aSelect);
>+  already_AddRefed<mozilla::dom::Element>

nit: |mozilla::| isn't necessary. |dom::Element| should be enough.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c622421db18
Make CreateBR return already_AddRefed. r=masayuki

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #2)
> nit: |mozilla::| isn't necessary. |dom::Element| should be enough.

Fixed.

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c622421db18