Closed Bug 1142337 Opened 5 years ago Closed 4 years ago

Implement NormalizedMap without __noSuchMethod__

Categories

(Chat Core :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Instantbird 43

People

(Reporter: aleth, Assigned: aleth)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

No description provided.
Attached patch WIP using subclassing with super (obsolete) — Splinter Review
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.
Depends on: 838540
Blocks: 683218
No longer depends on: 1140428
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)
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
Depends on: 1141863
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)
Attachment #8576333 - Attachment description: WIP using classes → WIP using subclassing with super
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
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
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 42
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
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
Backed out until ES6 classes move to m-a.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: harmony-classes
No longer depends on: 838540
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+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/a922edf851e5c69dc39639956fa1f74820a2e199
Bug 1142337 - Implement NormalizedMap without __noSuchMethod__. r=clokep
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Instantbird 42 → Instantbird 43
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)
Attachment #8576333 - Attachment is obsolete: true
Might be worth comparing to the patch in comment 7 to see which is preferable.
(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+
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.