955366 - Use Maps and Sets in IRC code

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Patrick Cloke [:clokep]]

*** Original post on bio 1929 at 2013-04-13 17:11:00 UTC ***

Using Maps and Sets should greatly improve the readability of our IRC code in some places, additionally it should allow us to stop using the global hasOwnProperty.

Candidates for replacement are, broken up by parent object:
ircChannel:
- _modes, [] -> Set
- banMasks, [] -> Set
- _participants, {} -> Map

ircParticipant:
- _modes, [] -> Set

ircConversation/ircChannel:
- _observedNicks, [] -> Set

ircAccount:
- _buddies, {} -> Map
- _conversations, {} -> Map
- whoisInformatoin, {} -> Map
- _caps, [] -> Set
- _chatRoomFieldsList, {} -> Map
- trackQueue must still be an array, I think (order matters), and it's probably easier to then keep pendingIsOnQueue as an array too

There are probably other instances too that could be Sets/Maps, but aren't on the class objects so are a bit harder to find.

Note that all the _modes ones would have to be changed at once as there is abstract code (_setMode) that touches them all.

Useful search:
http://lxr.instantbird.org/instantbird/search?string=[^\.]hasOwnProperty&regexp=on

Comment 1

[aleth [:aleth]]

*** Original post on bio 1929 at 2013-04-15 09:35:03 UTC ***

For extra fun, it's possible whoisinformation really wants to be a Map of Maps ;)

Comment 2

[Patrick Cloke [:clokep]]

Attachment: Convert _buddies

This is the way I'm going here, I'd like some feedback before I go hog wild and start converting everything.

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 8409973 [details] [diff] [review]
Convert _buddies

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

Just one drive by comment, I haven't really looked at the rest of the code, I'm happy for aleth to provide the feedback here :-).

::: chat/protocols/irc/ircUtils.jsm
@@ +246,5 @@
> +  get: function(aKey) this._map.get(this._normalize(aKey)),
> +  set: function(aKey, aValue)
> +    this._map.set(this._normalize(aKey), aValue),
> +  has: function(aKey) this._map.has(this._normalize(aKey)),
> +  forEach: function(...args) this._map.forEach.apply(args)

Don't you need to specify a this value here when using .apply?

Comment 4

[Patrick Cloke [:clokep]]

Attachment: 0 - Add a NormalizedMap

Comment 5

[Patrick Cloke [:clokep]]

Attachment: 1 - Convert buddies to a map

Comment 6

[Patrick Cloke [:clokep]]

Attachment: 2 - Convert whoisinformation to a map

Comment 7

[Patrick Cloke [:clokep]]

Attachment: 3 - Convert _participants to a Map

This is all I've gotten to so far, I'll get along to do more shortly!

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8413439 [details] [diff] [review]
0 - Add a NormalizedMap

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

::: chat/modules/NormalizedMap.jsm
@@ +4,5 @@
> +
> +const EXPORTED_SYMBOLS = ["NormalizedMap"];
> +
> +// A Map that automatically normalizes keys before accessing the values.
> +function NormalizedMap(aNormalizeFunction, aIt = []) {

This could do with some more documentation. aIt isn't an explicit name. Is it an initialized? an iterator (strange given that the default value is an array)?

@@ +27,5 @@
> +
> +  // Here's where the magic happens. If a method is called that isn't defined
> +  // here, just pass it to the internal _map object.
> +  __noSuchMethod__: function(aId, aArgs) this._map[aId].apply(this._map, aArgs)
> +}

nit: Missing ;

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8413444 [details] [diff] [review]
3 - Convert _participants to a Map

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

Looks good to me!

::: chat/protocols/yahoo/yahoo.js
@@ +136,5 @@
>                        _("system.message.conferenceLogoff", aName),
>                        {system: true});
>    },
>  
> +  getParticipantNames: function() [p.name for (p in this._participants)],

nit: Trailing , both before and after the patch.

Comment 10

[aleth [:aleth]]

Comment on attachment 8413439 [details] [diff] [review]
0 - Add a NormalizedMap

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

r+ with that changed variable name (and possibly a longer comment that flo likes ;)).

::: chat/modules/NormalizedMap.jsm
@@ +4,5 @@
> +
> +const EXPORTED_SYMBOLS = ["NormalizedMap"];
> +
> +// A Map that automatically normalizes keys before accessing the values.
> +function NormalizedMap(aNormalizeFunction, aIt = []) {

How about aIterable.

Comment 11

[aleth [:aleth]]

Comment on attachment 8413441 [details] [diff] [review]
1 - Convert buddies to a map

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

::: chat/protocols/irc/irc.js
@@ +1152,2 @@
>    // Return an array of buddy names.
> +  getBuddyNames: function() [buddyName for (buddyName of this.buddies.keys())],

What's the difference between this generator and this.buddies.keys()?

Comment 12

[aleth [:aleth]]

Comment on attachment 8413442 [details] [diff] [review]
2 - Convert whoisinformation to a map

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

Some welcome simplifications here.

Comment 13

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #11)
> What's the difference between this generator and this.buddies.keys()?

I should be more precise. What I mean is do we really need to convert to an array here?

Comment 14

[aleth [:aleth]]

Comment on attachment 8413444 [details] [diff] [review]
3 - Convert _participants to a Map

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

::: chat/modules/jsProtoHelper.jsm
@@ +591,2 @@
>      return new nsSimpleEnumerator(
> +      [participant for (participant of this._participants.values())]

new nsSimpleEnumerator(this._participants.values()) doesn't work?

It's a bit unfortunate we have to convert between different kinds of iterables. I suppose that's needed for XPCOM...

Comment 15

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #13)
> I should be more precise. What I mean is do we really need to convert to an
> array here?

I've just noticed nothing ever calls getBuddyNames. How about just dropping it altogether?

Comment 16

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #14)
> Comment on attachment 8413444 [details] [diff] [review]
> 3 - Convert _participants to a Map
> 
> Review of attachment 8413444 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: chat/modules/jsProtoHelper.jsm
> @@ +591,2 @@
> >      return new nsSimpleEnumerator(
> > +      [participant for (participant of this._participants.values())]
> 
> new nsSimpleEnumerator(this._participants.values()) doesn't work?
> 
> It's a bit unfortunate we have to convert between different kinds of
> iterables. I suppose that's needed for XPCOM...

I can't think of a better way to do this, unfortunately.

(In reply to aleth [:aleth] from comment #15)
> (In reply to aleth [:aleth] from comment #13)
> > I should be more precise. What I mean is do we really need to convert to an
> > array here?
> 
> I've just noticed nothing ever calls getBuddyNames. How about just dropping
> it altogether?

Yes, done!

Comment 17

[Patrick Cloke [:clokep]]

Attachment: 0.1 - Add a NormalizedMap

This adds some more documentation to NormalizedMap, hopefully this is clearer.

Comment 18

[Patrick Cloke [:clokep]]

Attachment: 1.1 - Convert buddies to a map

Comment 19

[Patrick Cloke [:clokep]]

Attachment: 3.1 - Convert _participants to a Map

Florian r+ed this, but please take another look at this! Maybe the enumerator can be simplified. (This fixes the trailing ,, btw.)

Comment 20

[Patrick Cloke [:clokep]]

Attachment: 4 - Convert Modes to a Set

Comment 21

[Benedikt Pfeifer]

Comment on attachment 8418238 [details] [diff] [review]
0.1 - Add a NormalizedMap

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

::: chat/modules/test/test_NormalizedMap.js
@@ +4,5 @@
> +Components.utils.import("resource:///modules/NormalizedMap.jsm");
> +
> +function test_setter_getter() {
> +  let m = new NormalizedMap(aStr => aStr.toLowerCase());
> +  m.set("foo", "bar")

Missing ;

@@ +5,5 @@
> +
> +function test_setter_getter() {
> +  let m = new NormalizedMap(aStr => aStr.toLowerCase());
> +  m.set("foo", "bar")
> +  m.set("BaZ", "blah")

Here too.

@@ +10,5 @@
> +  do_check_eq(m.get("FOO"), "bar");
> +
> +  let keys = [v for (v of m.keys())];
> +  do_check_eq(keys[0], "foo");
> +  do_check_eq(keys[1], "baz")

...

Comment 22

[Patrick Cloke [:clokep]]

(In reply to Benedikt Pfeifer [:Mic] from comment #21)
> Comment on attachment 8418238 [details] [diff] [review]

Thanks Mic. I took care of these nits and will upload a new patch if there are other review comments.

Comment 23

[aleth [:aleth]]

Comment on attachment 8418238 [details] [diff] [review]
0.1 - Add a NormalizedMap

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

r+ with nits fixed!

::: chat/modules/NormalizedMap.jsm
@@ +11,5 @@
> + *  aNormalize:   A function which takes a string and returns the "normalized"
> + *                version of it.
> + *  aIterable:    A iterable to prefill the map with, keys will be normalized.
> + *
> + * Returns a Map object that will automatically normalized any keys.

nit: grammar

Comment 24

[aleth [:aleth]]

Comment on attachment 8418240 [details] [diff] [review]
3.1 - Convert _participants to a Map

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

::: chat/modules/jsProtoHelper.jsm
@@ +591,2 @@
>      return new nsSimpleEnumerator(
> +      [participant for (participant of this._participants.values())]

*If* you want to improve this, the thing to do would be to fix imXPCOMUtils::nsSimpleEnumerator so its constructor can accept any iterable and not just arrays.
Up to you, this is certainly enough for r+.

Comment 25

[aleth [:aleth]]

Comment on attachment 8418241 [details] [diff] [review]
4 - Convert Modes to a Set

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

::: chat/protocols/irc/irc.js
@@ +518,5 @@
>    },
>  
>    setModesFromRestriction: function(aRestriction) {
>      // First remove all types from the list of modes.
> +    for each (let mode in this._account.channelRestrictionToModeMap)

I guess technically channelRestrictionToModeMap and userPrefixToModeMap could be Maps, but that doesn't really seem to make any difference here ;)

Comment 26

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/fe1feb473736
https://hg.mozilla.org/comm-central/rev/c6b03e55cb0f
https://hg.mozilla.org/comm-central/rev/fe529f9dd697
https://hg.mozilla.org/comm-central/rev/9c05e894da1e
https://hg.mozilla.org/comm-central/rev/ffcaadc29781

The following are still not done:
(In reply to Patrick Cloke [:clokep] from comment #0)
> ircChannel:
> - banMasks, [] -> Set
> 
> ircConversation/ircChannel:
> - _observedNicks, [] -> Set
> 
> ircAccount:
> - _conversations, {} -> Map
> - _caps, [] -> Set
> - _chatRoomFieldsList, {} -> Map

I'll try to get some more patches up soon.

Comment 27

[Patrick Cloke [:clokep]]

Attachment: Fix typo in attachment 8418240

Comment 28

[Patrick Cloke [:clokep]]

(In reply to Patrick Cloke [:clokep] from comment #27)
> Created attachment 8420046 [details] [diff] [review]
> Fix typo in attachment 8418240 [details] [diff] [review]

https://hg.mozilla.org/comm-central/rev/06bdc6dad628

Comment 29

[Patrick Cloke [:clokep]]

Just got the following when deopping myself:
Timestamp: 5/19/14, 8:42:42 AM
Error: Error running command MODE with handler RFC 2812:
{"rawMessage":":clokep_work![e-mail] MODE #testib -o clokep_work","command":"MODE","params":["#testib","-o","clokep_work"],"nickname":"clokep_work","user":"Instantbir","host":"7035D933.29DB994D.E20CE402.IP","source":"[e-mail]"}
this._modes.remove is not a function
Source File: resource://gre/components/irc.js
Line: 100
Source Code:
prpl-irc: ircHandlers._handleMessage

Comment 30

[Patrick Cloke [:clokep]]

Attachment: 5 - Convert conversations to a Map

Got the (last?) two now!

Comment 31

[Patrick Cloke [:clokep]]

Attachment: 6 - Convert usermodes variable in setMode method to Map

Comment 32

[Patrick Cloke [:clokep]]

Attachment: Fix topic settable bug introduced in attachment 8418241

Comment 33

[aleth [:aleth]]

Comment on attachment 8424813 [details] [diff] [review]
5 - Convert conversations to a Map

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

r+ modulo consideration of the comments ;)

::: chat/protocols/irc/irc.js
@@ +1496,1 @@
>      }

s/cls/class would be a lot easier to read.

::: chat/protocols/irc/ircBase.jsm
@@ +74,1 @@
>        continue; // Handle when we closed the window

Would it be better to s/this._conversations/this.conversations now we are also accessing it from outside the account object throughout the IRC code? It always looks a bit worrying when code outside the object touches "privates".

Comment 34

[aleth [:aleth]]

Comment on attachment 8424822 [details] [diff] [review]
Fix topic settable bug introduced in attachment 8418241 [details] [diff] [review]

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

Oops, indeed.

Comment 35

[Patrick Cloke [:clokep]]

Attachment: 5.1 - Convert conversations to a Map

This patch changed enough that I'll re-request review on it. s/cls/convClass/ (class is a reserved keyword) and s/_conversations/conversations/. Just would like a second pair of eyes on it! Thanks!

Comment 36

[aleth [:aleth]]

Comment on attachment 8424927 [details] [diff] [review]
5.1 - Convert conversations to a Map

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

::: chat/protocols/irc/irc.js
@@ -1177,1 @@
>  

Should we worry about the new Map being normalized with normalize() not normalizeNick()? (Just checking)

Comment 37

[Patrick Cloke [:clokep]]

Attachment: Fix wrong method call from attachment 8418241

And another minor type. Sets have a delete function, not a remove function. Oops.

Comment 38

[Patrick Cloke [:clokep]]

Attachment: Fix wrong method call from attachment 8418241

Uploaded the wrong patch, sorry.

Comment 39

[aleth [:aleth]]

Comment on attachment 8424940 [details] [diff] [review]
Fix wrong method call from attachment 8418241 [details] [diff] [review]

Sorry I didn't catch this on review.

Comment 40

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #36)
> Comment on attachment 8424927 [details] [diff] [review]
> 5.1 - Convert conversations to a Map
> 
> Review of attachment 8424927 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: chat/protocols/irc/irc.js
> @@ -1177,1 @@
> >  
> 
> Should we worry about the new Map being normalized with normalize() not
> normalizeNick()? (Just checking)

I did initially wonder this too though. Using it there seems to actually have been a bug, as some prefixes can be used for both participants and for channels (screwy, I know). So in that case we would have been stripping the prefix and isMUCName would return false. We shouldn't ever call getConversation with prefixed nicks anyway; the only place we should get prefixed nicks is for participants, we then unprefix those (in the _participants Map) and always use the unprefixed ones internally.

Comment 41

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/bcd505d23839
https://hg.mozilla.org/comm-central/rev/6ee809fb4c45
https://hg.mozilla.org/comm-central/rev/9001c159773f
https://hg.mozilla.org/comm-central/rev/3cb370e6812d

I think that was the last place that uses this (hasOwnProperty has no other calls) besides the stuff in bug 1012660.