| Summary: | [Streams API] Remove ReadableStreamController.enqueue() custom binding | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||
| Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | calvaris, commit-queue, darin | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
youenn fablet
2015-06-30 04:32:52 PDT
Created attachment 255824 [details]
Patch
Comment on attachment 255824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255824&action=review > Source/WebCore/bindings/js/ReadableJSStream.cpp:392 > + throwVMError(&state, error()); I thought initially about doing "ec = RangeError" here. But the same exception must be thrown and stored in the stream. I still see some value in introducing "RangeError" for other binding code. > Source/WebCore/bindings/js/ReadableJSStream.h:66 > void error(JSC::ExecState&, ExceptionCode&); I wonder whether we should rename controller methods (enqueue, error and close), in particular error() which may be misleading with m_error getter. Created attachment 256008 [details]
Using Default in IDL
Comment on attachment 256008 [details] Using Default in IDL View in context: https://bugs.webkit.org/attachment.cgi?id=256008&action=review > Source/WebCore/bindings/js/ReadableJSStream.cpp:344 > + throwVMError(&state, error()); This is messy. Having the same function sometimes set exceptions directly on ExecState and other times indicate the exception with an ExceptionCode is a bad idea. > Source/WebCore/dom/ExceptionCode.h:68 > + RangeError = 106, If you add this new type, I think you need to add handling for it in a few places. Comment on attachment 256008 [details] Using Default in IDL View in context: https://bugs.webkit.org/attachment.cgi?id=256008&action=review >> Source/WebCore/bindings/js/ReadableJSStream.cpp:344 >> + throwVMError(&state, error()); > > This is messy. Having the same function sometimes set exceptions directly on ExecState and other times indicate the exception with an ExceptionCode is a bad idea. OK >> Source/WebCore/dom/ExceptionCode.h:68 >> + RangeError = 106, > > If you add this new type, I think you need to add handling for it in a few places. I plan to reuse it in ReadableJSStream::create(). I will also add it in some binding places where the range message is not very meaningful. Created attachment 256021 [details]
Patch for landing
Comment on attachment 256021 [details] Patch for landing Clearing flags on attachment: 256021 Committed r186231: <http://trac.webkit.org/changeset/186231> All reviewed patches have been landed. Closing bug. |