1433542 - comm-central needs to stop using nsIDOMHTMLElement

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I'm removing it in bug 1418085.

Currently there are uses in: 

* chat/modules/imContentSink.jsm
* chat/modules/imSmileys.jsm
* suite/browser/nsTypeAheadFind.js
* suite/common/nsContextMenu.js

that could probably become nodeType and namespaceURI checks.  That looks like it in comm-central.

Jorg, not sure whether you care, since these are all not Thunderbird..

Comment 1

[Jorg K (CEST = GMT+2)]

Thanks for the heads-up, Boris. The chat/ is part of TB. I'll NI FRG for the suite part.

Comment 2

[Jorg K (CEST = GMT+2)]

Actually, we have five of them:
* chat/modules/imContentSink.jsm
* chat/modules/imSmileys.jsm
* chat/modules/imThemes.jsm
* suite/browser/nsTypeAheadFind.js
* suite/common/nsContextMenu.js

Comment 3

[Jorg K (CEST = GMT+2)]

Actually, I can just do the suite part here as well since it's straight forward unlike bug 1433363.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1433542-nsIDOMHTMLElement.patch

I think this should do it. I changed one comment that implied that something that is not a HTMLElement is "whitespace"(??). Text is not element.

How do we want to handle the reviews here? If Boris thinks that this is right, he could do an r+ instead of f+, or we leave it to the respective module peers/owners.

Comment 5

[Frank-Rainer Grahl (:frg)]

If Boris thinks it is ok I fine with it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8945900 [details] [diff] [review]
1433542-nsIDOMHTMLElement.patch

>+    if (node.nodeType == Node.ELEMENT_NODE) {

Where is Node coming from in this JSM?

Also, do we want to test for element, or HTML element?  The old thing tested for HTML element.

Same questions throughout the patch.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8945900 [details] [diff] [review]
1433542-nsIDOMHTMLElement.patch

Thanks for the feedback, Boris.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1433542-nsIDOMHTMLElement.patch (v2)

Now checking the namespace as well. As for the:
> Where is Node coming from in this JSM?
I honestly don't know, Philipp told me on IRC:
  Node is a global object available in DOM documents ...

I added debug to make sure the code is actually run and I see:
=== imContentSink.jsm
JavaScript error: resource:///modules/imContentSink.jsm, line 253: ReferenceError: Node is not defined

So do I hard-code 1? Aceman, do you know?

Comment 9

[Jorg K (CEST = GMT+2)]

Sorry, it's Ci.nsIDOMNode.ELEMENT_NODE.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1433542-nsIDOMHTMLElement.patch (v3)

OK, this appears to be working.

Comment 11

[:aceman]

(In reply to Jorg K (GMT+1) from comment #9)
> Sorry, it's Ci.nsIDOMNode.ELEMENT_NODE.

Yes, I think this is mostly what I wrote on IRC.

Comment 12

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8946146 [details] [diff] [review]
1433542-nsIDOMHTMLElement.patch (v3)

r+

-    if (aNode instanceof Ci.nsIDOMHTMLElement) {
+    if (aNode.nodeType == Components.interfaces.nsIDOMNode.ELEMENT_NODE &&

You are replacing Ci. with Components.interfaces. in two files. Might look inconsistent in them overall.

Do we have something like a file for global constants somewhere? "http://www.w3.org/1999/xhtml" might never change but it would probably fit the category? Or would this be something for appconstants.jsm?

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1433542-nsIDOMHTMLElement.patch (v3b)

Fixed the Ci. issue, it was replaced in one file only. As for the hard-coded namespace, let's see what Boris says.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8946163 [details] [diff] [review]
1433542-nsIDOMHTMLElement.patch (v3b)

>+    if (node.nodeType == Components.interfaces.nsIDOMNode.ELEMENT_NODE &&

How about:

  if (node.nodeType == node.ELEMENT_NODE &&

because Components.interfaces.nsIDOMNode.ELEMENT_NODE will stop working once I land bug 1432186.  At least if you know that "node" is a DOM node.

In general, you should not be adding Ci.nsIDOM* of any sort anywhere; they are all going away as soon as I can manage....

Comment 15

[Boris Zbarsky [:bzbarsky]]

Oh, and the hardcoded namespace is not going to change (would break the web).  But sure, if you want to put it in some shared place that would be fine...

Note that it's not 100% clear to me whether all these places _really_ care about checking for HTML elements as opposed to all elements; that's something for someone who knows that code or has time to dig into it to check.  I just suggested something that is behavior-preserving.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1433542-nsIDOMHTMLElement.patch (v4)

Sorry about the fourth iteration on this :-(
With my humble JS and XPCOM skills I'm surprised that node.ELEMENT_NODE works.

Comment 17

[:aceman]

Yes, it does, any nsI* object seems to contain also the constants of the nsI* interface. Only sometimes we do not have an object available so you can use Ci.nsI* to get at the constant.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Yes, it does, any nsI* object seems to contain also the constants of the nsI* interface.

Fwiw, these are not "nsI* objects"; they're WebIDL objects.

WebIDL constants are present on both the constructor and the prototype for the interface.  So node.ELEMENT_NODE finds the constant up the proto chain on node's global's Node.prototype.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8946203 [details] [diff] [review]
1433542-nsIDOMHTMLElement.patch (v4)

>+++ b/chat/modules/imSmileys.jsm
>+    if (node.nodeType == node.ELEMENT_NODE &&
>+        node.namespaceURI == "http://www.w3.org/1999/xhtml") {

As a followup bug, I bet the namespaceURI check here is really wrong and should go away.  This wants to process the kids of any elements, not just HTML ones...

>+++ b/chat/modules/imThemes.jsm
>@@ -972,27 +974,29 @@ function getMessagesForRange(aRange)
>+    if (aNode.nodeType == aNode.ELEMENT_NODE &&
>+        aNode.namespaceURI == "http://www.w3.org/1999/xhtml") {

Likewise here.

>+  if (currentNode.nodeType == currentNode.ELEMENT_NODE &&
>+      currentNode.namespaceURI == "http://www.w3.org/1999/xhtml") {

And possibly here.

Comment 20

[Jorg K (CEST = GMT+2)]

Thanks for the comments, Boris. I'm not familiar with the code and I did read your comments before re. the element vs. HTML element. In the suite/ part it checks for "content editable", so that needs to be done on an HTML element, but here is chat I agree that it's not necessary. Maybe Florian can help us out.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3239b98185d0
Port bug 1418085: Remove use of nsIDOMHTMLElement. rs=bustage-fix, r=frg, f=bz

Comment 22

[Jorg K (CEST = GMT+2)]

I had to land this as bustage-fix since bug 1418085 landed today.

Maybe we should carry over the chat part to another bug to investigate whether we can check the elements as suggested, see comment #19. The landed patch will replicate the existing functionality one-to-one.

Comment 23

[Florian Quèze [:florian]]

Comment on attachment 8946203 [details] [diff] [review]
1433542-nsIDOMHTMLElement.patch (v4)

Removing the review flag as this has already landed, but leaving on the needinfo on nhnt11 to figure out if some follow-up is needed here.