Use Maps and Sets in IRC code

RESOLVED FIXED in 1.6

Status

defect
--
minor
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: clokep, Assigned: clokep)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=clokep][good first bug])

Attachments

(10 attachments, 6 obsolete attachments)

9.65 KB, patch
aleth
: review+
Details | Diff | Splinter Review
4.44 KB, patch
aleth
: review+
florian
: review+
Details | Diff | Splinter Review
9.09 KB, patch
aleth
: review+
Details | Diff | Splinter Review
16.19 KB, patch
aleth
: review+
Details | Diff | Splinter Review
7.74 KB, patch
aleth
: review+
Details | Diff | Splinter Review
1.10 KB, patch
florian
: review+
Details | Diff | Splinter Review
2.52 KB, patch
aleth
: review+
Details | Diff | Splinter Review
1.09 KB, patch
aleth
: review+
Details | Diff | Splinter Review
13.36 KB, patch
aleth
: review+
Details | Diff | Splinter Review
1.20 KB, patch
aleth
: review+
Details | Diff | Splinter Review
*** 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
*** 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 ;)
Whiteboard: [mentor=clokep][good first bug]
Posted patch Convert _buddies (obsolete) — Splinter Review
This is the way I'm going here, I'd like some feedback before I go hog wild and start converting everything.
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attachment #8409973 - Flags: feedback?(aleth)
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?
Posted patch 0 - Add a NormalizedMap (obsolete) — Splinter Review
Attachment #8409973 - Attachment is obsolete: true
Attachment #8409973 - Flags: feedback?(aleth)
Attachment #8413439 - Flags: review?(aleth)
Posted patch 1 - Convert buddies to a map (obsolete) — Splinter Review
Attachment #8413441 - Flags: review?(aleth)
This is all I've gotten to so far, I'll get along to do more shortly!
Attachment #8413444 - Flags: review?(florian)
Attachment #8413444 - Flags: review?(aleth)
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 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.
Attachment #8413444 - Flags: review?(florian) → review+
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.
Attachment #8413439 - Flags: review?(aleth) → review+
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()?
Attachment #8413441 - Flags: review?(aleth) → feedback+
Comment on attachment 8413442 [details] [diff] [review]
2 - Convert whoisinformation to a map

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

Some welcome simplifications here.
Attachment #8413442 - Flags: review?(aleth) → review+
(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 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...
Attachment #8413444 - Flags: review?(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?
(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!
This adds some more documentation to NormalizedMap, hopefully this is clearer.
Attachment #8413439 - Attachment is obsolete: true
Attachment #8418238 - Flags: review?(florian)
Attachment #8418238 - Flags: review?(aleth)
Attachment #8413441 - Attachment is obsolete: true
Attachment #8418239 - Flags: review?(aleth)
Florian r+ed this, but please take another look at this! Maybe the enumerator can be simplified. (This fixes the trailing ,, btw.)
Attachment #8413444 - Attachment is obsolete: true
Attachment #8418240 - Flags: review?(aleth)
Attachment #8418241 - Flags: review?(aleth)
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")

...
(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 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
Attachment #8418238 - Flags: review?(aleth) → review+
Attachment #8418239 - Flags: review?(aleth) → review+
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+.
Attachment #8418240 - Flags: review?(aleth) → review+
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 ;)
Attachment #8418241 - Flags: review?(aleth) → review+
Attachment #8418238 - Flags: review?(florian) → review+
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.
Attachment #8420046 - Flags: review?(florian)
Attachment #8420046 - Flags: review?(florian) → review+
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!Instantbir@7035D933.29DB994D.E20CE402.IP MODE #testib -o clokep_work","command":"MODE","params":["#testib","-o","clokep_work"],"nickname":"clokep_work","user":"Instantbir","host":"7035D933.29DB994D.E20CE402.IP","source":"Instantbir@7035D933.29DB994D.E20CE402.IP"}
this._modes.remove is not a function
Source File: resource://gre/components/irc.js
Line: 100
Source Code:
prpl-irc: ircHandlers._handleMessage
Depends on: 1012660
Got the (last?) two now!
Attachment #8424813 - Flags: review?(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".
Attachment #8424813 - Flags: review?(aleth) → review+
Attachment #8424814 - Flags: review?(aleth) → review+
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.
Attachment #8424822 - Flags: review?(aleth) → review+
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!
Attachment #8424813 - Attachment is obsolete: true
Attachment #8424927 - Flags: review?(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)
Attachment #8424927 - Flags: review?(aleth) → review+
And another minor type. Sets have a delete function, not a remove function. Oops.
Attachment #8424933 - Flags: review?(aleth)
Uploaded the wrong patch, sorry.
Attachment #8424933 - Attachment is obsolete: true
Attachment #8424933 - Flags: review?(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.
Attachment #8424940 - Flags: review+
(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.
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.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.