Bug 146643 – [Streams API] Remove ReadableStreamReader.read() custom binding

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

We should remove the custom binding and migrate read() to using DOMPromise dedicated to iterators.

[youenn fablet]

Created attachment 256222 [details]
Patch

[WebKit Commit Bot]

Attachment 256222 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:117:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 256222 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:413
> +inline JSC::JSValue toJSIterator(JSC::ExecState* state, JSDOMGlobalObject*, JSC::JSValue value)
> +{
> +    return createIteratorResultObject(state, value, false);
> +}
> +
> +template<typename T> inline JSC::JSValue toJSIterator(JSC::ExecState* state, JSDOMGlobalObject* globalObject, const T& value)
> +{
> +    return createIteratorResultObject(state, toJS(state, globalObject, value), false);
> +}
> +
> +inline JSC::JSValue toJSIteratorEnd(JSC::ExecState* state)
> +{
> +    return createIteratorResultObject(state, JSC::jsUndefined(), true);
> +}

I suggest having all these new functions take references rather than pointers.

> Source/WebCore/bindings/js/JSDOMPromise.h:49
> +    JSDOMGlobalObject* globalObject() const { return m_globalObject.get(); } 

If this can never be null, then I suggest returning a reference, not a pointer.

> Source/WebCore/bindings/js/JSDOMPromise.h:117
> +    DOMPromiseIteratorWithCallback(DeferredWrapper&& wrapper) : m_wrapper(WTF::move(wrapper)) { }

Please consider marking this explicit.

> Source/WebCore/bindings/js/JSDOMPromise.h:131
> +    DOMPromiseIteratorWithCallback(DOMPromiseIteratorWithCallback&& promise)
> +        : m_wrapper(WTF::move(promise.m_wrapper))
> +        , m_resolveCallback(WTF::move(promise.m_resolveCallback))
> +        , m_resolveEndCallback(WTF::move(promise.m_resolveEndCallback))
> +        , m_rejectCallback(WTF::move(promise.m_rejectCallback)) { }

This should be generated automatically by the compiler as long as you don’t explicitly define it. Could you try omitting it?

> Source/WebCore/bindings/js/JSDOMPromise.h:283
> +    ASSERT(m_wrapper);

Seems overkill to assert this here, since m_wrapper.value() will also assert it.

> Source/WebCore/bindings/js/JSDOMPromise.h:284
> +    JSDOMGlobalObject* globalObject = m_wrapper.value().globalObject();

Should use a reference here since this can never be null. Also might want to use auto& instead of JSDOMGlobalObject&.

> Source/WebCore/bindings/js/JSDOMPromise.h:295
> +    ASSERT(m_wrapper);

Seems overkill to assert this here, since m_wrapper.value() will also assert it.

> Source/WebCore/bindings/js/JSDOMPromise.h:306
> +    ASSERT(m_wrapper);

Seems overkill to assert this here, since m_wrapper.value() will also assert it.

[youenn fablet]

(In reply to comment #3)
> Comment on attachment 256222 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256222&action=review

Thanks for the review.

> 
> > Source/WebCore/bindings/js/JSDOMBinding.h:413
> > +inline JSC::JSValue toJSIterator(JSC::ExecState* state, JSDOMGlobalObject*, JSC::JSValue value)
> > +{
> > +    return createIteratorResultObject(state, value, false);
> > +}
> > +
> > +template<typename T> inline JSC::JSValue toJSIterator(JSC::ExecState* state, JSDOMGlobalObject* globalObject, const T& value)
> > +{
> > +    return createIteratorResultObject(state, toJS(state, globalObject, value), false);
> > +}
> > +
> > +inline JSC::JSValue toJSIteratorEnd(JSC::ExecState* state)
> > +{
> > +    return createIteratorResultObject(state, JSC::jsUndefined(), true);
> > +}
> 
> I suggest having all these new functions take references rather than
> pointers.

OK. I was not sure of doing that given the other functions in the same file but I will try to stick with this rule.

> > Source/WebCore/bindings/js/JSDOMPromise.h:49
> > +    JSDOMGlobalObject* globalObject() const { return m_globalObject.get(); } 
> 
> If this can never be null, then I suggest returning a reference, not a
> pointer.

It can be null in theory, but in practice, this method should never be called if it is null. I will do the change, adding an ASSERT within the getter.

> 
> > Source/WebCore/bindings/js/JSDOMPromise.h:117
> > +    DOMPromiseIteratorWithCallback(DeferredWrapper&& wrapper) : m_wrapper(WTF::move(wrapper)) { }
> 
> Please consider marking this explicit.

We cannot do that as we use implicit conversion from DeferredWrapper (created in generated binding) to DOMPromiseXXX in DOM classes.

> 
> > Source/WebCore/bindings/js/JSDOMPromise.h:131
> > +    DOMPromiseIteratorWithCallback(DOMPromiseIteratorWithCallback&& promise)
> > +        : m_wrapper(WTF::move(promise.m_wrapper))
> > +        , m_resolveCallback(WTF::move(promise.m_resolveCallback))
> > +        , m_resolveEndCallback(WTF::move(promise.m_resolveEndCallback))
> > +        , m_rejectCallback(WTF::move(promise.m_rejectCallback)) { }
> 
> This should be generated automatically by the compiler as long as you don’t
> explicitly define it. Could you try omitting it?

Tried and it is working. I will do the same for DOMPromiseWithCallback.

> > Source/WebCore/bindings/js/JSDOMPromise.h:283
> > +    ASSERT(m_wrapper);
> 
> Seems overkill to assert this here, since m_wrapper.value() will also assert
> it.

OK

[youenn fablet]

Created attachment 256283 [details]
Patch for landing

[youenn fablet]

*** Bug 146405 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Comment on attachment 256283 [details]
Patch for landing

Clearing flags on attachment: 256283

Committed r186414: <http://trac.webkit.org/changeset/186414>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.