Closed
Bug 1142337
Opened 7 years ago
Closed 7 years ago
Implement NormalizedMap without __noSuchMethod__
Categories
(Chat Core :: General, defect)
Chat Core
General
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)
2.57 KB,
patch
|
aleth
:
review-
|
Details | Diff | Splinter Review |
4.55 KB,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
3.40 KB,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
3.01 KB,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•7 years ago
|
||
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.
Updated•7 years ago
|
Comment 2•7 years ago
|
||
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•7 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-
Updated•7 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 5•7 years ago
|
||
Patrick, are you still working on this? You're not assigned so this may have just slipped your radar.
Flags: needinfo?(clokep)
Comment 6•7 years ago
|
||
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•7 years ago
|
Attachment #8576333 -
Attachment description: WIP using classes → WIP using subclassing with super
Assignee | ||
Comment 7•7 years ago
|
||
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 8•7 years ago
|
||
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+
Updated•7 years ago
|
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•7 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•7 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 42
Assignee | ||
Comment 10•7 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•7 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•7 years ago
|
||
Backed out until ES6 classes move to m-a.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•7 years ago
|
Depends on: harmony-classes
Assignee | ||
Comment 13•7 years ago
|
||
Classes aren't going to ship in time.
Attachment #8662004 -
Flags: review?(clokep)
Comment 14•7 years ago
|
||
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•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 15•7 years ago
|
||
https://hg.mozilla.org/comm-central/rev/a922edf851e5c69dc39639956fa1f74820a2e199 Bug 1142337 - Implement NormalizedMap without __noSuchMethod__. r=clokep
Assignee | ||
Updated•7 years ago
|
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Instantbird 42 → Instantbird 43
Assignee | ||
Comment 16•6 years ago
|
||
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•6 years ago
|
Attachment #8576333 -
Attachment is obsolete: true
Assignee | ||
Comment 17•6 years ago
|
||
Might be worth comparing to the patch in comment 7 to see which is preferable.
Assignee | ||
Comment 18•6 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 19•6 years ago
|
||
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•6 years 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.
Description
•