Bug 146547 – [Streams API] Remove ReadableStream custom constructor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

We should remove ReaableStream custom constructor

[youenn fablet]

Created attachment 256009 [details]
Patch

[youenn fablet]

Created attachment 256087 [details]
Patch

[Darin Adler]

Comment on attachment 256087 [details]
Patch

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

> Source/WebCore/Modules/streams/ReadableStream.h:68
> +    static RefPtr<ReadableStream> create(JSC::ExecState&, JSC::JSValue, const Dictionary&);

Why does this return RefPtr instead of Ref? Can it return null?

> Source/WebCore/Modules/streams/ReadableStream.idl:32
> +    Constructor([Default=Undefined] optional any source, [Default=Undefined] optional Dictionary strategy),

Seems a bit wordy. Do we really need “Default=Undefined”? What is the default if it’s not undefined?

> Source/WebCore/bindings/js/ReadableJSStream.cpp:210
> +        throwVMError(&state, createDOMException(&state, TypeError));

Should probably be using throwVMTypeError, which should be overloaded as needed if it’s not already. And should have custom message, I think, like all the many other uses of throwVMTypeErrror.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:220
> +                throwVMError(&state, createDOMException(&state, TypeError));

Should be using throwVMTypeError, as above. Also, should this check for infinity as well, or only NaN?

> Source/WebCore/bindings/js/ReadableJSStream.cpp:224
> +                throwVMError(&state, createDOMException(&state, RangeError));

Should make a helper function throwVMRangeError and use it.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:233
> +                throwVMError(&state, createDOMException(&state, TypeError));

Should be using throwVMTypeError, as above.

> Source/WebCore/bindings/js/ReadableJSStream.h:55
>  class ReadableJSStream: public ReadableStream {

Missing space before colon. Also, I think this class should be marked "final".

> Source/WebCore/bindings/js/ReadableJSStream.h:57
> +    static RefPtr<ReadableStream> create(JSC::ExecState&, JSC::JSValue, const Dictionary&);

The return value should remain specific, ReadableJSStream rather than ReadableStream. The other call site can and should convert from one RefPtr to the other. If you’re worried about the exact code generated we could make sure we have the right kind of move constructor to make that work efficiently with no reference count manipulation.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4692
> +            if ($codeGenerator->ExtendedAttributeContains($interface->extendedAttributes->{"ConstructorCallWith"}, "ScriptState")) {
> +                 push(@$outputArray, "    if (UNLIKELY(exec->hadException()))\n");
> +                 push(@$outputArray, "        return JSValue::encode(jsUndefined());\n");
> +            }

I am not sure it’s exactly right that “call with script state” automatically also means “raises exception”.

[youenn fablet]

Comment on attachment 256087 [details]
Patch

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

>> Source/WebCore/Modules/streams/ReadableStream.h:68
>> +    static RefPtr<ReadableStream> create(JSC::ExecState&, JSC::JSValue, const Dictionary&);
> 
> Why does this return RefPtr instead of Ref? Can it return null?

It can return null in case an exception is thrown.

>> Source/WebCore/Modules/streams/ReadableStream.idl:32
>> +    Constructor([Default=Undefined] optional any source, [Default=Undefined] optional Dictionary strategy),
> 
> Seems a bit wordy. Do we really need “Default=Undefined”? What is the default if it’s not undefined?

We need to set the parameters as optional according the spec.

If not setting "Default=" explicitly, the binding generator should probably expect the DOM class to provide different methods.
But "optional without default" parameters are apparently not supported yet by the binding generator.

In our case, it is anyway more convenient to use "Default=..." so that the DOM class has just one constructor.

>> Source/WebCore/bindings/js/ReadableJSStream.cpp:210
>> +        throwVMError(&state, createDOMException(&state, TypeError));
> 
> Should probably be using throwVMTypeError, which should be overloaded as needed if it’s not already. And should have custom message, I think, like all the many other uses of throwVMTypeErrror.

OK

>> Source/WebCore/bindings/js/ReadableJSStream.h:55
>>  class ReadableJSStream: public ReadableStream {
> 
> Missing space before colon. Also, I think this class should be marked "final".

OK

>> Source/WebCore/bindings/js/ReadableJSStream.h:57
>> +    static RefPtr<ReadableStream> create(JSC::ExecState&, JSC::JSValue, const Dictionary&);
> 
> The return value should remain specific, ReadableJSStream rather than ReadableStream. The other call site can and should convert from one RefPtr to the other. If you’re worried about the exact code generated we could make sure we have the right kind of move constructor to make that work efficiently with no reference count manipulation.

OK.
I think to recall there is already a move constructor that could do that.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4692
>> +            }
> 
> I am not sure it’s exactly right that “call with script state” automatically also means “raises exception”.

The same check is done for methods called with ScriptState.
A specific keyword might be used to control this but I am not sure this is actually worth it.

[youenn fablet]

Created attachment 256198 [details]
Patch for landing

[youenn fablet]

Created attachment 256199 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 256199 [details]
Patch for landing

Clearing flags on attachment: 256199

Committed r186323: <http://trac.webkit.org/changeset/186323>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.