1682939 - replace idl nsISimpleEnumerator usage with Array<T> in chat/

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Ben Campbell]

Replacing nsISimpleEnumerator with Array<> (where sensible!)

Might be worth breaking into smaller bugs, but this covers:
chat/components/public/imICoreService.idl
chat/components/public/prplIProtocol.idl
chat/components/public/imIStatusInfo.idl
chat/components/public/imILogger.idl
chat/components/public/prplIPref.idl
chat/components/public/imIAccount.idl
chat/components/public/prplIConversation.idl
chat/components/public/imIAccountsService.idl
chat/components/public/imIConversationsService.idl
chat/components/public/imIContactsService.idl

Comment 1

[Ben Campbell]

Attachment: [checked in] 1682939-remove-nsISimpleEnumerator-from-imICoreService-1.patch

First in a series to remove nsISimpleEnumerator from the chat interfaces.
Patrick: I assume you're the person to review these but if not, feel free to bump the r? on to someone else :-)

Comment 2

[Patrick Cloke [:clokep]]

Comment on attachment 9199168 [details] [diff] [review]
[checked in] 1682939-remove-nsISimpleEnumerator-from-imICoreService-1.patch

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

Seems fine. I wouldn't be surprised if almost every use of `nsSimpleEnumerator` could be replaced.

Comment 3

[Ben Campbell]

(In reply to Patrick Cloke [:clokep] from comment #2)

I wouldn't be surprised if almost every use of nsSimpleEnumerator could be replaced.

Yep, that's the plan - I'm just taking it in small steps. More patches on the way :-)

Comment 4

[Ben Campbell]

Attachment: [checked in] 1682939-remove-nsISimpleEnumerator-from-prplIProtocol-1.patch

Comment 5

[Ben Campbell]

Attachment: [checked in] 1682939-remove-nsISimpleEnumerator-from-imIStatusInfo-1.patch

No major rocket surgery in these two patches, but I did set a try run going:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=fc5015d8f936a944df4b5bf6309fb7e2674935e6

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/18b87f375fda
Remove nsISimpleEnumerator from imICoreService. r=clokep

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 9200326 [details] [diff] [review]
[checked in] 1682939-remove-nsISimpleEnumerator-from-imIStatusInfo-1.patch

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

I think this needs change to `chat/components/src/imContacts.jsm` too? I believe that inherits from imStatusInfo. Maybe it doesn't though since it just passes through?

I'm guessing the iteration in `chat/content/chat-tooltip.js` works out fine with no changes?

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 9200325 [details] [diff] [review]
[checked in] 1682939-remove-nsISimpleEnumerator-from-prplIProtocol-1.patch

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

I think this looks OK.

Comment 9

[Ben Campbell]

(In reply to Patrick Cloke [:clokep] from comment #7)

I think this needs change to chat/components/src/imContacts.jsm too? I
believe that inherits from imStatusInfo. Maybe it doesn't though since it
just passes through?

I think it should be fine - the getTooltipInfo() implementations in imContacts.jsm are just shim pass-throughs.
https://searchfox.org/comm-central/source/chat/components/src/imContacts.jsm#977
https://searchfox.org/comm-central/source/chat/components/src/imContacts.jsm#1340

I'm guessing the iteration in chat/content/chat-tooltip.js works out fine
with no changes?

Yes - mostly the iterating code just wants an iterable object (in the javascript sense), so nsISimpleEnumerator and Array both fit the bill nicely.

There is still an nsISimpleEnumerator use in chat/content/chat-tooltip.js, in response to the nsIObserver 'user-info-received' event. It'd be nice to replace this (and I think there might be a couple of other events which also have an nsISimpleEnumerator payload). But I'm not quite sure what the best way forward is on this. Any suggestions most welcome :-)

Comment 10

[Magnus Melin [:mkmelin]]

If you convert getTooltipInfo() to Array<T>, then it looks like that can just be QId to an array instead. Indeed a very pointless enumerator...

Comment 11

[Ben Campbell]

Attachment: [checked in]1682939-remove-nsISimpleEnumerator-in-prplIPref-1.patch

A nice simple one.

Comment 12

[Ben Campbell]

Attachment: [checked in] 1682939-remove-nsISimpleEnumerator-in-imILogger-1.patch

A more involved patch.
I removed one unused function (getMessagesEnumerator()).
There's one bit of gratuitous renaming I should justify: I renamed the internal _getLogArray() to _getLogEntries() to try and make it clearer that it's returning directory entries rather than imILog objects. The places where it was used made it a little confusing what was being returned.

Comment 13

[Ben Campbell]

(oop - I forgot to set the checkin-needed for the prplIProtocol patch.)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/81397c98fda5
Remove nsISimpleEnumerator use from prplIProtocol. r=clokep

Comment 15

[Patrick Cloke [:clokep]]

Comment on attachment 9201318 [details] [diff] [review]
[checked in] 1682939-remove-nsISimpleEnumerator-in-imILogger-1.patch

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

I think this is OK. I believe the reasoning behind getMessagesEnumerator is no longer valid due to improvements in the underlying engine? Or is there a concern this will cause performance regressions?

Comment 16

[Ben Campbell]

(In reply to Patrick Cloke [:clokep] from comment #15)

I think this is OK. I believe the reasoning behind getMessagesEnumerator is
no longer valid due to improvements in the underlying engine? Or is there a
concern this will cause performance regressions?

To be honest I don't really know - I just saw that it wasn't being used anywhere.
All the messages are stored in an array on the logger anyway, so I don't think the enumerator provided any major benefits over getMessages(). That is, other than wrapping the data up for XPCOM via new LogMessage(...). getMessagesEnumerator created the LogMessage objects on the fly, while getMessages creates them all up front. But I don't think that's a big deal.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/80015c852161
Remove nsISimpleEnumerator use in imIStatusInfo. r=clokep
https://hg.mozilla.org/comm-central/rev/159dbfe9c699
Remove nsISimpleEnumerator use in prplIPref. r=clokep
https://hg.mozilla.org/comm-central/rev/e2fd576dfc42
Remove nsISimpleEnumerator use in imILogger. r=clokep

Comment 18

[Ben Campbell]

Attachment: [checked in]1682939-remove-nsISimpleEnumerator-from-imIAccount-1.patch

This one also fixes up a couple of minor errors in getChatRoomDefaultFieldValues().

Comment 19

[Ben Campbell]

Attachment: [checked in] 1682939-remove-nsISimpleEnumerator-in-prplIConversation-1.patch

Comment 20

[Ben Campbell]

Attachment: [checked in] 1682939-remove-nsISimpleEnumerator-in-imIConversationsService-1.patch

Just a few more :-)
I could have combined a bunch of these but I'm not at all familiar with the code and not so confident of the test coverage, so it seems more prudent to take it in little steps...

Comment 21

[Ben Campbell]

Attachment: [checked in] 1682939-remove-nsISimpleEnumerator-in-imIAccountsService-1.patch

Comment 22

[Ben Campbell]

Attachment: [checked in] 1682939-remove-nsISimpleEnumerator-from-mIContactsService-and-prplIConversation-1.patch

And one last trivial one to mop up some unnecessary includes.

Comment 23

[Patrick Cloke [:clokep]]

Comment on attachment 9202166 [details] [diff] [review]
[checked in]1682939-remove-nsISimpleEnumerator-from-imIAccount-1.patch

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

Seems fine. The change to use map doesn't really look necessary, but nothing wrong with it AFAICT.

::: chat/modules/jsProtoHelper.jsm
@@ +261,4 @@
>    },
>    getChatRoomDefaultFieldValues(aDefaultChatName) {
>      if (!this.chatRoomFields) {
> +      return new ChatRoomFieldValues({});

Huh, I wonder if this was doing the right thing regardless...

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3d0442e4664d
Remove nsISimpleEnumerator use in imIAccount. r=clokep
https://hg.mozilla.org/comm-central/rev/fc51a3203e33
Remove nsISimpleEnumerator use in prplIConversation. r=clokep
https://hg.mozilla.org/comm-central/rev/06a30b25517f
Remove nsISimpleEnumerator use in imIConversationsService. r=clokep
https://hg.mozilla.org/comm-central/rev/0039bbdb0280
Remove nsISimpleEnumerator use in imIAccountsService. r=clokep
https://hg.mozilla.org/comm-central/rev/302172a9e62f
Remove nsISimpleEnumerator include from imIContactsService.idl and prplIConversation.idl. r=clokep