1432603 - Port bug 1432186 to mailnews: Remove some more nsIDOMNode bits

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

The following will go away:
1) nsIDOMNode::GetLocalName. Replacement: nsINode::LocalName, was bug 1432194.
2) nsIDOMNode::GetNodeValue. Replacement: nsINode::GetNodeValue, was bug 1432281.
3) nsIDOMNode::GetNodeType. Replacement: nsINode::NodeType, was bug 1432286.
4) nsIDOMNode::GetTextContent. Replacement: nsINode::GetTextContent, was bug 1432351.
5) nsIDOMNode::GetLastChild/GetFirstChild. Replacement: nsINode methods, was bug 1432352.
6) nsIDOMNode::GetNextSibling/GetPreviousSibling. Replacement: nsINode methods, was bug 1432353.
7) nsIDOMNode::GetChildNodes. Replacement: nsINode::ChildNodes, was bug 1432377.
8) nsIDOMNode::HasChildNodes. Replacement: nsINode::HasChildNodes, was bug 1432379.
9) nsIDOMNode::GetOwnerDocument. Replacement: nsINode::OwnerDoc. Watch the leak, was bug 1432383.
10) nsIDOMNode::GetParentNode. Replacement: nsINode::GetParentNode, was bug 1432387

All used in nsMsgCompose.cpp, nsMsgSend.cpp and nsWMUtils.cpp.

Boris, let me handle all this in one bug. At least a few of those bugs are the replacement of one call only.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1432603-replace-nsIDOMNode.patch - part 1 (v1)

Boris, you split into 10 bugs, so let me split it into two parts. Here come the first to affected files.

Comment 12

[Jorg K (CEST = GMT+2)]

s/to/two/.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1432603-replace-nsIDOMNode-2.patch - part 2 (v1) WIP

I've converted most calls but not all in nsMsgCompose.cpp. The rest will come soon.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8944871 [details] [diff] [review]
1432603-replace-nsIDOMNode.patch - part 1 (v1)

r=me.  Thank you for picking this up!

Comment 15

[Jorg K (CEST = GMT+2)]

For the record, part 2 still needs the following replacements:
GetLocalName: 3x
GetLastChild/GetFirstChild: 2x and 1x
GetNextSibling/GetPreviousSibling: 1x and 3x
GetChildNodes: 2x
GetParentNode: 1x.
13 calls. I'll get to it.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1432603-replace-nsIDOMNode-2.patch - part 2 (v2)

This should cover everything. Notes:
- There is a nsIAbDirectory::GetChildNodes().
- I changed/fixed the logic in nsMsgCompose::TagConvertible() as it
  appeared to be wrong.
- I'll attach a |hg qdiff -w| when the try run is done and I request review.

I sent myself a message and it didn't crash, so that's a bonus ;-)
Try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f92b59689111e3f02f550533786eb64870734eb5

Comment 17

[Jorg K (CEST = GMT+2)]

Test failures in:
cloudfile/test-cloudfile-attachment-urls.js
composition/test-signature-updating.js

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1432603-replace-nsIDOMNode-2.patch - part 2 (v2b)

Fixed the woeful logic in GetChildOffset() :-(

Both tests pass now. Boris, please see comment #16. Let me explain the "convertible" business. We basically want to work out whether an e-mail can be downgraded to plaintext without any loss. So we also look at the body attributes.

If we find a text colour != #000 and a background colour != #fff we return "altering", if we find a background image or "dir" attribute (never seen one), we return "No". The previous if/elseif block it could return "altering" based on the colours and then skip the "dir" test.

Also, I've duped another bug over here. So I need to replace
nsIDOMElement::{Get|Set|Has}Attribute(). Is that also in bug 1432186, I can't see it there.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: patch-w.patch

The same thing as part 2, but |hg qdiff -w|. Maybe it helps a bit.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8945215 [details] [diff] [review]
1432603-replace-nsIDOMNode-2.patch - part 2 (v2b)

>+++ b/mailnews/compose/src/nsMsgCompose.cpp
>+  nsINodeList* childNodes = aParent->ChildNodes();
>+  if (!childNodes)
>+    return NS_ERROR_NULL_POINTER;

ChildNodes() never returns null.  That's why it has no "Get" prefix.

>+    nsINode* childNode;
>+    childNode = childNodes->Item(i);

Why not put that all on one line?

>+nsresult nsMsgCompose::TagConvertible(Element *node,  int32_t *_retval)
>-    rv = node->GetNodeName(element);
...
>+    element = node->LocalName();

Why the change from nodeName to localName?

>+        if (node->HasAttribute(NS_LITERAL_STRING("text"))) {
>+          node->GetAttribute(NS_LITERAL_STRING("text"), color);
>+          if (!color.EqualsLiteral("#000000")) {
>+            *_retval = nsIMsgCompConvertible::Altering;
>+          } else {
>+            if (node->HasAttribute(NS_LITERAL_STRING("bgcolor"))) {

This looks wrong to me.  If the bgcolor attribute _is_ set and the text attribute is _not_ set, this will not set *_retval to Altering, whereas it should.

>@@ -5528,87 +5511,71 @@ nsresult nsMsgCompose::TagConvertible(ns
>+        nsINodeList* children = node->ChildNodes();
>+        if (children && children->Length() > 0) {

Again, children will not be null here.

>+nsresult nsMsgCompose::_NodeTreeConvertible(Element *node, int32_t *_retval)
>+    nsINodeList* children = node->ChildNodes();
>+    if (children)

And here.

>@@ -5683,29 +5652,29 @@ nsMsgCompose::MoveToAboveQuote(void)
>-    rv = node->GetNextSibling(getter_AddRefs(node));

Wow.  I really hope this code is just never reached.

>+    if (!node) {
>       // No further siblings found, so we didn't find what we were looking for.
>       rv = NS_OK;
>       node = nullptr;

node is already nullptr here.

>+++ b/mailnews/compose/src/nsMsgCompose.h
>+  nsresult TagConvertible(Element *node,  int32_t *_retval);
>+  nsresult _NodeTreeConvertible(Element *node, int32_t *_retval);

Those both need to be mozilla::dom::Element, no?

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: 1432603-replace-nsIDOMNode-2.patch - part 2 (v2c)

Thanks for the quick feedback, I hope this addresses all the issues.

To answer your questions: LocalName() was used by mistake. I fixed the converting logic, I tried to optimise :-( (and I'm still trying).

As for the "Wow.  I really hope this code is just never reached." - As far as I'm aware, that code is reached. Maybe my "assembler mind" is just too naïve, but once the call is despatched, 'node' can be overwritten, no?

Comment 23

[Jorg K (CEST = GMT+2)]

https://bugzilla.mozilla.org/attachment.cgi?oldid=8945215&action=interdiff&newid=8945258&headers=1
works (it doesn't always).

Comment 24

[Boris Zbarsky [:bzbarsky]]

> but once the call is despatched, 'node' can be overwritten, no?

C++ does not guarantee the order of the argument evaluation and the operator-> evaluation, last I checked.  Certainly code just like that used to crash-on-null with some compilers at some optimization levels in the past.

Comment 25

[Jorg K (CEST = GMT+2)]

Hmm, OK, that code runs on all platforms for every reply we send when the signature is placed above the quoted message (MoveToAboveQuote()). It's going now, so do you see the need to uplift a fix to prior versions?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8945258 [details] [diff] [review]
1432603-replace-nsIDOMNode-2.patch - part 2 (v2c)

r=me

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/49cc2b43c34f
replace some nsIDOMNode methods with nsINode methods (part 1). r=bz
https://hg.mozilla.org/comm-central/rev/cde38eb25eba
replace some nsIDOMNode methods with nsINode methods (part 2). r=bz

Comment 28

[Jorg K (CEST = GMT+2)]

Thanks, Boris!
I wanted to apologise for all the noise I created duplicating the initial 10 bugs here, but the issues needed to be fixed together.