1416080 - Don't use virtual function for CreateBR

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement, P3)

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1416080 - Part 1. Move all CreateBR methods to TextEditor.

One of CreateBR is still virtual method, but it is unnecessary to use it.
So we should remove virtual keyword and remove override method.

Also, we should move native dom version of CreateBR to TextEditor to use it
on TextEdtitor.

Review commit: https://reviewboard.mozilla.org/r/203094/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203094/

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1416080 - Part 2. CreateMozBR should use native dom node.

Part 1. moves native dom method of CreateBR, so we should use it for
CreateMozBR, and use native dom as parameter.

Review commit: https://reviewboard.mozilla.org/r/203096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203096/

Comment 3

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

Comment on attachment 8932026 [details]
Bug 1416080 - Part 1. Move all CreateBR methods to TextEditor.

https://reviewboard.mozilla.org/r/203094/#review208400

Comment 4

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

Comment on attachment 8932027 [details]
Bug 1416080 - Part 2. CreateMozBR should use native dom node.

https://reviewboard.mozilla.org/r/203096/#review208402

::: editor/libeditor/TextEditRules.cpp:1631
(Diff revision 1)
> -TextEditRules::CreateBRInternal(nsIDOMNode* inParent,
> +TextEditRules::CreateBRInternal(nsINode& inParent,
>                                  int32_t inOffset,
>                                  bool aMozBR,
> -                                nsIDOMNode** outBRNode)
> +                                Element** aOutBRElement)
>  {
> -  NS_ENSURE_TRUE(inParent, NS_ERROR_NULL_POINTER);
> -
> -  nsCOMPtr<nsIDOMNode> brNode;
>    NS_ENSURE_STATE(mTextEditor);
> -  nsresult rv = mTextEditor->CreateBR(inParent, inOffset, address_of(brNode));
> +  RefPtr<Element> brElem = mTextEditor->CreateBR(&inParent, inOffset);

Could you make this method hold mTextEditor with local variable and use it instead of mTextEditor in this method? CreateBR() may be intercepted by mutation observers.

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 8932027 [details]
Bug 1416080 - Part 2. CreateMozBR should use native dom node.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203096/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/971d5eac56e4
Part 1. Move all CreateBR methods to TextEditor. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/70581f6aeb1d
Part 2. CreateMozBR should use native dom node. r=masayuki

Comment 7

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/971d5eac56e4
https://hg.mozilla.org/mozilla-central/rev/70581f6aeb1d