Bug 142348 – Upgrade Map, Set and WeakMap constructor interface

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Upgrade Map and Set constructor interface

[Yusuke Suzuki]

One concern is that, Set / Map is already shipped in Safari 8 with different interface.

https://bugs.webkit.org/show_bug.cgi?id=141351#c11

[Yusuke Suzuki]

Created attachment 247973 [details]
rev1 prototype

rev1 prototype. Tests not included.

[Yusuke Suzuki]

Created attachment 248069 [details]
Patch

[WebKit Commit Bot]

Attachment 248069 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 248071 [details]
Patch

[Yusuke Suzuki]

OK, patch is ready for review.

Joseph, what do you think of? Any runtime flags are needed? I'd like to hear your opinion for this breaking change from Safari 8.

At least, Map interface doesn't break things because it didn't add any argument into its data.
However, since Set added its argument to its data in the different style from the latest spec, it is breaking change.

[Joseph Pecoraro]

(In reply to comment #6)
> OK, patch is ready for review.
> 
> Joseph, what do you think of? Any runtime flags are needed? I'd like to hear
> your opinion for this breaking change from Safari 8.
> 
> At least, Map interface doesn't break things because it didn't add any
> argument into its data.
> However, since Set added its argument to its data in the different style
> from the latest spec, it is breaking change.

I think we should just land it! We are actually aligning with other browsers with this change (Firefox/Chrome).

For vendors that have shipped WebKit with the older, incompatible Set/Map constructors they may want to mention this in release notes to inform developers ("JavaScript Set/Map constructor behavior change ...") but I don't really think there is anything we should need to do here to workaround the issues of the past.

In the unlikely event that this breaks something significant we can revisit this.

[Yusuke Suzuki]

(In reply to comment #7)
> I think we should just land it! We are actually aligning with other browsers
> with this change (Firefox/Chrome).
> 
> For vendors that have shipped WebKit with the older, incompatible Set/Map
> constructors they may want to mention this in release notes to inform
> developers ("JavaScript Set/Map constructor behavior change ...") but I
> don't really think there is anything we should need to do here to workaround
> the issues of the past.
> 
> In the unlikely event that this breaks something significant we can revisit
> this.

Agreed. Thank you for your comment :D

[Yusuke Suzuki]

I'll resubmit patch with a little fix about IteratorClose(value)

[Yusuke Suzuki]

(In reply to comment #9)
> I'll resubmit patch with a little fix about IteratorClose(value)

Ah, sorry, it's no problem. current patch is correct.

[Yusuke Suzuki]

Comment on attachment 248071 [details]
Patch

Ah, before r?, I'll add WeakMap / WeakSet upgrade too.

[Yusuke Suzuki]

Created attachment 248093 [details]
Patch

[Yusuke Suzuki]

I've upgraded the patch with WeakMap. WeakSet is not implemented in JSC, I've opened issue 142408.

[Yusuke Suzuki]

Created attachment 248094 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 248094 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248094&action=review

Added one note.

> Source/JavaScriptCore/runtime/WeakMapConstructor.cpp:140
> +    callData.native.function = callWeakMap;

One note is that, in Call case, we need to set some function to make `typeof WeakMap === 'function'`.

[Build Bot]

Comment on attachment 248094 [details]
Patch

Attachment 248094 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4757494261874688

New failing tests:
js/dom/basic-weakmap.html

[Build Bot]

Created attachment 248098 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Yusuke Suzuki]

Created attachment 248099 [details]
Patch

[Filip Pizlo]

Comment on attachment 248099 [details]
Patch

R=me.  We'll watch bugreports in nightlies to see if this is the kind of breaking change that actually breaks websites.  I bet not.

[WebKit Commit Bot]

Comment on attachment 248099 [details]
Patch

Clearing flags on attachment: 248099

Committed r181333: <http://trac.webkit.org/changeset/181333>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

(In reply to comment #19)
> Comment on attachment 248099 [details]
> Patch
> 
> R=me.  We'll watch bugreports in nightlies to see if this is the kind of
> breaking change that actually breaks websites.  I bet not.

Landed, thank you :D