Closed Bug 1009349 Opened 8 years ago Closed 7 years ago

Deprecate optional second argument to WeakMap.prototype.get

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1127827

People

(Reporter: jorendorff, Assigned: Swatinem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

TC39, in its wisdom, decided against `weakmap.get(key, fallbackValue)`.

So we're really not in compliance as long as we support it, and we should drop it.
Blocks: es6
Is there an easy way to figure out what code already uses this parameter?

I’m talking about WeakMap users in firefox chrome code itself.
I see two different options:
1. create a build that doesn't only remove the second parameter, but causes an error to be thrown if it is passed. Do a full try server run and see what turns up
3. manually inspect all 132 usages of WeakMap currently in mozilla-central[1]

Unfortunately, both of these won't catch usages in addons. There is a mxr for addons, but it is almost always nearly entirely useless for stuff like this, because it only returns up to 1000 hits, and those are spent almost entirely on the same hits in the addons SDK over and over again. I don't know of a way to filter those out.


[1]: http://mxr.mozilla.org/mozilla-central/search?string=new+WeakMap%28%29&case=on&tree=mozilla-central
1. sounds reasonable. Hopefully all firefox js should go through some kind of unit tests, so that should catch it.

Otherwise, since this is undocumented, and an experimental unfinished API, extensions should kind of expect it to break, no?
(In reply to Arpad Borsos (Swatinem) from comment #3)
> Otherwise, since this is undocumented, and an experimental unfinished API,
> extensions should kind of expect it to break, no?

Yes, they should. Whether they do is a different matter altogether. The fact that it's undocumented makes me pretty hopeful, though, and also expect that a search through mozilla-central will turn up empty.
Attached patch weakmapget.patchSplinter Review
Im trying to push a version with an assert to try, but try is really slow right now.
So lets see tomorrow if some of the tests hits that assertion.
Assignee: nobody → arpad.borsos
Status: NEW → ASSIGNED
Attachment #8421821 - Flags: review?(jorendorff)
JS_ASSERT(args.length() == 1);

https://tbpl.mozilla.org/?tree=Try&rev=525bc83e932c

Lets see if any of the tests hit this assertion.
(In reply to Arpad Borsos (Swatinem) from comment #6)
> Lets see if any of the tests hit this assertion.

I think we can count that as a "yes". ;)

I also just realized that an assert isn't too useful: it stops the entire test suite, and it doesn't even give us the info we need to fix the call site (because it doesn't give us the call site). At least not without in-depth analysis of the test case.

Instead, you could test for `args.length() > 1` and dump the JS stack in that case. Grepping through the logs would then give us the information we need.
https://tbpl.mozilla.org/?tree=Try&rev=a8236264ca13
Ok, that should be clearer. Still need to grep through all the logs however.
Depends on: 1010568
https://tbpl.mozilla.org/?tree=Try&rev=c490b8b58561
With the one case that was all over the logs fixed. I also suspect that I based my last try push on a broken inbound rev, so hopefully this will break less, haha.
Depends on: 1011854
Depends on: 1011856
Comment on attachment 8421821 [details] [diff] [review]
weakmapget.patch

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

That is a correct patch, if our plan is just to rip it out without warning about it. It might be better to get some data on who's using it in addon-land. If a bunch of our stuff broke... :-\
Attachment #8421821 - Flags: review?(jorendorff)
Comment on attachment 8421821 [details] [diff] [review]
weakmapget.patch

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

r=me, warily
Attachment #8421821 - Flags: review+
There are still some uses in firefox itself, haven’t come around to filing bugs+patches for those yet.
Keywords: addon-compat
Some more callsites that are in the logs:
I’m currently on something else, so if someone wants to take this on:

modules/commonjs/sdk/page-worker.js:43
modules/commonjs/sdk/ui/state.js:88
modules/commonjs/sdk/io/buffer.js:143
modules/commonjs/sdk/io/buffer.js:147
browser/content/devtools/webaudioeditor-view.js:135
modules/accessibility/ContentControl.jsm:327
Whiteboard: [DocArea=JS]
WeakMap implementation was updated to conform to the latest spec draft in bug 1127827. Resolving as duplicate.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1127827
Keywords and docs in bug 1127827 now, too.
Whiteboard: [DocArea=JS]
You need to log in before you can comment on or make changes to this bug.