Bug 146458

Summary: [Streams API] Remove ReadableStream and Reader cancel() custom binding
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Using Default in IDL
none
Patch for landing none

Description youenn fablet 2015-06-30 09:40:43 PDT
We should remove as much custom binding as possible
Comment 1 youenn fablet 2015-07-01 01:56:49 PDT
Created attachment 255911 [details]
Patch
Comment 2 youenn fablet 2015-07-02 06:44:51 PDT
Created attachment 256007 [details]
Using Default in IDL
Comment 3 youenn fablet 2015-07-02 10:47:32 PDT
Comment on attachment 256007 [details]
Using Default in IDL

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3246
> +            push @optionalCallbackArguments, GenerateReturnParameters($function);

With the use of Default=Undefined, the binding generator changes are not needed anymore to support cancel custom removal.
But they are still useful in theory, hence why I kept it in this patch.
I can remove these changes if needed or add a specific binding expectation test for it.
Comment 4 Darin Adler 2015-07-02 15:05:26 PDT
Comment on attachment 256007 [details]
Using Default in IDL

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

> Source/WebCore/Modules/streams/ReadableStream.idl:35
> +    [RaisesException] Promise cancel([Default=Undefined] optional any reason);

What default do we get if we don’t specify [Default=Undefined]? JavaScript null? C++ nullptr?

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3246
>> +            push @optionalCallbackArguments, GenerateReturnParameters($function);
> 
> With the use of Default=Undefined, the binding generator changes are not needed anymore to support cancel custom removal.
> But they are still useful in theory, hence why I kept it in this patch.
> I can remove these changes if needed or add a specific binding expectation test for it.

Definitely should add a bindings test for this if you are keeping it.
Comment 5 youenn fablet 2015-07-02 23:30:43 PDT
Comment on attachment 256007 [details]
Using Default in IDL

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

>> Source/WebCore/Modules/streams/ReadableStream.idl:35
>> +    [RaisesException] Promise cancel([Default=Undefined] optional any reason);
> 
> What default do we get if we don’t specify [Default=Undefined]? JavaScript null? C++ nullptr?

If no default is specified, the DOM class must provide methods with and without the argument, meaning in that case the two following methods:
ReadableStream::cancel(JSPromiseDeferred*, ExceptionCode&);
ReadableStream::cancel(JSC::JSValue, JSPromiseDeferred*, ExceptionCode&);

>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3246
>>> +            push @optionalCallbackArguments, GenerateReturnParameters($function);
>> 
>> With the use of Default=Undefined, the binding generator changes are not needed anymore to support cancel custom removal.
>> But they are still useful in theory, hence why I kept it in this patch.
>> I can remove these changes if needed or add a specific binding expectation test for it.
> 
> Definitely should add a bindings test for this if you are keeping it.

OK
Comment 6 youenn fablet 2015-07-02 23:31:46 PDT
Created attachment 256082 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2015-07-03 00:29:08 PDT
Comment on attachment 256082 [details]
Patch for landing

Clearing flags on attachment: 256082

Committed r186257: <http://trac.webkit.org/changeset/186257>
Comment 8 WebKit Commit Bot 2015-07-03 00:29:11 PDT
All reviewed patches have been landed.  Closing bug.