Closed Bug 1682939 Opened 2 years ago Closed 1 year ago

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

Categories

(MailNews Core :: General, task)

Tracking

(thunderbird_esr78 wontfix, thunderbird86 wontfix)

RESOLVED FIXED
87 Branch
Tracking Status
thunderbird_esr78 --- wontfix
thunderbird86 --- wontfix

People

(Reporter: benc, Assigned: benc)

References

(Blocks 1 open bug)

Details

Attachments

(10 files)

4.28 KB, patch
clokep
: review+
Details | Diff | Splinter Review
8.97 KB, patch
clokep
: review+
Details | Diff | Splinter Review
6.67 KB, patch
clokep
: review+
Details | Diff | Splinter Review
2.59 KB, patch
clokep
: review+
Details | Diff | Splinter Review
13.87 KB, patch
clokep
: review+
Details | Diff | Splinter Review
3.19 KB, patch
clokep
: review+
Details | Diff | Splinter Review
2.75 KB, patch
clokep
: review+
Details | Diff | Splinter Review
2.94 KB, patch
clokep
: review+
Details | Diff | Splinter Review
2.92 KB, patch
clokep
: review+
Details | Diff | Splinter Review
1.76 KB, patch
clokep
: review+
Details | Diff | Splinter Review

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

Summary: replace idl nsISimpleEnumerator usage with Array<T> in → replace idl nsISimpleEnumerator usage with Array<T> in chat/

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 :-)

Assignee: nobody → benc
Attachment #9199168 - Flags: review?(clokep)
Keywords: leave-open
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.
Attachment #9199168 - Flags: review?(clokep) → review+

(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 :-)

Status: NEW → ASSIGNED
Target Milestone: --- → 87 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/18b87f375fda
Remove nsISimpleEnumerator from imICoreService. r=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 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.
Attachment #9200325 - Flags: review?(clokep) → review+

(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 :-)

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...

Attachment #9199168 - Attachment description: 1682939-remove-nsISimpleEnumerator-from-imICoreService-1.patch → [checked in] 1682939-remove-nsISimpleEnumerator-from-imICoreService-1.patch

A nice simple one.

Attachment #9201316 - Flags: review?(clokep)

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.

Attachment #9201318 - Flags: review?(clokep)

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

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/81397c98fda5
Remove nsISimpleEnumerator use from prplIProtocol. r=clokep

Attachment #9201316 - Flags: review?(clokep) → review+
Attachment #9200326 - Flags: review?(clokep) → review+
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?
Attachment #9201318 - Flags: review?(clokep) → review+

(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.

Attachment #9200325 - Attachment description: 1682939-remove-nsISimpleEnumerator-from-prplIProtocol-1.patch → [checked in] 1682939-remove-nsISimpleEnumerator-from-prplIProtocol-1.patch

Pushed by geoff@darktrojan.net:
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

Attachment #9200326 - Attachment description: 1682939-remove-nsISimpleEnumerator-from-imIStatusInfo-1.patch → [checked in] 1682939-remove-nsISimpleEnumerator-from-imIStatusInfo-1.patch
Attachment #9201316 - Attachment description: 1682939-remove-nsISimpleEnumerator-in-prplIPref-1.patch → [checked in]1682939-remove-nsISimpleEnumerator-in-prplIPref-1.patch
Attachment #9201318 - Attachment description: 1682939-remove-nsISimpleEnumerator-in-imILogger-1.patch → [checked in] 1682939-remove-nsISimpleEnumerator-in-imILogger-1.patch

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

Attachment #9202166 - Flags: review?(clokep)

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...

Attachment #9202189 - Flags: review?(clokep)

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

Attachment #9202215 - Flags: review?(clokep)
Attachment #9202186 - Flags: review?(clokep) → review+
Attachment #9202189 - Flags: review?(clokep) → review+
Attachment #9202214 - Flags: review?(clokep) → review+
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...
Attachment #9202166 - Flags: review?(clokep) → review+
Attachment #9202215 - Flags: review?(clokep) → review+

Pushed by mkmelin@iki.fi:
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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Attachment #9202166 - Attachment description: 1682939-remove-nsISimpleEnumerator-from-imIAccount-1.patch → [checked in]1682939-remove-nsISimpleEnumerator-from-imIAccount-1.patch
Attachment #9202186 - Attachment description: 1682939-remove-nsISimpleEnumerator-in-prplIConversation-1.patch → [checked in] 1682939-remove-nsISimpleEnumerator-in-prplIConversation-1.patch
Attachment #9202189 - Attachment description: 1682939-remove-nsISimpleEnumerator-in-imIConversationsService-1.patch → [checked in] 1682939-remove-nsISimpleEnumerator-in-imIConversationsService-1.patch
Attachment #9202214 - Attachment description: 1682939-remove-nsISimpleEnumerator-in-imIAccountsService-1.patch → [checked in] 1682939-remove-nsISimpleEnumerator-in-imIAccountsService-1.patch
Attachment #9202215 - Attachment description: 1682939-remove-nsISimpleEnumerator-from-mIContactsService-and-prplIConversation-1.patch → [checked in] 1682939-remove-nsISimpleEnumerator-from-mIContactsService-and-prplIConversation-1.patch
You need to log in before you can comment on or make changes to this bug.