1458057 - Replace use of nsIDOMNode in mailnews and Calendar

Status: RESOLVED FIXED

(MailNews Core :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

nsIDOMNode will go away one day, there is C++ usage in mailnews/compose/src/nsMsgCompose.cpp and some JS use in mailnews and Calendar:
https://dxr.mozilla.org/comm-central/search?q=nsIDOMNode&redirect=false

Comment 1

[Jorg K (CEST = GMT+2)]

Hmm, right now we can't do that due to the use of
HTMLEditor::InsertAsQuotation(const nsAString& aQuotedText, nsIDOMNode** aNodeInserted).

Boris, what are the plans for nsIDOMNode? I know that Aryeh Gregor used to work on its elimination, but there is still some/much use in Core::Editor, like the one above.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Boris, what are the plans for nsIDOMNode?

I'll likely kill it in the next week or two.

Comment 3

[Jorg K (CEST = GMT+2)]

Please CC me on the bug. When another version of HTMLEditor::InsertAsQuotation() becomes available, I'll start.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Please CC me on the bug

Done, but it was the only other open bug with nsIDOMNode in the summary....

In any case, you probably want to just watch bug 1387169 and cc yourself on anything that gets filed blocking it.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1458057-replace-nsIDOMNode.patch

This is very straight forward. This compiles with bug 1463327 and bug 1463981 applied.

Boris, can you please advise me on the remaining usage of nsIDOMNode in Calendar and logHelper.js
https://dxr.mozilla.org/comm-central/search?q=nsIDOMNode&redirect=false

I guess the Calendar stuff is just comment, so that will continue working. How should I change the comments? Just 'Node' on 'nsINode'. And what happened to nsIDOMNode List? That's gone too, right? What should that comment say? 'nsINodeList'?

What about |else if (aObj instanceof Ci.nsIDOMNode) {| in logHelper.js?

Is that the last of the nsIDOM* removals that will affect us?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8980377 [details] [diff] [review]
1458057-replace-nsIDOMNode.patch

r=me

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I guess the Calendar stuff is just comment, so that will continue working. How should I change the comments? 

I would write {Node} instead of {nsIDOMNode}.  Similar for {NodeList}.

> What about |else if (aObj instanceof Ci.nsIDOMNode) {| in logHelper.js?

Once the patches for bug 1455676 land, you will be able to add "Node" to the importGlobalProperties call in that file and do:

  else if (Node.isInstance(aObj)) {

I suppose I could land the support for that now if that would be helpful, even before I finish writing the other Node patches.

Comment 8

[Jorg K (CEST = GMT+2)]

Thanks for the review and the answers. Don't worry, I'll just land the C++ part for now since the editor changes will be merged soon. I suppose bug 1455676 isn't far off?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I suppose bug 1455676 isn't far off?

Hopefully sometime next week.  Definitely not before Wednesday...

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1458057-js-part.patch

Philipp, please see comment #7. The C++ part will land now, the JS part needs to wait for bug 1455676.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1a6333657ff7
Replace use of nsIDOMNode in nsMsgCompose.cpp. r=bz

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8980494 [details] [diff] [review]
1458057-js-part.patch

Looks like Philipp won't be reviewing this on time.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8980494 [details] [diff] [review]
1458057-js-part.patch

r=me

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/446d5a0da792
Replace use of nsIDOMNode in comments in Calendar, fix logHelper.js. r=bz DONTBUILD

Comment 15

[Jorg K (CEST = GMT+2)]

Pushed with DONTBUILD. The next Daily will build this, by then bug 1455676 should be merged to M-C, currently on inbound.

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8980494 [details] [diff] [review]
1458057-js-part.patch

Review of attachment 8980494 [details] [diff] [review]:
-----------------------------------------------------------------

Some minor nits I would appreciate, but given this already landed you can also just ignore them and we'll take care some other time.

::: calendar/base/content/calendar-ui-utils.js
@@ +613,5 @@
>   * Sets up the attendance context menu, based on the given items
>   *
> + * @param {Node}  aMenu   The context menu item containing the required
> + *                          menu or menuitem elements
> + * @param {Array} aItems  An array of the selecetd calEvent or calTodo

Can you change this to:

@param {calIItemBase[]} aItems    An array of the selected ...

(typo fix, and more specific type)

::: calendar/lightning/content/lightning-item-panel.js
@@ +488,5 @@
>  /**
>   * Handler for changing privacy. aEvent is used for the popup menu
>   * event-privacy-menupopup in the Privacy toolbar button.
>   *
> + * @param {Node}       aTarget      Has the new privacy in its "value" attribute

No need for the extra spaces, I know this is how it is in some other files but given a goal of max line length of ~100, I'm starting to prefer one space only between the type and argument name.

Comment 17

[Jorg K (CEST = GMT+2)]

Thanks. The "@param {Array}" documentation was pre-existing and punctual white-space management in an overall inconsistent code base doesn't seem all that useful.