Implement NormalizedMap without __noSuchMethod__

RESOLVED FIXED in Instantbird 43

Status

Chat Core
General
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

(Depends on: 1 bug)

trunk
Instantbird 43
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

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

Updated

3 years ago
Depends on: 838540
Blocks: 683218
No longer depends on: 1140428
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.
Attachment #8582816 - Flags: review?(aleth)
(Assignee)

Comment 3

2 years ago
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.
Attachment #8582816 - Flags: review?(aleth) → review-
Duplicate of this bug: 1148638
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Updated

2 years ago
Depends on: 1141863

Comment 5

2 years ago
Patrick, are you still working on this? You're not assigned so this may have just slipped your radar.
Flags: needinfo?(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?
Flags: needinfo?(clokep)
(Assignee)

Updated

2 years ago
Attachment #8576333 - Attachment description: WIP using classes → WIP using subclassing with super
(Assignee)

Comment 7

2 years ago
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.)
Attachment #8645055 - Flags: review?(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.
Attachment #8645055 - Flags: review?(clokep) → review+
Assignee: nobody → aleth
Status: NEW → ASSIGNED
(Assignee)

Comment 9

2 years ago
url:        https://hg.mozilla.org/comm-central/rev/a50aec3c9add26db0bba6341fafb84943e44751c
changeset:  a50aec3c9add26db0bba6341fafb84943e44751c
user:       aleth <aleth@instantbird.org>
date:       Fri Aug 07 22:50:13 2015 +0200
description:
Bug 1142337 - Implement NormalizedMap without __noSuchMethod__. r=clokep
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 42
(Assignee)

Comment 10

2 years ago
url:        https://hg.mozilla.org/comm-central/rev/ab1b99f2dc53314555fe0b94da988ca33e9aa942
changeset:  ab1b99f2dc53314555fe0b94da988ca33e9aa942
user:       aleth <aleth@instantbird.org>
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
(Assignee)

Comment 11

2 years ago
url:        https://hg.mozilla.org/releases/comm-aurora/rev/c48610831fdfa3decd6f976cd4386b819a8a88ac
changeset:  c48610831fdfa3decd6f976cd4386b819a8a88ac
user:       aleth <aleth@instantbird.org>
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
(Assignee)

Comment 12

2 years ago
Backed out until ES6 classes move to m-a.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

2 years ago
Depends on: 837314
(Assignee)

Updated

2 years ago
No longer depends on: 838540
(Assignee)

Comment 13

2 years ago
Created attachment 8662004 [details] [diff] [review]
Patch without class

Classes aren't going to ship in time.
Attachment #8662004 - Flags: review?(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.
Attachment #8662004 - Flags: review?(clokep) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 15

2 years ago
https://hg.mozilla.org/comm-central/rev/a922edf851e5c69dc39639956fa1f74820a2e199
Bug 1142337 - Implement NormalizedMap without __noSuchMethod__. r=clokep
(Assignee)

Updated

2 years ago
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Instantbird 42 → Instantbird 43
(Assignee)

Comment 16

2 years ago
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.
Attachment #8729075 - Flags: review?(clokep)
(Assignee)

Updated

2 years ago
Attachment #8576333 - Attachment is obsolete: true
(Assignee)

Comment 17

2 years ago
Might be worth comparing to the patch in comment 7 to see which is preferable.
(Assignee)

Comment 18

2 years ago
(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 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!
Attachment #8729075 - Flags: review?(clokep) → review+
(Assignee)

Comment 20

a year ago
https://hg.mozilla.org/comm-central/rev/5fe04a507c755ea09c503e6948170ecc1f4cde1b
Bug 1142337 - Implement NormalizedMap using a subclass. r=clokep
You need to log in before you can comment on or make changes to this bug.