comm-central needs to stop using nsIDOMHTMLElement

RESOLVED FIXED in Thunderbird 60.0

Status

defect
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: bzbarsky, Assigned: jorgk, NeedInfo)

Tracking

unspecified
Thunderbird 60.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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..
Thanks for the heads-up, Boris. The chat/ is part of TB. I'll NI FRG for the suite part.
Component: General → Instant Messaging
Flags: needinfo?(frgrahl)
Product: SeaMonkey → Thunderbird
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
Actually, I can just do the suite part here as well since it's straight forward unlike bug 1433363.
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.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8945900 - Flags: review?(frgrahl)
Attachment #8945900 - Flags: review?(florian)
Attachment #8945900 - Flags: feedback?(bzbarsky)
If Boris thinks it is ok I fine with it.
Flags: needinfo?(frgrahl)
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.
Attachment #8945900 - Flags: feedback?(bzbarsky) → feedback-
Comment on attachment 8945900 [details] [diff] [review]
1433542-nsIDOMHTMLElement.patch

Thanks for the feedback, Boris.
Attachment #8945900 - Flags: review?(frgrahl)
Attachment #8945900 - Flags: review?(florian)
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?
Attachment #8945900 - Attachment is obsolete: true
Flags: needinfo?(acelists)
Sorry, it's Ci.nsIDOMNode.ELEMENT_NODE.
Flags: needinfo?(acelists)
OK, this appears to be working.
Attachment #8946145 - Attachment is obsolete: true
Attachment #8946146 - Flags: review?(frgrahl)
Attachment #8946146 - Flags: review?(florian)
Attachment #8946146 - Flags: feedback?(bzbarsky)
(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 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?
Attachment #8946146 - Flags: review?(frgrahl) → review+
Fixed the Ci. issue, it was replaced in one file only. As for the hard-coded namespace, let's see what Boris says.
Attachment #8946146 - Attachment is obsolete: true
Attachment #8946146 - Flags: review?(florian)
Attachment #8946146 - Flags: feedback?(bzbarsky)
Attachment #8946163 - Flags: review?(florian)
Attachment #8946163 - Flags: feedback?(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....
Attachment #8946163 - Flags: feedback?(bzbarsky) → feedback-
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.
Sorry about the fourth iteration on this :-(
With my humble JS and XPCOM skills I'm surprised that node.ELEMENT_NODE works.
Attachment #8946163 - Attachment is obsolete: true
Attachment #8946163 - Flags: review?(florian)
Attachment #8946203 - Flags: review?(florian)
Attachment #8946203 - Flags: feedback?(bzbarsky)
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.
> 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 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.
Attachment #8946203 - Flags: feedback?(bzbarsky) → feedback+
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.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3239b98185d0
Port bug 1418085: Remove use of nsIDOMHTMLElement. rs=bustage-fix, r=frg, f=bz
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
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.
Target Milestone: --- → Thunderbird 60.0
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.
Attachment #8946203 - Flags: review?(florian)
You need to log in before you can comment on or make changes to this bug.