Add ReadableWritablePair to Readable Streams for pipeThrough

In an effort to change the blink implementation of readable streams to
better match the Web IDL descriptions in the Streams API Standard, this
CL adds the ReadableWritablePair dictionary [1] to the ReadableStream
class.

ReadableWritablePair is used as a parameter for pipeThrough() to avoid
unnecessary extraction of the readable and writable streams separately.
This CL also includes changing the return type of pipeThrough() to
ReadableStream*. Furthermore, the comments and ordering of operations
in pipeThrough have also been changed to better reflect how the steps
outlined in the Streams API Standard. [2]

[1] https://streams.spec.whatwg.org/#dictdef-readablewritablepair
[2] https://streams.spec.whatwg.org/#rs-pipe-through

Bug: 1093862
Change-Id: I4a602f8bd03d300de5c4342d932b1ef8573943eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2505260
Commit-Queue: Nidhi Jaju <nidhijaju@google.com>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822111}

third_party/blink/renderer/bindings/generated_in_core.gni

@@ -271,6 +271,8 @@ generated_dictionary_sources_in_core = [
   "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_property_definition.h",
   "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_queuing_strategy_init.cc",
   "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_queuing_strategy_init.h",
+  "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_readable_writable_pair.cc",
+  "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_readable_writable_pair.h",
   "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_reporting_observer_options.cc",
   "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_reporting_observer_options.h",
   "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_request_init.cc",

third_party/blink/renderer/bindings/idl_in_core.gni

@@ -486,6 +486,7 @@ static_idl_files_in_core = get_path_info(
           "//third_party/blink/renderer/core/streams/readable_stream.idl",
           "//third_party/blink/renderer/core/streams/readable_stream_default_controller.idl",
           "//third_party/blink/renderer/core/streams/readable_stream_default_reader.idl",
+          "//third_party/blink/renderer/core/streams/readable_writable_pair.idl",
           "//third_party/blink/renderer/core/streams/stream_pipe_options.idl",
           "//third_party/blink/renderer/core/streams/transform_stream.idl",
           "//third_party/blink/renderer/core/streams/transform_stream_default_controller.idl",

third_party/blink/renderer/core/core_idl_files.gni

@@ -736,6 +736,7 @@ core_dictionary_idl_files =
                     "mojo/test/mojo_interface_request_event_init.idl",
                     "page/scrolling/scroll_state_init.idl",
                     "resize_observer/resize_observer_options.idl",
+                    "streams/readable_writable_pair.idl",
                     "streams/stream_pipe_options.idl",
                     "streams/queuing_strategy_init.idl",
                     "timing/measure_memory/measure_memory.idl",

third_party/blink/renderer/core/streams/readable_stream.cc

@@ -18,6 +18,7 @@
 #include "third_party/blink/renderer/core/streams/promise_handler.h"
 #include "third_party/blink/renderer/core/streams/readable_stream_default_controller.h"
 #include "third_party/blink/renderer/core/streams/readable_stream_reader.h"
+#include "third_party/blink/renderer/core/streams/readable_writable_pair.h"
 #include "third_party/blink/renderer/core/streams/stream_algorithms.h"
 #include "third_party/blink/renderer/core/streams/stream_pipe_options.h"
 #include "third_party/blink/renderer/core/streams/stream_promise_resolver.h"
@@ -1229,99 +1230,52 @@ ReadableStreamDefaultReader* ReadableStream::getReader(
   return getReader(script_state, exception_state);
 }
 
-ScriptValue ReadableStream::pipeThrough(ScriptState* script_state,
-                                        ScriptValue transform_stream,
+ReadableStream* ReadableStream::pipeThrough(ScriptState* script_state,
+                                            ReadableWritablePair* transform,
                                             ExceptionState& exception_state) {
-  return pipeThrough(script_state, transform_stream,
-                     StreamPipeOptions::Create(), exception_state);
+  return pipeThrough(script_state, transform, StreamPipeOptions::Create(),
+                     exception_state);
 }
 
 // https://streams.spec.whatwg.org/#rs-pipe-through
-ScriptValue ReadableStream::pipeThrough(ScriptState* script_state,
-                                        ScriptValue transform_stream,
+ReadableStream* ReadableStream::pipeThrough(ScriptState* script_state,
+                                            ReadableWritablePair* transform,
                                             const StreamPipeOptions* options,
                                             ExceptionState& exception_state) {
   // https://streams.spec.whatwg.org/#rs-pipe-through
-  // The first part of this function implements the unpacking of the {readable,
-  // writable} argument to the method.
-  v8::Local<v8::Value> pair_value = transform_stream.V8Value();
-  v8::Local<v8::Context> context = script_state->GetContext();
-
-  constexpr char kWritableIsNotWritableStream[] =
-      "parameter 1's 'writable' property is not a WritableStream.";
-  constexpr char kReadableIsNotReadableStream[] =
-      "parameter 1's 'readable' property is not a ReadableStream.";
-  constexpr char kWritableIsLocked[] = "parameter 1's 'writable' is locked.";
-
-  v8::Local<v8::Object> pair;
-  if (!pair_value->ToObject(context).ToLocal(&pair)) {
-    exception_state.ThrowTypeError(kWritableIsNotWritableStream);
-    return ScriptValue();
-  }
-
-  v8::Isolate* isolate = script_state->GetIsolate();
-  v8::Local<v8::Value> writable, readable;
-  {
-    v8::TryCatch block(isolate);
-    if (!pair->Get(context, V8String(isolate, "writable")).ToLocal(&writable)) {
-      exception_state.RethrowV8Exception(block.Exception());
-      return ScriptValue();
-    }
-    DCHECK(!block.HasCaught());
-
-    if (!pair->Get(context, V8String(isolate, "readable")).ToLocal(&readable)) {
-      exception_state.RethrowV8Exception(block.Exception());
-      return ScriptValue();
-    }
-    DCHECK(!block.HasCaught());
-  }
-
-  // 2. If ! IsWritableStream(_writable_) is *false*, throw a *TypeError*
-  //    exception.
-  WritableStream* writable_stream =
-      V8WritableStream::ToImplWithTypeCheck(isolate, writable);
-  if (!writable_stream) {
-    exception_state.ThrowTypeError(kWritableIsNotWritableStream);
-    return ScriptValue();
-  }
+  DCHECK(transform->hasReadable());
+  ReadableStream* readable_stream = transform->readable();
 
-  // 3. If ! IsReadableStream(_readable_) is *false*, throw a *TypeError*
-  //    exception.
-  if (!V8ReadableStream::HasInstance(readable, isolate)) {
-    exception_state.ThrowTypeError(kReadableIsNotReadableStream);
-    return ScriptValue();
-  }
-
-  // 4. If signal is not undefined, and signal is not an instance of the
-  //    AbortSignal interface, throw a TypeError exception.
-  auto* pipe_options = MakeGarbageCollected<PipeOptions>(options);
+  DCHECK(transform->hasWritable());
+  WritableStream* writable_stream = transform->writable();
 
-  // 5. If ! IsReadableStreamLocked(*this*) is *true*, throw a *TypeError*
-  //    exception.
+  // 1. If ! IsReadableStreamLocked(this) is true, throw a TypeError exception.
   if (IsLocked(this)) {
     exception_state.ThrowTypeError("Cannot pipe a locked stream");
-    return ScriptValue();
+    return nullptr;
   }
 
-  // 6. If ! IsWritableStreamLocked(_writable_) is *true*, throw a *TypeError*
-  //    exception.
+  // 2. If ! IsWritableStreamLocked(transform["writable"]) is true, throw a
+  //    TypeError exception.
   if (WritableStream::IsLocked(writable_stream)) {
-    exception_state.ThrowTypeError(kWritableIsLocked);
-    return ScriptValue();
+    exception_state.ThrowTypeError("parameter 1's 'writable' is locked");
+    return nullptr;
   }
 
-  // 7. Let _promise_ be ! ReadableStreamPipeTo(*this*, _writable_,
-  //    _preventClose_, _preventAbort_, _preventCancel_,
-  //   _signal_).
+  // 3. Let signal be options["signal"] if it exists, or undefined otherwise.
+  auto* pipe_options = MakeGarbageCollected<PipeOptions>(options);
 
+  // 4. Let promise be ! ReadableStreamPipeTo(this, transform["writable"],
+  //    options["preventClose"], options["preventAbort"],
+  //    options["preventCancel"], signal).
   ScriptPromise promise =
       PipeTo(script_state, this, writable_stream, pipe_options);
 
-  // 8. Set _promise_.[[PromiseIsHandled]] to *true*.
+  // 5. Set promise.[[PromiseIsHandled]] to true.
   promise.MarkAsHandled();
 
-  // 9. Return _readable_.
-  return ScriptValue(script_state->GetIsolate(), readable);
+  // 6. Return transform["readable"].
+  return readable_stream;
 }
 
 ScriptPromise ReadableStream::pipeTo(ScriptState* script_state,

third_party/blink/renderer/core/streams/readable_stream.h

@@ -21,6 +21,7 @@ class AbortSignal;
 class ExceptionState;
 class MessagePort;
 class ReadableStreamDefaultController;
+class ReadableWritablePair;
 class ScriptPromise;
 class ScriptState;
 class StrategySizeAlgorithm;
@@ -115,13 +116,13 @@ class CORE_EXPORT ReadableStream : public ScriptWrappable {
                                          ScriptValue options,
                                          ExceptionState&);
 
-  ScriptValue pipeThrough(ScriptState*,
-                          ScriptValue transform_stream,
+  ReadableStream* pipeThrough(ScriptState*,
+                              ReadableWritablePair* transform,
                               ExceptionState&);
 
   // https://streams.spec.whatwg.org/#rs-pipe-through
-  ScriptValue pipeThrough(ScriptState*,
-                          ScriptValue transform_stream,
+  ReadableStream* pipeThrough(ScriptState*,
+                              ReadableWritablePair* transform,
                               const StreamPipeOptions* options,
                               ExceptionState&);
 

third_party/blink/renderer/core/streams/readable_stream.idl

@@ -12,8 +12,8 @@
     // TODO(yhirano): function length is different from what's specced. Fix it.
     [RaisesException, CallWith=ScriptState] Promise<any> cancel(optional any reason);
     [RaisesException, CallWith=ScriptState, MeasureAs=ReadableStreamGetReader] ReadableStreamDefaultReader getReader(optional any mode);
-    [RaisesException, CallWith=ScriptState, MeasureAs=ReadableStreamPipeThrough] any pipeThrough(
-        any transformStream, optional StreamPipeOptions options);
+    [RaisesException, CallWith=ScriptState, MeasureAs=ReadableStreamPipeThrough] ReadableStream pipeThrough(
+        ReadableWritablePair transform, optional StreamPipeOptions options);
     [RaisesException, CallWith=ScriptState, MeasureAs=ReadableStreamPipeTo] Promise<any> pipeTo(
         WritableStream destination, optional StreamPipeOptions options);
     [RaisesException, CallWith=ScriptState] any tee();

third_party/blink/renderer/core/streams/readable_writable_pair.idl

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// https://streams.spec.whatwg.org/#dictdef-readablewritablepair
+
+dictionary ReadableWritablePair {
+  required ReadableStream readable;
+  required WritableStream writable;
+};

third_party/blink/web_tests/external/wpt/streams/piping/pipe-through.any-expected.txt

-This is a testharness.js-based test.
-PASS Piping through a duck-typed pass-through transform stream should work
-PASS Piping through a transform errored on the writable end does not cause an unhandled promise rejection
-PASS pipeThrough should not call pipeTo on this
-PASS pipeThrough should not call pipeTo on the ReadableStream prototype
-PASS pipeThrough should brand-check this and not allow 'null'
-FAIL pipeThrough should brand-check readable and not allow 'null' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'undefined'
-FAIL pipeThrough should brand-check readable and not allow 'undefined' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow '0'
-FAIL pipeThrough should brand-check readable and not allow '0' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'NaN'
-FAIL pipeThrough should brand-check readable and not allow 'NaN' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'true'
-FAIL pipeThrough should brand-check readable and not allow 'true' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'ReadableStream'
-FAIL pipeThrough should brand-check readable and not allow 'ReadableStream' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow '[object ReadableStream]'
-FAIL pipeThrough should brand-check readable and not allow '[object ReadableStream]' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check writable and not allow 'null'
-PASS pipeThrough should brand-check writable and not allow 'undefined'
-PASS pipeThrough should brand-check writable and not allow '0'
-PASS pipeThrough should brand-check writable and not allow 'NaN'
-PASS pipeThrough should brand-check writable and not allow 'true'
-PASS pipeThrough should brand-check writable and not allow 'WritableStream'
-PASS pipeThrough should brand-check writable and not allow '[object WritableStream]'
-PASS pipeThrough should rethrow errors from accessing readable or writable
-PASS invalid values of signal should throw; specifically 'null'
-PASS invalid values of signal should throw; specifically '0'
-PASS invalid values of signal should throw; specifically 'NaN'
-PASS invalid values of signal should throw; specifically 'true'
-PASS invalid values of signal should throw; specifically 'AbortSignal'
-PASS invalid values of signal should throw; specifically '[object AbortSignal]'
-PASS pipeThrough should accept a real AbortSignal
-PASS pipeThrough should throw if this is locked
-PASS pipeThrough should throw if writable is locked
-PASS pipeThrough should not care if readable is locked
-PASS preventCancel should work
-PASS preventClose should work
-PASS preventAbort should work
-PASS pipeThrough() should throw if an option getter grabs a writer
-Harness: the test ran to completion.
-

third_party/blink/web_tests/external/wpt/streams/piping/pipe-through.any.serviceworker-expected.txt

-This is a testharness.js-based test.
-PASS Piping through a duck-typed pass-through transform stream should work
-PASS Piping through a transform errored on the writable end does not cause an unhandled promise rejection
-PASS pipeThrough should not call pipeTo on this
-PASS pipeThrough should not call pipeTo on the ReadableStream prototype
-PASS pipeThrough should brand-check this and not allow 'null'
-FAIL pipeThrough should brand-check readable and not allow 'null' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'undefined'
-FAIL pipeThrough should brand-check readable and not allow 'undefined' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow '0'
-FAIL pipeThrough should brand-check readable and not allow '0' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'NaN'
-FAIL pipeThrough should brand-check readable and not allow 'NaN' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'true'
-FAIL pipeThrough should brand-check readable and not allow 'true' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'ReadableStream'
-FAIL pipeThrough should brand-check readable and not allow 'ReadableStream' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow '[object ReadableStream]'
-FAIL pipeThrough should brand-check readable and not allow '[object ReadableStream]' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check writable and not allow 'null'
-PASS pipeThrough should brand-check writable and not allow 'undefined'
-PASS pipeThrough should brand-check writable and not allow '0'
-PASS pipeThrough should brand-check writable and not allow 'NaN'
-PASS pipeThrough should brand-check writable and not allow 'true'
-PASS pipeThrough should brand-check writable and not allow 'WritableStream'
-PASS pipeThrough should brand-check writable and not allow '[object WritableStream]'
-PASS pipeThrough should rethrow errors from accessing readable or writable
-PASS invalid values of signal should throw; specifically 'null'
-PASS invalid values of signal should throw; specifically '0'
-PASS invalid values of signal should throw; specifically 'NaN'
-PASS invalid values of signal should throw; specifically 'true'
-PASS invalid values of signal should throw; specifically 'AbortSignal'
-PASS invalid values of signal should throw; specifically '[object AbortSignal]'
-PASS pipeThrough should accept a real AbortSignal
-PASS pipeThrough should throw if this is locked
-PASS pipeThrough should throw if writable is locked
-PASS pipeThrough should not care if readable is locked
-PASS preventCancel should work
-PASS preventClose should work
-PASS preventAbort should work
-PASS pipeThrough() should throw if an option getter grabs a writer
-Harness: the test ran to completion.
-

third_party/blink/web_tests/external/wpt/streams/piping/pipe-through.any.sharedworker-expected.txt

-This is a testharness.js-based test.
-PASS Piping through a duck-typed pass-through transform stream should work
-PASS Piping through a transform errored on the writable end does not cause an unhandled promise rejection
-PASS pipeThrough should not call pipeTo on this
-PASS pipeThrough should not call pipeTo on the ReadableStream prototype
-PASS pipeThrough should brand-check this and not allow 'null'
-FAIL pipeThrough should brand-check readable and not allow 'null' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'undefined'
-FAIL pipeThrough should brand-check readable and not allow 'undefined' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow '0'
-FAIL pipeThrough should brand-check readable and not allow '0' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'NaN'
-FAIL pipeThrough should brand-check readable and not allow 'NaN' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'true'
-FAIL pipeThrough should brand-check readable and not allow 'true' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'ReadableStream'
-FAIL pipeThrough should brand-check readable and not allow 'ReadableStream' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow '[object ReadableStream]'
-FAIL pipeThrough should brand-check readable and not allow '[object ReadableStream]' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check writable and not allow 'null'
-PASS pipeThrough should brand-check writable and not allow 'undefined'
-PASS pipeThrough should brand-check writable and not allow '0'
-PASS pipeThrough should brand-check writable and not allow 'NaN'
-PASS pipeThrough should brand-check writable and not allow 'true'
-PASS pipeThrough should brand-check writable and not allow 'WritableStream'
-PASS pipeThrough should brand-check writable and not allow '[object WritableStream]'
-PASS pipeThrough should rethrow errors from accessing readable or writable
-PASS invalid values of signal should throw; specifically 'null'
-PASS invalid values of signal should throw; specifically '0'
-PASS invalid values of signal should throw; specifically 'NaN'
-PASS invalid values of signal should throw; specifically 'true'
-PASS invalid values of signal should throw; specifically 'AbortSignal'
-PASS invalid values of signal should throw; specifically '[object AbortSignal]'
-PASS pipeThrough should accept a real AbortSignal
-PASS pipeThrough should throw if this is locked
-PASS pipeThrough should throw if writable is locked
-PASS pipeThrough should not care if readable is locked
-PASS preventCancel should work
-PASS preventClose should work
-PASS preventAbort should work
-PASS pipeThrough() should throw if an option getter grabs a writer
-Harness: the test ran to completion.
-

third_party/blink/web_tests/external/wpt/streams/piping/pipe-through.any.worker-expected.txt

-This is a testharness.js-based test.
-PASS Piping through a duck-typed pass-through transform stream should work
-PASS Piping through a transform errored on the writable end does not cause an unhandled promise rejection
-PASS pipeThrough should not call pipeTo on this
-PASS pipeThrough should not call pipeTo on the ReadableStream prototype
-PASS pipeThrough should brand-check this and not allow 'null'
-FAIL pipeThrough should brand-check readable and not allow 'null' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'undefined'
-FAIL pipeThrough should brand-check readable and not allow 'undefined' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow '0'
-FAIL pipeThrough should brand-check readable and not allow '0' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'NaN'
-FAIL pipeThrough should brand-check readable and not allow 'NaN' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'true'
-FAIL pipeThrough should brand-check readable and not allow 'true' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow 'ReadableStream'
-FAIL pipeThrough should brand-check readable and not allow 'ReadableStream' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check this and not allow '[object ReadableStream]'
-FAIL pipeThrough should brand-check readable and not allow '[object ReadableStream]' assert_false: writable should not have been accessed expected false got true
-PASS pipeThrough should brand-check writable and not allow 'null'
-PASS pipeThrough should brand-check writable and not allow 'undefined'
-PASS pipeThrough should brand-check writable and not allow '0'
-PASS pipeThrough should brand-check writable and not allow 'NaN'
-PASS pipeThrough should brand-check writable and not allow 'true'
-PASS pipeThrough should brand-check writable and not allow 'WritableStream'
-PASS pipeThrough should brand-check writable and not allow '[object WritableStream]'
-PASS pipeThrough should rethrow errors from accessing readable or writable
-PASS invalid values of signal should throw; specifically 'null'
-PASS invalid values of signal should throw; specifically '0'
-PASS invalid values of signal should throw; specifically 'NaN'
-PASS invalid values of signal should throw; specifically 'true'
-PASS invalid values of signal should throw; specifically 'AbortSignal'
-PASS invalid values of signal should throw; specifically '[object AbortSignal]'
-PASS pipeThrough should accept a real AbortSignal
-PASS pipeThrough should throw if this is locked
-PASS pipeThrough should throw if writable is locked
-PASS pipeThrough should not care if readable is locked
-PASS preventCancel should work
-PASS preventClose should work
-PASS preventAbort should work
-PASS pipeThrough() should throw if an option getter grabs a writer
-Harness: the test ran to completion.
-