Closed Bug 1407309 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Editor, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Attachment #8917058 - Flags: review?(masayuki)
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).
Attachment #8917057 - Flags: review?(masayuki) → review-
Attachment #8917057 - Attachment is obsolete: true
ping?
Comment on attachment 8917410 [details] [diff] [review]
Part 1: Rewrite HTMLEditor::CopyLastEditableChildStyles() to use internal DOM APIs

Oh, really sorry!
Attachment #8917410 - Flags: review?(masayuki) → review+
No worries, thanks for the review!
Pushed by eakhgari@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/19cc8ad9bfba
https://hg.mozilla.org/mozilla-central/rev/490ba7b770e9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.