[Streams API] Reading ReadableStream ready and closed attributes should not always...
authoryouenn.fablet@crf.canon.fr <youenn.fablet@crf.canon.fr@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 24 Feb 2015 14:48:34 +0000 (14:48 +0000)
committeryouenn.fablet@crf.canon.fr <youenn.fablet@crf.canon.fr@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 24 Feb 2015 14:48:34 +0000 (14:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=141650

Reviewed by Benjamin Poulain.

Source/WebCore:

The JSPromiseDeferred objects returned by JSReadableStream::ready and JSReadableStream::closed should be stored somewhere.
This patch stores them as private slots in JSReadableStream.
This is close to the description in https://streams.spec.whatwg.org/#rs-internal-slots.

An alternative would be to have these objects as JSReadableStream class members, thus modifying the binding generator.
That may be the cleanest solution, especially if other APIs express the same need.
Another alternative is to store DeferredWrapper in ReadableStream objects.
But it is currently preferred to keep DeferredWrapper in bindings/js code.

Added new constructor for DeferredWrapper to use these stored values.
Updated ready and closed methods to use that new constructor.
Changes are covered by new test in streams/readablestream-constructor.html.

* bindings/js/JSDOMPromise.cpp:
(WebCore::DeferredWrapper::DeferredWrapper):
* bindings/js/JSDOMPromise.h:
* bindings/js/JSReadableStreamCustom.cpp:
(WebCore::closedPromiseSlotName):
(WebCore::getOrCreatePromiseDeferredFromObject):
(WebCore::readyPromiseSlotName):
(WebCore::JSReadableStream::ready):
(WebCore::JSReadableStream::closed):
* bindings/js/ReadableStreamJSSource.cpp:
(WebCore::setInternalSlotToObject):
(WebCore::getInternalSlotFromObject):
* bindings/js/ReadableStreamJSSource.h:

LayoutTests:

* streams/readablestream-constructor-expected.txt:
* streams/readablestream-constructor.html: Added test to ensure ready and closed always return the same promise object.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@180559 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/streams/readablestream-constructor-expected.txt
LayoutTests/streams/readablestream-constructor.html
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSDOMPromise.cpp
Source/WebCore/bindings/js/JSDOMPromise.h
Source/WebCore/bindings/js/JSReadableStreamCustom.cpp
Source/WebCore/bindings/js/ReadableStreamJSSource.cpp
Source/WebCore/bindings/js/ReadableStreamJSSource.h

index 1e5fcfa8a131a0139ab0cc9c14efa20152928fa1..e63deee8d003dc222adc34f11f31346702c96dd2 100644 (file)
@@ -1,3 +1,13 @@
+2015-02-24  Xabier Rodriguez Calvar <calvaris@igalia.com> and Youenn Fablet  <youenn.fablet@crf.canon.fr>
+
+        [Streams API] Reading ReadableStream ready and closed attributes should not always create a new promise
+        https://bugs.webkit.org/show_bug.cgi?id=141650
+
+        Reviewed by Benjamin Poulain.
+
+        * streams/readablestream-constructor-expected.txt:
+        * streams/readablestream-constructor.html: Added test to ensure ready and closed always return the same promise object.
+
 2015-02-24  Dhi Aurrahman  <diorahman@rockybars.com>
 
         Always serialize :lang()'s arguments to strings
index 90cd24e8e6cec23d870654608ef330f84e3dbd9d..653bbe4a955e6c8da67450f9da5a918787bb7f3c 100644 (file)
@@ -3,4 +3,5 @@ PASS ReadableStream constructor should get an object as argument
 PASS ReadableStream instances should have the correct list of properties 
 PASS ReadableStream can be constructed with no arguments 
 PASS ReadableStream instances have the correct methods and properties 
+PASS ReadableStream ready and closed should always return the same promise object 
 
index 52e55b6b2d407002aaebde46da120f3285b4c69a..d393e12fd38e55b10b63b641d191c22beaf60d47 100644 (file)
@@ -58,4 +58,17 @@ test(function() {
     assert_equals(typeof rs.closed.then, 'function', 'closed property is thenable');
 }, 'ReadableStream instances have the correct methods and properties');
 
+test(function() {
+    rs = new ReadableStream();
+    readyPromise = rs.ready;
+    closedPromise = rs.closed;
+
+    assert_equals(readyPromise, rs.ready);
+    assert_equals(closedPromise, rs.closed);
+
+    // We check ReadableStream properties here as ready and closed promises are persistent and need to be stored in some way that is not observable.
+    assert_array_equals(Object.keys(rs), ['closed', 'ready']);
+    assert_array_equals(Object.getOwnPropertyNames(rs), ['closed', 'ready']);
+}, 'ReadableStream ready and closed should always return the same promise object');
+
 </script>
index 5620bb8bb3227ccc77839e3cf22e5cb1b7312ddd..a8cb9ccbca39a165e3ae6ded24650f3a3dda751e 100644 (file)
@@ -1,3 +1,37 @@
+2015-02-24  Xabier Rodriguez Calvar <calvaris@igalia.com> and Youenn Fablet  <youenn.fablet@crf.canon.fr>
+
+        [Streams API] Reading ReadableStream ready and closed attributes should not always create a new promise
+        https://bugs.webkit.org/show_bug.cgi?id=141650
+
+        Reviewed by Benjamin Poulain.
+
+        The JSPromiseDeferred objects returned by JSReadableStream::ready and JSReadableStream::closed should be stored somewhere.
+        This patch stores them as private slots in JSReadableStream.
+        This is close to the description in https://streams.spec.whatwg.org/#rs-internal-slots.
+
+        An alternative would be to have these objects as JSReadableStream class members, thus modifying the binding generator.
+        That may be the cleanest solution, especially if other APIs express the same need.
+        Another alternative is to store DeferredWrapper in ReadableStream objects.
+        But it is currently preferred to keep DeferredWrapper in bindings/js code.
+
+        Added new constructor for DeferredWrapper to use these stored values.
+        Updated ready and closed methods to use that new constructor.
+        Changes are covered by new test in streams/readablestream-constructor.html.
+
+        * bindings/js/JSDOMPromise.cpp:
+        (WebCore::DeferredWrapper::DeferredWrapper):
+        * bindings/js/JSDOMPromise.h:
+        * bindings/js/JSReadableStreamCustom.cpp:
+        (WebCore::closedPromiseSlotName):
+        (WebCore::getOrCreatePromiseDeferredFromObject):
+        (WebCore::readyPromiseSlotName):
+        (WebCore::JSReadableStream::ready):
+        (WebCore::JSReadableStream::closed):
+        * bindings/js/ReadableStreamJSSource.cpp:
+        (WebCore::setInternalSlotToObject):
+        (WebCore::getInternalSlotFromObject):
+        * bindings/js/ReadableStreamJSSource.h:
+
 2015-02-24  Dhi Aurrahman  <diorahman@rockybars.com>
 
         Always serialize :lang()'s arguments to strings
index c3a637fe076fdd048d6e7d5486d29df1aa7957ae..46bddba18f8c85bfcb7fd1e8cc67c577d517df12 100644 (file)
@@ -38,6 +38,12 @@ DeferredWrapper::DeferredWrapper(ExecState* exec, JSDOMGlobalObject* globalObjec
 {
 }
 
+DeferredWrapper::DeferredWrapper(ExecState* exec, JSDOMGlobalObject* globalObject, JSPromiseDeferred* promiseDeferred)
+    : m_globalObject(exec->vm(), globalObject)
+    , m_deferred(exec->vm(), promiseDeferred)
+{
+}
+
 JSObject* DeferredWrapper::promise() const
 {
     return m_deferred->promise();
index 0d656c5927b5048cd61acde3444ea3e842e59f2c..42d02d9f28e9e6a84a4e75c3a7318f9614f9a01c 100644 (file)
@@ -39,6 +39,7 @@ namespace WebCore {
 class DeferredWrapper {
 public:
     DeferredWrapper(JSC::ExecState*, JSDOMGlobalObject*);
+    DeferredWrapper(JSC::ExecState*, JSDOMGlobalObject*, JSC::JSPromiseDeferred*);
 
     template<class ResolveResultType>
     void resolve(const ResolveResultType&);
index 4a05b70f265939de2035dd74a83cbcbc52d85608..0f7993deece88edf5a13f085a6b97eb800c454be 100644 (file)
@@ -36,6 +36,7 @@
 #include "JSDOMPromise.h"
 #include "ReadableStream.h"
 #include "ReadableStreamJSSource.h"
+#include <wtf/NeverDestroyed.h>
 
 using namespace JSC;
 
@@ -47,9 +48,28 @@ JSValue JSReadableStream::read(ExecState* exec)
     return exec->vm().throwException(exec, error);
 }
 
+static JSPromiseDeferred* getOrCreatePromiseDeferredFromObject(ExecState* exec, JSValue thisObject, JSGlobalObject* globalObject, PrivateName &name)
+{
+    JSValue slot = getInternalSlotFromObject(exec, thisObject, name);
+    JSPromiseDeferred* promiseDeferred = slot ? jsDynamicCast<JSPromiseDeferred*>(slot) : nullptr;
+
+    if (!promiseDeferred) {
+        promiseDeferred = JSPromiseDeferred::create(exec, globalObject);
+        setInternalSlotToObject(exec, thisObject, name, promiseDeferred);
+    }
+    return promiseDeferred;
+}
+
+static JSC::PrivateName& readyPromiseSlotName()
+{
+    static NeverDestroyed<JSC::PrivateName> readyPromiseSlotName("readyPromise");
+    return readyPromiseSlotName;
+}
+
 JSValue JSReadableStream::ready(ExecState* exec) const
 {
-    DeferredWrapper wrapper(exec, globalObject());
+    JSPromiseDeferred* promiseDeferred = getOrCreatePromiseDeferredFromObject(exec, this, globalObject(), readyPromiseSlotName());
+    DeferredWrapper wrapper(exec, globalObject(), promiseDeferred);
     auto successCallback = [wrapper](RefPtr<ReadableStream> stream) mutable {
         wrapper.resolve(stream.get());
     };
@@ -58,9 +78,16 @@ JSValue JSReadableStream::ready(ExecState* exec) const
     return wrapper.promise();
 }
 
+static JSC::PrivateName& closedPromiseSlotName()
+{
+    static NeverDestroyed<JSC::PrivateName> closedPromiseSlotName("closedPromise");
+    return closedPromiseSlotName;
+}
+
 JSValue JSReadableStream::closed(ExecState* exec) const
 {
-    DeferredWrapper wrapper(exec, globalObject());
+    JSPromiseDeferred* promiseDeferred = getOrCreatePromiseDeferredFromObject(exec, this, globalObject(), closedPromiseSlotName());
+    DeferredWrapper wrapper(exec, globalObject(), promiseDeferred);
     auto successCallback = [wrapper](RefPtr<ReadableStream> stream) mutable {
         wrapper.resolve(stream.get());
     };
index 9b43457f8dc4b966ba94e7739a87309c5f23ef14..b81aacd5008b574482d9ec163a7a370aed7cd7d7 100644 (file)
 #if ENABLE(STREAMS_API)
 #include "ReadableStreamJSSource.h"
 
+#include "JSDOMPromise.h"
 #include "JSReadableStream.h"
 #include <runtime/Error.h>
 #include <runtime/JSCJSValueInlines.h>
+#include <runtime/JSString.h>
+#include <runtime/StructureInlines.h>
 
 using namespace JSC;
 
 namespace WebCore {
 
+void setInternalSlotToObject(ExecState* exec, JSValue objectValue, PrivateName& name, JSValue value)
+{
+    JSObject* object = objectValue.toObject(exec);
+    PutPropertySlot propertySlot(objectValue);
+    object->put(object, exec, Identifier::from(name), value, propertySlot);
+}
+
+JSValue getInternalSlotFromObject(ExecState* exec, JSValue objectValue, PrivateName& name)
+{
+    JSObject* object = objectValue.toObject(exec);
+    PropertySlot propertySlot(objectValue);
+
+    PropertyName propertyName = Identifier::from(name);
+    if (!object->getOwnPropertySlot(object, exec, propertyName, propertySlot))
+        return JSValue();
+    return propertySlot.getValue(exec, propertyName);
+}
+
 Ref<ReadableStreamJSSource> ReadableStreamJSSource::create(JSC::ExecState* exec)
 {
     return adoptRef(*new ReadableStreamJSSource(exec));
index e720fa23ad78d9a4b8a463990186b4b0af9315ba..4463461266a6def233855600c2f787a2830a2076 100644 (file)
@@ -36,6 +36,7 @@
 #include <heap/Strong.h>
 #include <heap/StrongInlines.h>
 #include <runtime/JSCJSValue.h>
+#include <runtime/PrivateName.h>
 #include <wtf/Ref.h>
 
 namespace WebCore {
@@ -61,6 +62,9 @@ private:
     JSC::Strong<JSC::Unknown> m_error;
 };
 
+void setInternalSlotToObject(JSC::ExecState*, JSC::JSValue, JSC::PrivateName&, JSC::JSValue);
+JSC::JSValue getInternalSlotFromObject(JSC::ExecState*, JSC::JSValue, JSC::PrivateName&);
+
 } // namespace WebCore
 
 #endif // ENABLE(STREAMS_API)