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
Assignee

Description

6 years ago
*** 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

6 years ago
*** 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 ;)
Assignee

Updated

5 years ago
Whiteboard: [mentor=clokep][good first bug]
Assignee

Comment 2

5 years ago
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?
Assignee

Comment 4

5 years ago
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)
Assignee

Comment 5

5 years ago
Posted patch 1 - Convert buddies to a map (obsolete) — Splinter Review
Attachment #8413441 - Flags: review?(aleth)
Assignee

Comment 7

5 years ago
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 10

5 years ago
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 11

5 years ago
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 12

5 years ago
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+

Comment 13

5 years ago
(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

5 years ago
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)

Comment 15

5 years ago
(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?
Assignee

Comment 16

5 years ago
(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!
Assignee

Comment 17

5 years ago
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)
Assignee

Comment 18

5 years ago
Attachment #8413441 - Attachment is obsolete: true
Attachment #8418239 - Flags: review?(aleth)
Assignee

Comment 19

5 years ago
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)
Assignee

Comment 20

5 years ago
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")

...
Assignee

Comment 22

5 years ago
(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

5 years ago
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+

Updated

5 years ago
Attachment #8418239 - Flags: review?(aleth) → review+

Comment 24

5 years ago
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 25

5 years ago
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+
Assignee

Comment 26

5 years ago
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.
Assignee

Comment 27

5 years ago
Attachment #8420046 - Flags: review?(florian)
Attachment #8420046 - Flags: review?(florian) → review+
Assignee

Comment 29

5 years ago
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
Assignee

Updated

5 years ago
Depends on: 1012660
Assignee

Comment 30

5 years ago
Got the (last?) two now!
Attachment #8424813 - Flags: review?(aleth)

Comment 33

5 years ago
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+

Updated

5 years ago
Attachment #8424814 - Flags: review?(aleth) → review+

Comment 34

5 years ago
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.

Updated

5 years ago
Attachment #8424822 - Flags: review?(aleth) → review+
Assignee

Comment 35

5 years ago
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 36

5 years ago
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+
Assignee

Comment 37

5 years ago
And another minor type. Sets have a delete function, not a remove function. Oops.
Attachment #8424933 - Flags: review?(aleth)
Assignee

Comment 38

5 years ago
Uploaded the wrong patch, sorry.
Attachment #8424933 - Attachment is obsolete: true
Attachment #8424933 - Flags: review?(aleth)

Comment 39

5 years ago
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+
Assignee

Comment 40

5 years ago
(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.
Assignee

Comment 41

5 years ago
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
Last Resolved: 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.