1142337 - Implement NormalizedMap without __noSuchMethod__

Status: RESOLVED FIXED

Description

[aleth [:aleth]]

Comment 1

[aleth [:aleth]]

Created attachment 8576333 [details] [diff] [review]
WIP using subclassing with super

Seeing that support for 'extends' landed, I was optimistic that we could nicely fix this with classes. But sadly bug 838540 stops this WIP from working. Filing it here for possible future use.

Comment 2

[Patrick Cloke [:clokep]]

Created attachment 8582816 [details] [diff] [review]
Patch using proxies

Have you actually seen any warning about using __noSuchMethod__? Here's an implementation that uses a Proxy instead. I'm *extremely* unhappy with it, as I feel it obfuscates the code. This passes all our tests, but I didn't actually try running with this.

Comment 3

[aleth [:aleth]]

Comment on attachment 8582816 [details] [diff] [review]
Patch using proxies

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

::: chat/modules/NormalizedMap.jsm
@@ +22,5 @@
> +  // Create the wrapped Map; use the provided iterable after normalizing the
> +  // keys.
> +  let map = new Map([[aNormalize(key), val] for ([key, val] of aIterable)]);
> +
> +  return Proxy(map, new NormalizedMapHandler(aNormalize));

This bit here makes tests fail: 
ERROR TypeError: Proxy is not a function

@@ +34,2 @@
>    if (typeof(aNormalize) != "function")
>      throw "NormalizedMap must have a normalize function!";

Should be moved above let map = as that's the first place we call aNormalize.

Comment 5

[Florian Bender]

Patrick, are you still working on this? You're not assigned so this may have just slipped your radar.

Comment 6

[Patrick Cloke [:clokep]]

I'm nominally ensuring this gets done. I'll fix it eventually if no one gets to it first. Is this blocking you in some way?

Comment 7

[aleth [:aleth]]

Created attachment 8645055 [details] [diff] [review]
Fix using a class with explicit composition

I think we should take this for now to get rid of the warnings and then simplify the code using super when that becomes available. (From jorendorff's example with bugs and missing bits fixed.)

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 8645055 [details] [diff] [review]
Fix using a class with explicit composition

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

r+ with the extra comment back.

::: chat/modules/NormalizedMap.jsm
@@ +29,5 @@
> +  delete(key) { return this._map.delete(this._normalize(key)); }
> +  get(key) { return this._map.get(this._normalize(key)); }
> +  set(key, val) {
> +    this._map.set(this._normalize(key), val);
> +    return this;

Huh is this actually fixing a bug in our old implementation?

@@ -30,5 @@
> -  _map: null,
> -  // The function to apply to all keys.
> -  _normalize: null,
> -
> -  // Anything that accepts a key as an input needs to be manually overridden.

Please keep this comment.

Comment 9

[aleth [:aleth]]

url:        https://hg.mozilla.org/comm-central/rev/a50aec3c9add26db0bba6341fafb84943e44751c
changeset:  a50aec3c9add26db0bba6341fafb84943e44751c
user:       aleth <[e-mail]>
date:       Fri Aug 07 22:50:13 2015 +0200
description:
Bug 1142337 - Implement NormalizedMap without __noSuchMethod__. r=clokep

Comment 10

[aleth [:aleth]]

url:        https://hg.mozilla.org/comm-central/rev/ab1b99f2dc53314555fe0b94da988ca33e9aa942
changeset:  ab1b99f2dc53314555fe0b94da988ca33e9aa942
user:       aleth <[e-mail]>
date:       Tue Aug 11 22:03:29 2015 +0200
description:
Backout Bug 1142337 (Implement NormalizedMap without __noSuchMethod__) as ES6 classes are only available in nightlies. rs=bustage-fix a=aleth

Comment 11

[aleth [:aleth]]

url:        https://hg.mozilla.org/releases/comm-aurora/rev/c48610831fdfa3decd6f976cd4386b819a8a88ac
changeset:  c48610831fdfa3decd6f976cd4386b819a8a88ac
user:       aleth <[e-mail]>
date:       Tue Aug 11 22:08:17 2015 +0200
description:
Backout Bug 1142337 (Implement NormalizedMap without __noSuchMethod__) as ES6 classes are only available in nightlies. a=bustage-fix

Comment 12

[aleth [:aleth]]

Backed out until ES6 classes move to m-a.

Comment 13

[aleth [:aleth]]

Created attachment 8662004 [details] [diff] [review]
Patch without class

Classes aren't going to ship in time.

Comment 14

[Patrick Cloke [:clokep]]

Comment on attachment 8662004 [details] [diff] [review]
Patch without class

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

Have I mentioned that I hate this bug?

Thanks for looking at this again.

Comment 15

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/a922edf851e5c69dc39639956fa1f74820a2e199
Bug 1142337 - Implement NormalizedMap without __noSuchMethod__. r=clokep

Comment 16

[aleth [:aleth]]

Created attachment 8729075 [details] [diff] [review]
Implement NormalizedMap using a subclass

ES6 classes have now shipped beyond nightlies. ES6 subclassing has some quirks, but this is cleaner than what we have currently.

Comment 17

[aleth [:aleth]]

Might be worth comparing to the patch in comment 7 to see which is preferable.

Comment 18

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #17)
> Might be worth comparing to the patch in comment 7 to see which is
> preferable.

See also the discussion here: https://bugzilla.mozilla.org/show_bug.cgi?id=1141863#c7

Comment 19

[Patrick Cloke [:clokep]]

Comment on attachment 8729075 [details] [diff] [review]
Implement NormalizedMap using a subclass

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

I think I like this one better. Thanks for taking a look!

Comment 20

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/5fe04a507c755ea09c503e6948170ecc1f4cde1b
Bug 1142337 - Implement NormalizedMap using a subclass. r=clokep