Implement high water mark as defined in https://streams.spec.whatwg.org/#rs-constructor and https://streams.spec.whatwg.org/#validate-and-normalize-queuing-strategy
Created attachment 255406 [details] Patch
Comment on attachment 255406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255406&action=review > Source/WebCore/bindings/js/ReadableJSStream.cpp:201 > +static inline double getHighWaterMark(ExecState& exec, JSObject* strategy) WebKIt coding style does not involve the word "get" for a function with a return value like this. The strategy argument should be a JSObject&. > Source/WebCore/bindings/js/ReadableJSStream.cpp:205 > + if (!jsHighWaterMark) > + return 1; This dead code should be removed. The get function can never return an “empty” JSValue like this. I don’t even think we need this local variable. However, we should return here if we had an exception before running toNumber. > Source/WebCore/bindings/js/ReadableJSStream.cpp:207 > + double highWaterMark = jsHighWaterMark.toNumber(&exec); IF we get an exception from toNumber we should return that exception, not the "highWaterMark parameter is NaN" one. > Source/WebCore/bindings/js/ReadableJSStream.cpp:209 > + throwVMError(&exec, createTypeError(&exec, ASCIILiteral("highWaterMark parameter is NaN"))); Isn’t there a more efficient way to do this perhaps with a single function call? It seems overkill here to have a custom strong that includes the argument name. Can we make this more consistent with other bindings? > Source/WebCore/bindings/js/ReadableJSStream.cpp:213 > + throwVMError(&exec, createRangeError(&exec, ASCIILiteral("highWaterMark parameter is not a positive value"))); Same thought here. > Source/WebCore/bindings/js/ReadableJSStream.cpp:219 > RefPtr<ReadableJSStream> ReadableJSStream::create(ExecState& exec, ScriptExecutionContext& scriptExecutionContext) It’s a bad pattern to do so much bindings work here in the DOM class. I wonder if there is some way to improve the factoring. Sam Weinig might have some comments about that. > Source/WebCore/bindings/js/ReadableJSStream.cpp:236 > + highWaterMark = getHighWaterMark(exec, strategyObject); Need to return after this if there is an exception before calling getPropertyFromObject below because side effects can be observed. > Source/WebCore/bindings/js/ReadableJSStream.cpp:251 > + RefPtr<ReadableJSStream> readableStream = adoptRef(*new ReadableJSStream(scriptExecutionContext, exec, jsSource, highWaterMark, sizeFunction)); Is it correct to pass an empty JSValue for sizeFunction when there is no function? How about passing "undefined" instead? Also we should change the type to be JSFunction* (or something like that) since we are doing the type checking here outside the class. > Source/WebCore/bindings/js/ReadableJSStream.h:88 > + JSC::Strong<JSC::Unknown> m_sizeFunction; I’m concerned that this could result in a reference cycle, if the size function has captured a reference to the stream. I guess I should have the same worry about m_errorFunction. Generally speaking I think we need to use GC marking rather than strong references in this class to avoid cycles and leaks.
Comment on attachment 255406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255406&action=review >> Source/WebCore/bindings/js/ReadableJSStream.cpp:207 >> + double highWaterMark = jsHighWaterMark.toNumber(&exec); > > IF we get an exception from toNumber we should return that exception, not the "highWaterMark parameter is NaN" one. To be clearer, I mean we should return so that exception is not overwritten bythe "highWaterMark parameter is NaN" one or any other.
Comment on attachment 255406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255406&action=review >> Source/WebCore/bindings/js/ReadableJSStream.cpp:201 >> +static inline double getHighWaterMark(ExecState& exec, JSObject* strategy) > > WebKIt coding style does not involve the word "get" for a function with a return value like this. > > The strategy argument should be a JSObject&. ok >> Source/WebCore/bindings/js/ReadableJSStream.cpp:205 >> + return 1; > > This dead code should be removed. The get function can never return an “empty” JSValue like this. I don’t even think we need this local variable. Good catch, we shoud check whether jsHighWaterMark is undefined or not. > However, we should return here if we had an exception before running toNumber. OK >>> Source/WebCore/bindings/js/ReadableJSStream.cpp:207 >>> + double highWaterMark = jsHighWaterMark.toNumber(&exec); >> >> IF we get an exception from toNumber we should return that exception, not the "highWaterMark parameter is NaN" one. > > To be clearer, I mean we should return so that exception is not overwritten bythe "highWaterMark parameter is NaN" one or any other. Right. >> Source/WebCore/bindings/js/ReadableJSStream.cpp:209 >> + throwVMError(&exec, createTypeError(&exec, ASCIILiteral("highWaterMark parameter is NaN"))); > > Isn’t there a more efficient way to do this perhaps with a single function call? It seems overkill here to have a custom strong that includes the argument name. Can we make this more consistent with other bindings? I am not sure to totally understand the point here. We can remove the ASCIILiteral string, it is not mandatory, that what is doing setDOMException(). Or we could set it to "Value is NaN". Is it what is suggested? >> Source/WebCore/bindings/js/ReadableJSStream.cpp:219 >> RefPtr<ReadableJSStream> ReadableJSStream::create(ExecState& exec, ScriptExecutionContext& scriptExecutionContext) > > It’s a bad pattern to do so much bindings work here in the DOM class. I wonder if there is some way to improve the factoring. Sam Weinig might have some comments about that. We could slightly reduce the binding by making the constructor not custom. But we need anyway an ExecState, so we would use the globalExec retrieved from the source object. I'll have some thoughts about it later. >> Source/WebCore/bindings/js/ReadableJSStream.cpp:236 >> + highWaterMark = getHighWaterMark(exec, strategyObject); > > Need to return after this if there is an exception before calling getPropertyFromObject below because side effects can be observed. ok >> Source/WebCore/bindings/js/ReadableJSStream.cpp:251 >> + RefPtr<ReadableJSStream> readableStream = adoptRef(*new ReadableJSStream(scriptExecutionContext, exec, jsSource, highWaterMark, sizeFunction)); > > Is it correct to pass an empty JSValue for sizeFunction when there is no function? How about passing "undefined" instead? Also we should change the type to be JSFunction* (or something like that) since we are doing the type checking here outside the class. It is covered by tests, but this is not recommended I guess. Let's change that to a JSFunction*/nullptr >> Source/WebCore/bindings/js/ReadableJSStream.h:88 >> + JSC::Strong<JSC::Unknown> m_sizeFunction; > > I’m concerned that this could result in a reference cycle, if the size function has captured a reference to the stream. I guess I should have the same worry about m_errorFunction. Generally speaking I think we need to use GC marking rather than strong references in this class to avoid cycles and leaks. I share the same concerns. Let's do that as a follow up patch, which may also encompass source and maybe pull fulfill/reject promise callbacks. I filed bug 146278 for that purpose.
Comment on attachment 255406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255406&action=review >>> Source/WebCore/bindings/js/ReadableJSStream.cpp:201 >>> +static inline double getHighWaterMark(ExecState& exec, JSObject* strategy) >> >> WebKIt coding style does not involve the word "get" for a function with a return value like this. >> >> The strategy argument should be a JSObject&. Done >>> Source/WebCore/bindings/js/ReadableJSStream.cpp:205 >>> + return 1; >> >> This dead code should be removed. The get function can never return an “empty” JSValue like this. I don’t even think we need this local variable. >> >> However, we should return here if we had an exception before running toNumber. > > Good catch, we shoud check whether jsHighWaterMark is undefined or not. Done >>>> Source/WebCore/bindings/js/ReadableJSStream.cpp:207 >>>> + double highWaterMark = jsHighWaterMark.toNumber(&exec); >>> >>> IF we get an exception from toNumber we should return that exception, not the "highWaterMark parameter is NaN" one. >> >> To be clearer, I mean we should return so that exception is not overwritten bythe "highWaterMark parameter is NaN" one or any other. Done >>> Source/WebCore/bindings/js/ReadableJSStream.cpp:209 >>> + throwVMError(&exec, createTypeError(&exec, ASCIILiteral("highWaterMark parameter is NaN"))); >> >> Isn’t there a more efficient way to do this perhaps with a single function call? It seems overkill here to have a custom strong that includes the argument name. Can we make this more consistent with other bindings? Changed to "Value is NaN" >> Source/WebCore/bindings/js/ReadableJSStream.cpp:213 >> + throwVMError(&exec, createRangeError(&exec, ASCIILiteral("highWaterMark parameter is not a positive value"))); >> >> Same thought here. Changed to "Not a positive value" >>> Source/WebCore/bindings/js/ReadableJSStream.cpp:219 >>> RefPtr<ReadableJSStream> ReadableJSStream::create(ExecState& exec, ScriptExecutionContext& scriptExecutionContext) >> >> It’s a bad pattern to do so much bindings work here in the DOM class. I wonder if there is some way to improve the factoring. Sam Weinig might have some comments about that. Added a FIXME >>> Source/WebCore/bindings/js/ReadableJSStream.cpp:236 >>> + highWaterMark = getHighWaterMark(exec, strategyObject); >> >> Need to return after this if there is an exception before calling getPropertyFromObject below because side effects can be observed. Done >>> Source/WebCore/bindings/js/ReadableJSStream.cpp:251 >>> + RefPtr<ReadableJSStream> readableStream = adoptRef(*new ReadableJSStream(scriptExecutionContext, exec, jsSource, highWaterMark, sizeFunction)); >> >> Is it correct to pass an empty JSValue for sizeFunction when there is no function? How about passing "undefined" instead? Also we should change the type to be JSFunction* (or something like that) since we are doing the type checking here outside the class. Done >>> Source/WebCore/bindings/js/ReadableJSStream.h:88 >>> + JSC::Strong<JSC::Unknown> m_sizeFunction; >> >> I’m concerned that this could result in a reference cycle, if the size function has captured a reference to the stream. I guess I should have the same worry about m_errorFunction. Generally speaking I think we need to use GC marking rather than strong references in this class to avoid cycles and leaks. Added a FIXME referring to the filed bug
Created attachment 255546 [details] Patch for landing
Comment on attachment 255546 [details] Patch for landing Rejecting attachment 255546 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 255546, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.appspot.com/results/4887571318439936
Created attachment 255547 [details] Patch for landing
Comment on attachment 255547 [details] Patch for landing Clearing flags on attachment: 255547 Committed r185953: <http://trac.webkit.org/changeset/185953>
All reviewed patches have been landed. Closing bug.