Bug 146312

Created attachment 255549 [details] Patch

Created attachment 255551 [details] style fix

Comment on attachment 255551 [details] style fix View in context: https://bugs.webkit.org/attachment.cgi?id=255551&action=review > Source/WebCore/bindings/js/ReadableJSStream.cpp:339 > + if (state.hadException()) { > + storeError(state, state.exception()->value()); > + return; > + } If we are turning the exception into a stored error, then I assume we don’t want to also leave it in the execution state. Do we have a test covering this? Seems like this should be doing clearException(). Or we should add a takeException() function that combines fetching the exception with clearing it. > Source/WebCore/bindings/js/ReadableJSStream.cpp:358 > + JSValue sizeValue = callFunction(state, m_sizeFunction.get(), jsUndefined(), arguments); > + > + if (state.hadException()) > + return 0; I’d leave out these blank lines. > Source/WebCore/bindings/js/ReadableJSStream.cpp:363 > + double size = sizeValue.toNumber(&state); > + > + if (state.hadException()) > + return 0; Ditto. > Source/WebCore/bindings/js/ReadableJSStream.cpp:365 > + if (std::isnan(size) || std::isinf(size)) { if (!std::isfinite(size)) { > Source/WebCore/bindings/js/ReadableJSStream.cpp:366 > + throwVMError(&state, createRangeError(&state, ASCIILiteral("Incorrect double value."))); I’m not a big fan of these ad hoc error messages. Why in this case do we use the words “Incorrect double value” with a period? We need to share more code so that each bit of JavaScript binding like this doesn’t end up doing things differently than other bindings performing the same operation. > Source/WebCore/bindings/js/ReadableJSStream.h:82 > + double chunkSize(JSC::ExecState&, JSC::JSValue); Because there are side effects, we might want a verb in the name of this function. It’s not just a simple getter because it calls the size function and possibly runs code in the process of converting the value to a number as well.

Comment on attachment 255551 [details] style fix View in context: https://bugs.webkit.org/attachment.cgi?id=255551&action=review >> Source/WebCore/bindings/js/ReadableJSStream.cpp:339 >> + } > > If we are turning the exception into a stored error, then I assume we don’t want to also leave it in the execution state. Do we have a test covering this? > > Seems like this should be doing clearException(). Or we should add a takeException() function that combines fetching the exception with clearing it. Test coverage is found in LayoutTests/streams/reference-implementation/bad-strategies.html (e.g. "Readable stream: throwing strategy.size method"). If enqueue is throwing within a start call and start JS callback does not catch it, the constructor will throw. This sounds good. If enqueue is throwing within a further pull and pull JS callback does not catch it, this will trigger corresponding promise rejection (like read() for instance). >> Source/WebCore/bindings/js/ReadableJSStream.cpp:358 >> + return 0; > > I’d leave out these blank lines. OK >> Source/WebCore/bindings/js/ReadableJSStream.cpp:363 >> + return 0; > > Ditto. OK >> Source/WebCore/bindings/js/ReadableJSStream.cpp:365 >> + if (std::isnan(size) || std::isinf(size)) { > > if (!std::isfinite(size)) { OK >> Source/WebCore/bindings/js/ReadableJSStream.cpp:366 >> + throwVMError(&state, createRangeError(&state, ASCIILiteral("Incorrect double value."))); > > I’m not a big fan of these ad hoc error messages. Why in this case do we use the words “Incorrect double value” with a period? We need to share more code so that each bit of JavaScript binding like this doesn’t end up doing things differently than other bindings performing the same operation. Will remove the ".". As part of refactoring, I will remove enqueue custom binding once we can use a direct "any->JSC::JSValue" binding rule. At that time, this exception will be transmitted to binding level using an ExceptionCode. >> Source/WebCore/bindings/js/ReadableJSStream.h:82 >> + double chunkSize(JSC::ExecState&, JSC::JSValue); > > Because there are side effects, we might want a verb in the name of this function. It’s not just a simple getter because it calls the size function and possibly runs code in the process of converting the value to a number as well. Let's rename it to retrieveChunkSize then.

Created attachment 255722 [details] Patch for landing

Comment on attachment 255722 [details] Patch for landing Clearing flags on attachment: 255722 Committed r186044: <http://trac.webkit.org/changeset/186044>

All reviewed patches have been landed. Closing bug.