Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversion to sequence<T> need not throw on a RegExp object #145

Closed
rniwa opened this issue Aug 13, 2016 · 2 comments
Closed

Conversion to sequence<T> need not throw on a RegExp object #145

rniwa opened this issue Aug 13, 2016 · 2 comments
Labels
☕☕ difficulty:medium Hard to fix ⌛ duration:short Should be a short fix

Comments

@rniwa
Copy link

rniwa commented Aug 13, 2016

When an ECMAScript value V is converted to an IDL sequence value, we don't really need to throw a TypeError when V is a nativeRegExp object.

Ordinarily, a RegExp object wouldn't have Symbol.iterator defined. If it's defined, e.g. x = new RegExp; x[Symbol.iterator] = function* () { yield 1; }, then we should be able to iterate over it.

@domenic
Copy link
Member

domenic commented Aug 15, 2016

In general I find the way the Web IDL spec treats RegExp and Date objects specially rather strange... Maybe there is historical context I am missing.

kisg pushed a commit to paul99/webkit-mips that referenced this issue Aug 16, 2016
https://bugs.webkit.org/show_bug.cgi?id=160801

Reviewed by Darin Adler.

Source/JavaScriptCore:

Export functions used to iterate over iterable objects.

* runtime/IteratorOperations.h:
(JSC::forEachInIterable):

Source/WebCore:

Added the proper iterator support for sequence<T> with one caveat that we don't check for RegExp object
per whatwg/webidl#145.

See http://heycam.github.io/webidl/#es-sequence and http://heycam.github.io/webidl/#es-overloads

Tests: bindings/scripts/test/TestOverloadedConstructorsWithSequence.idl

* bindings/js/JSDOMBinding.cpp:
(WebCore::hasIteratorMethod): Added. A helper function for checking whether a JSValue is iterable or not.
* bindings/js/JSDOMBinding.h:
(WebCore::NativeValueTraits<unsigned>::nativeValue): Removed the check for isNumber to match the spec'ed
behavior at http://heycam.github.io/webidl/#es-unsigned-long which calls ToNumber first without checking
whether the value is a number or not.
(WebCore::toRefPtrNativeArray): Replaced isJSArray check by isObject check and throw a TypeError. Deployed
forEachInIterable to support non-JSArray iterable objects. Also removed the function pointer from the third
argument since we were always calling JSCT::toWrapped.
(WebCore::toNativeArray): Ditto.
* bindings/js/JSDOMConvert.h:
(WebCore::Converter<Vector<T>>::convert): Removed the comment about toNativeArray not throwing when value
is not an object.
* bindings/scripts/CodeGeneratorJS.pm:
(GenerateOverloadedFunctionOrConstructor): Removed the check for isJSArray for sequence<T> as an iterable
object is not necessary a JSArray.
(WillConvertUndefinedToDefaultParameterValue): Don't return 1 for all sequences since toNativeArray and
toRefPtrNativeArray now throws on undefined due to isObject check.
(JSValueToNative): Removed the third argument from toRefPtrNativeArray.

* bindings/scripts/test/GObject/WebKitDOMTestOverloadedConstructorsWithSequence.cpp: Added.
* bindings/scripts/test/GObject/WebKitDOMTestOverloadedConstructorsWithSequence.h: Added.
* bindings/scripts/test/GObject/WebKitDOMTestOverloadedConstructorsWithSequencePrivate.h: Added.
* bindings/scripts/test/JS/JSTestObj.cpp:
(WebCore::jsTestObjPrototypeFunctionMethodWithOptionalSequenceIsEmpty):
* bindings/scripts/test/JS/JSTestOverloadedConstructorsWithSequence.cpp: Added.
* bindings/scripts/test/JS/JSTestOverloadedConstructorsWithSequence.h: Added.
* bindings/scripts/test/JS/JSTestTypedefs.cpp:
(WebCore::jsTestTypedefsPrototypeFunctionFunc):
* bindings/scripts/test/TestOverloadedConstructorsWithSequence.idl: Added.

LayoutTests:

Added test cases for converting non-JSArray objects to sequence<T> for MutationObserver, FontFaceSet, and WebSocket.

* fast/dom/MutationObserver/observe-exceptions-expected.txt:
* fast/dom/MutationObserver/observe-exceptions.html:
* fast/text/font-face-set-javascript-expected.txt:
* fast/text/font-face-set-javascript.html:
* http/tests/dom/window-open-about-webkit-org-and-access-document-expected.txt: Rebaselined due to js-test-pre.js change.
* http/tests/resources/js-test-pre.js: Merged ToT from resources/js-test-pre.js.
* http/tests/security/xssAuditor/block-does-not-leak-location-expected.txt: Rebaselined due to js-test-pre.js change.
* http/tests/security/xssAuditor/block-does-not-leak-referrer-expected.txt: Ditto.
* http/tests/websocket/tests/hybi/websocket-constructor-protocols-expected.txt: Added.
* http/tests/websocket/tests/hybi/websocket-constructor-protocols.html: Added.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@204500 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@bzbarsky
Copy link
Collaborator

The historical context is twofold:

  1. The intent was to make Date and RegExp distinguishable from dictionaries and sequences during overload resolution.
  2. At one point the intent was that adding an overload should generally not change which overload got invoked for previously-accepted values. That meant that an overload accepting dictionary or sequence should not accept RegExp or Date. The idea was to avoid forward-compat bugs in APIs where behavior would change for existing callers when a new overload was added.

I think we've already deviated that principle in item 2 when we made dictionary and sequence distinguishable while still allowing iterables to represent dictionaries. And I, personally, don't think it's a big deal to drop that principle entirely.

That said, for the specific case of RegExp, I think removing it entirely as #148 suggests is the right way to go.

@tobie tobie added ⌛ duration:short Should be a short fix ☕☕ difficulty:medium Hard to fix labels Sep 5, 2016
tobie added a commit to tobie/webidl that referenced this issue Nov 3, 2016
tobie added a commit to tobie/webidl that referenced this issue Nov 3, 2016
@tobie tobie closed this as completed in #220 Nov 3, 2016
tobie added a commit that referenced this issue Nov 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☕☕ difficulty:medium Hard to fix ⌛ duration:short Should be a short fix
Development

No branches or pull requests

4 participants