Bug 146458 - [Streams API] Remove ReadableStream and Reader cancel() custom binding
Summary: [Streams API] Remove ReadableStream and Reader cancel() custom binding
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-30 09:40 PDT by youenn fablet
Modified: 2015-07-03 00:29 PDT (History)
1 user (show)

See Also:


Attachments
Patch (8.29 KB, patch)
2015-07-01 01:56 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Using Default in IDL (6.49 KB, patch)
2015-07-02 06:44 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (11.04 KB, patch)
2015-07-02 23:31 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.