1407309 - Rewrite HTMLEditor::CopyLastEditableChildStyles() to use internal DOM APIs

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Comment 1

[(no longer active)]

Attachment: Part 1: Rewrite HTMLEditor::CopyLastEditableChildStyles() to use internal DOM APIs

Comment 2

[(no longer active)]

Attachment: Part 2: Remove some dead code

Comment 3

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eccf1105a55486e20e97a07c967dc057267a3d60

Comment 4

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

Comment on attachment 8917057 [details] [diff] [review]
Part 1: Rewrite HTMLEditor::CopyLastEditableChildStyles() to use internal DOM APIs

> nsresult
>-HTMLEditor::CopyLastEditableChildStyles(nsIDOMNode* aPreviousBlock,
>-                                        nsIDOMNode* aNewBlock,
>+HTMLEditor::CopyLastEditableChildStyles(nsINode* aPreviousBlock,
>+                                        nsINode* aNewBlock,
>                                         Element** aOutBrNode)
> {
>   nsCOMPtr<nsINode> newBlock = do_QueryInterface(aNewBlock);
>   NS_ENSURE_STATE(newBlock || !aNewBlock);
>   *aOutBrNode = nullptr;
>-  nsCOMPtr<nsIDOMNode> child, tmp;
>+  nsCOMPtr<nsINode> child, tmp;
>   // first, clear out aNewBlock.  Contract is that we want only the styles from previousBlock.
>-  nsresult rv = aNewBlock->GetFirstChild(getter_AddRefs(child));
>-  while (NS_SUCCEEDED(rv) && child) {
>-    rv = DeleteNode(child);
>+  child = aNewBlock->GetFirstChild();
>+  while (child) {
>+    nsresult rv = DeleteNode(child);
>     NS_ENSURE_SUCCESS(rv, rv);
>-    rv = aNewBlock->GetFirstChild(getter_AddRefs(child));
>+    child = aNewBlock->GetFirstChild();
>   }
>   // now find and clone the styles
>   child = aPreviousBlock;
>   tmp = aPreviousBlock;
>   while (tmp) {
>     child = tmp;
>-    nsCOMPtr<nsINode> child_ = do_QueryInterface(child);
>-    NS_ENSURE_STATE(child_ || !child);
>-    tmp = GetAsDOMNode(GetLastEditableChild(*child_));
>+    tmp = GetLastEditableChild(*child);
>   }
>   while (child && TextEditUtils::IsBreak(child)) {
>-    nsCOMPtr<nsIDOMNode> priorNode;
>-    rv = GetPriorHTMLNode(child, address_of(priorNode));
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    nsCOMPtr<nsINode> priorNode = GetPriorHTMLNode(child);

nit: looks like that this can be nsICOntent* or nsINode*.
https://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/editor/libeditor/HTMLEditor.h#778

>+    if (NS_WARN_IF(!priorNode)) {
>+      return NS_ERROR_FAILURE;
>+    }

Hmm, old method may return NS_OK even if the result is nullptr.
https://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/editor/libeditor/HTMLEditor.cpp#3884

So, if the first child is a break, this patch may change the result of this method (actually, following code checks if the result of this loop is nullptr).

Comment 5

[(no longer active)]

Attachment: Part 1: Rewrite HTMLEditor::CopyLastEditableChildStyles() to use internal DOM APIs

Comment 6

[(no longer active)]

ping?

Comment 7

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

Comment on attachment 8917410 [details] [diff] [review]
Part 1: Rewrite HTMLEditor::CopyLastEditableChildStyles() to use internal DOM APIs

Oh, really sorry!

Comment 8

[(no longer active)]

No worries, thanks for the review!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19cc8ad9bfba
Part 1: Rewrite HTMLEditor::CopyLastEditableChildStyles() to use internal DOM APIs; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/490ba7b770e9
Part 2: Remove some dead code; r=masayuki

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/19cc8ad9bfba
https://hg.mozilla.org/mozilla-central/rev/490ba7b770e9