nsMsgCompose.cpp should stop using nsIDOMNode::AppendChild

RESOLVED FIXED in Thunderbird 53.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: bz, Assigned: Jorg K (GMT+2))

Tracking

unspecified
Thunderbird 53.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

I'd like to remove it, along with other nsIDOMNode stuff.  So it would be best to switch this to nsINode::AppendChild (the one that takes an ErrorResult argument).
Blocks: 1318479
(Assignee)

Comment 1

5 months ago
We have tree call sites:

mailnews/compose/src/nsMsgCompose.cpp
582 rv = divElem->AppendChild(newTextNode, getter_AddRefs(resultNode));
589 rv = divElem->AppendChild(brElem, getter_AddRefs(resultNode));
811 rv = divElem->AppendChild(brElem, getter_AddRefs(extraBr));

However, the old interface nsIDOMNode is used throughout Mailnews. nsINode is not used at all. Surely we can replace these three calls, but in the scheme of things it doesn't appear right to use the new interface in one single spot.

What else do you want to remove?
> What else do you want to remove?

Long-term, all of nsIDOMNode.

In the short term, I'm only removing things that are actually unused (including in mailnews, if dxr is not lying to me), except for AppendChild, which has very few uses and whose removal allows the removal of some nontrivial Gecko code.
(Assignee)

Comment 3

5 months ago
I'll do a patch before the end of the weekend, how is that ;-)

(Meanwhile we still need a solution for bug 1316256.)
(Assignee)

Comment 4

5 months ago
Created attachment 8812164 [details] [diff] [review]
1318480.patch

I know you're not a Mailnews peer, but I think we'd be honoured by your review ;-)

Is there any better way to "convert" ErrorResult to nsresult?

Oops, no r?bz possible.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 5

5 months ago
Comment on attachment 8812164 [details] [diff] [review]
1318480.patch

Oops, that wasn't right. New one will come soon.
Attachment #8812164 - Attachment is obsolete: true
(Assignee)

Comment 6

5 months ago
Created attachment 8812174 [details] [diff] [review]
1318480.patch (v2).

This should do it. Boris, can you please take a look.
Comment on attachment 8812174 [details] [diff] [review]
1318480.patch (v2).

>@@ -573,26 +574,33 @@ nsMsgCompose::InsertDivWrappedTextAtSele
>+    ErrorResult rv2;

You want to make this an IgnoredErrorResult, so it won't assert that the exception is unhandled if things actually fail.

>     rv = htmlEditor->CreateElementWithDefaults(NS_LITERAL_STRING("br"),
>                                                getter_AddRefs(brElem));

This is preexisting, but this code should really check that either rv is success or brElem is non-null.  Otherwise it will crash doing the append.  This used to kinda work because the XPCOM AppendChild would null-check, but the nsINode one expects the caller to do that themselves.  Just throw in a "NS_ENSURE_SUCCESS_VOID(rv);" after this line, please.

>@@ -803,18 +810,24 @@ nsMsgCompose::ConvertAndLoadComposeWindo

In this one, using ErrorResult is correct, but you want to handle errors like so:

          if (rv2.Failed()) {
            return rv2.StealNSResult();
          }

to indicate that you're handling things.

>+          rv = divElem->GetFirstChild(getter_AddRefs(extraBr));

Why not keep the outer-scoped extraBr and just assign to it when you CreateElementWithDefaults the <br>?  Seems like it would preserve the current logic better.

r=me with those fixed.
Flags: needinfo?(bzbarsky)
Attachment #8812174 - Flags: review+
And thank you for picking this up!
(Assignee)

Comment 9

5 months ago
Created attachment 8812208 [details] [diff] [review]
1318480.patch (v3).

Boris, sorry to trouble you again, would you be able to look at this again.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> Why not keep the outer-scoped extraBr and just assign to it when you
> CreateElementWithDefaults the <br>?  Seems like it would preserve the
> current logic better.

Yes, but I'm a little confused:
This got removed:
- rv = divElem->AppendChild(brElem, getter_AddRefs(extraBr));
- NS_ENSURE_SUCCESS(rv, rv);
If I understand your suggestion correctly, you want me to just delete the <br> returned from CreateElementWithDefaults(). Before we deleted the 'extraBr' returned from AppendChild(). What's the difference? Are they the same? I'm even more surprised that I can't find nsDOMNode::AppendChild(), it seems to me mapped to nsDOMNode::InsertBefore() which I can't find either.
Attachment #8812174 - Attachment is obsolete: true
> Before we deleted the 'extraBr' returned from AppendChild().

appendChild always returns the thing passed to it.  So extraBr and brElem are exactly the same object in the old code, in the case when that appendChild call happens.

> I'm even more surprised that I can't find nsDOMNode::AppendChild()

There is no nsDOMNode.  Do you mean nsINode::AppendChild, or implementations of nsIDOMNode::AppendChild?
(Assignee)

Comment 11

5 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #10)
> Do you mean nsINode::AppendChild, or implementations
> of nsIDOMNode::AppendChild?
The latter. But never mind, if the old appendChild returns what was passed in, we're cool.

So I addressed the issues from comment #7 and this should be good to go.
Comment on attachment 8812208 [details] [diff] [review]
1318480.patch (v3).

r=me

Can you commit to comm-central, or do you need me to do that?
Attachment #8812208 - Flags: review+
(Assignee)

Comment 13

5 months ago
I have Level 3 rights and if you look at https://treeherder.mozilla.org/#/jobs?repo=comm-central you'll see who does a lot of pushing there ;-)

Thanks again for the cooperation. I'll land it after the next M-I to M-C merge. We like to sparse out our landings on C-C to see what's new in M-C and find problems quickly.
OK.  I did push changes to m-c a few mins ago that will burn the tree without this patch, because I was expecting it to land.  Let me know if that's a problem?
(Assignee)

Comment 15

5 months ago
Oh, they did another merge just now. No problem. I'll land my patch right now. Thanks for the heads-up.
(Assignee)

Comment 16

5 months ago
https://hg.mozilla.org/comm-central/rev/a562487662d2e0c031647269e9d3b848d75c3d8f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
You need to log in before you can comment on or make changes to this bug.