André Bargull [:anba] Reporter
	Description • 10 years ago

Same issue as in bug 726223, only this time for WeakMaps.

Simple test case:
---
key = {};
wm = new WeakMap([[key, 123]]);
value = wm.get(key);
assertEq(value, 123);
---

	Tooru Fujisawa [:arai] Assignee
	Comment 1 • 10 years ago

Implemented ES6 23.3.1.1 steps 5-7 and 11-12, except `null` in step 6,
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-weakmap-iterable

Also, added almost same testcases as Map-constructor-*.
There are so much differences between them, so I added separetely.

Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7efe84f31791


For steps 2-4 and 8-9, I leave it to bug 1062075 and bug 804279,
and `null` in step 6 to bug 1092538.
(Both could cause compatibility problem, more likely than this patch)

Then, in step 7a, it gets "set" property of this-value, to call it while iterating,
it means that if we alter `WeakMap.prototype.set` to another function,
WeakMap constructor should call it, right? (what jit-test/tests/collections/WeakMap-constructor-set.js tests)
Similar thing is noted in bug 804279, for generic.
If this is true for non-generic call, other collections's constructor should also be fixed.
If not, i'll remove that part from this patch, and leave it to bug 804279.

	Tooru Fujisawa [:arai] Assignee
	Comment 2 • 10 years ago

Sorry, I overlooked second example in bug 804279 comment #0.
It's just what I asked about step 7a in comment #1.

	Tom S [:evilpie]
	Comment 3 • 10 years ago

I would propose that we should just implement this like all the other Map/Set/WeakSet constructors right now and totally ignore calling any method instead of just doing JS::SetWeakMapEntry. I don't see any fault however with handling null/undefined correctly from the start.

	Tooru Fujisawa [:arai] Assignee
	Comment 4 • 10 years ago

Okay, thank you for suggestion.
I'll remove step7a things (and any other differences between other containers) for now.
Then try to fix bug 1092538 as a next step.

	Tooru Fujisawa [:arai] Assignee
	Comment 5 • 10 years ago

Comment on attachment 8516452 [details] [diff] [review]
Handle optional iterable argument in WeakMap constructor.

Clearing r? for now.
I'll post updated patch shortly.

	Tooru Fujisawa [:arai] Assignee
	Comment 6 • 10 years ago

Removed step7a, will be fixed as bug 804279.
Also, use SetWeakMapEntry instead of SetWeakMapEntryInternal as WeakSet does.

Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f1fdfb5b74c0

	Tom S [:evilpie]
	Comment 7 • 10 years ago

Comment on attachment 8521385 [details] [diff] [review]
Handle optional iterable argument in WeakMap constructor.

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

Looks really good.

::: js/src/builtin/MapObject.cpp
@@ +1250,5 @@
>              if (done)
>                  break;
>              if (!pairVal.isObject()) {
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr,
> +                                     JSMSG_INVALID_MAP_ITERABLE, "map");

Map

::: js/src/jsweakmap.cpp
@@ +548,5 @@
> +
> +            // Step 12f.
> +            if (!pairVal.isObject()) {
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr,
> +                                     JSMSG_INVALID_MAP_ITERABLE, "weakmap");

WeakMap

	Tom S [:evilpie]
	Comment 8 • 10 years ago

Thanks arai, nice work! I landed this for you, here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dae8ccc52200

	Tooru Fujisawa [:arai] Assignee
	Comment 9 • 10 years ago

Thank you!

	Carsten Book [:Tomcat]
	Comment 10 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/dae8ccc52200

	Tooru Fujisawa [:arai] Assignee
	Comment 11 • 10 years ago

Updated following documents:
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla
  https://developer.mozilla.org/en-US/Firefox/Releases/36