1449147 - Clean up more nsIDOMNode usages in editor

Status: RESOLVED FIXED

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

Description

[Makoto Kato [:m_kato]]

To avoid unnecessary QI, I would like to clean up to use nsINode more instead of nsIDOMNode.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1449147 - Clean up more nsIDOMNode usages in editor.

To reduce QI, I would like to replace nsIDOMNode with nsINode.  And some
parameters in *DataTransder.cpp's methods is unused (it uses as nullptr),
so we should remove these parameters.

Also nsIDOMNodeList is unused now, so we should remove this including.

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

Comment 2

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

Comment on attachment 8962683 [details]
Bug 1449147 - Clean up more nsIDOMNode usages in editor.

https://reviewboard.mozilla.org/r/231548/#review237380

::: editor/libeditor/HTMLEditRules.cpp:5307
(Diff revision 1)
> -  NS_ENSURE_STATE(mHTMLEditor);
> -  firstChild = mHTMLEditor->GetFirstEditableChild(*node);
> -  NS_ENSURE_STATE(mHTMLEditor);
> +  nsCOMPtr<nsIContent> firstChild, lastChild;
> +  firstChild = htmlEditor->GetFirstEditableChild(aNode);
> +  lastChild = htmlEditor->GetLastEditableChild(aNode);
> -  lastChild = mHTMLEditor->GetLastEditableChild(*node);
>    if (!firstChild) {
>      // this cell has no content, nothing to align
> -  } else if (firstChild == lastChild &&
> -             firstChild->IsHTMLElement(nsGkAtoms::div)) {
> +    return NS_OK;
> +  }
> +
> +  if (firstChild == lastChild && firstChild->IsHTMLElement(nsGkAtoms::div)) {

Could you define each variable of firstChild and lastChild at first use? And then, please move lastChild after checking firstChild is not nullptr.

::: editor/libeditor/HTMLEditRules.cpp:5326
(Diff revision 1)
> -    RefPtr<Element> divElem =
> +  RefPtr<Element> divElem =
> -      mHTMLEditor->CreateNode(nsGkAtoms::div, atStartOfNode);
> -    NS_ENSURE_STATE(divElem);
> +    htmlEditor->CreateNode(nsGkAtoms::div, atStartOfNode);
> +  if (NS_WARN_IF(!divElem)) {
> +    return NS_ERROR_FAILURE;
> +  }
> -    // set up the alignment on the div
> +  // set up the alignment on the div
> -    NS_ENSURE_STATE(mHTMLEditor);

CreateNode() may cause dispatching mutation event. So, we need to check if editor is still not destroyed here. However, checking with caller may cause developers forget to check. So, it might be better to make methods which modifies the DOM tree return error if editor is destroyed and mark them MOZ_MUST_USE. I'll file a bug for it. So, keeping this is okay.

::: editor/libeditor/HTMLEditorDataTransfer.cpp:171
(Diff revision 1)
> -  return InsertHTMLWithContext(aInString, empty, empty, empty,
> -                               nullptr,  nullptr, 0, true);
> +  return DoInsertHTMLWithContext(aInString, empty, empty, empty,
> +                                 nullptr,  nullptr, 0, true, true, false);

Sigh, really difficult to understand each argument...

Comment 3

[Makoto Kato [:m_kato]]

Comment on attachment 8962683 [details]
Bug 1449147 - Clean up more nsIDOMNode usages in editor.

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

Comment 4

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 209a4b7601f7516ca90edac89abdfa5a6069d9a6 -d 55370a40de63: rebasing 454483:209a4b7601f7 "Bug 1449147 - Clean up more nsIDOMNode usages in editor. r=masayuki" (tip)
merging editor/libeditor/EditorUtils.h
merging editor/libeditor/HTMLEditorDataTransfer.cpp
merging editor/libeditor/TextEditorDataTransfer.cpp
warning: conflicts while merging editor/libeditor/HTMLEditorDataTransfer.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging editor/libeditor/TextEditorDataTransfer.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf09ea48a6f
Clean up more nsIDOMNode usages in editor. r=masayuki

Comment 6

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/eaf09ea48a6f