[serial] Replace SerialPort.clearReadError()

This change replaces the clearReadError() method with implicit behavior
in the getters for the readable and writable attributes. This should be
much more convenient for developers without any change in functionality.

Bug: 893334
Change-Id: I76cab6656a2ad117fcc3d1fda5eaebd052e4d2b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1722011Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682828}

third_party/blink/renderer/modules/serial/serial_port.cc

@@ -17,6 +17,10 @@
 namespace blink {
 
 namespace {
+const char kResourcesExhaustedReadBuffer[] =
+    "Resources exhausted allocating read buffer.";
+const char kResourcesExhaustedWriteBuffer[] =
+    "Resources exhausted allocation write buffer.";
 const char kOpenError[] = "Failed to open serial port.";
 const char kUnexpectedCloseError[] = "The port closed unexpectedly.";
 const int kMaxBufferSize = 16 * 1024 * 1024; /* 16 MiB */
@@ -122,7 +126,7 @@ ScriptPromise SerialPort::open(ScriptState* script_state,
     return ScriptPromise::RejectWithDOMException(
         script_state, MakeGarbageCollected<DOMException>(
                           DOMExceptionCode::kQuotaExceededError,
-                          "Resources exhausted allocating read buffer."));
+                          kResourcesExhaustedReadBuffer));
   }
 
   // Pipe handle pair for the WritableStream.
@@ -132,7 +136,7 @@ ScriptPromise SerialPort::open(ScriptState* script_state,
     return ScriptPromise::RejectWithDOMException(
         script_state, MakeGarbageCollected<DOMException>(
                           DOMExceptionCode::kQuotaExceededError,
-                          "Resources exhausted allocating read buffer."));
+                          kResourcesExhaustedWriteBuffer));
   }
 
   device::mojom::blink::SerialPortClientPtr client_ptr;
@@ -153,37 +157,54 @@ ScriptPromise SerialPort::open(ScriptState* script_state,
   return open_resolver_->Promise();
 }
 
-void SerialPort::clearReadError(ScriptState* script_state,
-                                ExceptionState& exception_state) {
-  if (!port_) {
-    exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
-                                      "Port is not open.");
-    return;
-  }
+ReadableStream* SerialPort::readable(ScriptState* script_state,
+                                     ExceptionState& exception_state) {
+  if (readable_)
+    return readable_;
 
-  if (underlying_source_) {
-    exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
-                                      "No error to clear.");
-    return;
-  }
+  if (!port_ || open_resolver_)
+    return nullptr;
 
   mojo::ScopedDataPipeConsumerHandle readable_pipe;
   mojo::ScopedDataPipeProducerHandle readable_pipe_producer;
   if (!CreateDataPipe(&readable_pipe_producer, &readable_pipe)) {
-    exception_state.ThrowDOMException(
-        DOMExceptionCode::kQuotaExceededError,
-        "Resources exhausted allocating read buffer.");
-    return;
+    exception_state.ThrowDOMException(DOMExceptionCode::kQuotaExceededError,
+                                      kResourcesExhaustedReadBuffer);
+    return nullptr;
   }
 
   port_->ClearReadError(std::move(readable_pipe_producer));
   InitializeReadableStream(script_state, std::move(readable_pipe));
+  return readable_;
+}
+
+WritableStream* SerialPort::writable(ScriptState* script_state,
+                                     ExceptionState& exception_state) {
+  if (writable_)
+    return writable_;
+
+  if (!port_ || open_resolver_)
+    return nullptr;
+
+  mojo::ScopedDataPipeProducerHandle writable_pipe;
+  mojo::ScopedDataPipeConsumerHandle writable_pipe_consumer;
+  if (!CreateDataPipe(&writable_pipe, &writable_pipe_consumer)) {
+    exception_state.ThrowDOMException(DOMExceptionCode::kQuotaExceededError,
+                                      kResourcesExhaustedWriteBuffer);
+    return nullptr;
+  }
+
+  port_->ClearSendError(std::move(writable_pipe_consumer));
+  InitializeWritableStream(script_state, std::move(writable_pipe));
+  return writable_;
 }
 
 void SerialPort::close() {
   if (underlying_source_) {
     // The ReadableStream will report "done" when the data pipe is closed.
     underlying_source_->ExpectClose();
+    underlying_source_ = nullptr;
+    readable_ = nullptr;
   }
   if (underlying_sink_) {
     // TODO(crbug.com/893334): Rather than triggering an error on the
@@ -191,8 +212,12 @@ void SerialPort::close() {
     // is locked.
     underlying_sink_->SignalErrorOnClose(MakeGarbageCollected<DOMException>(
         DOMExceptionCode::kInvalidStateError, "The port has been closed."));
+    underlying_sink_ = nullptr;
+    writable_ = nullptr;
   }
-  ContextDestroyed();
+  port_.reset();
+  if (client_binding_.is_bound())
+    client_binding_.Unbind();
 }
 
 void SerialPort::UnderlyingSourceClosed() {
@@ -208,12 +233,6 @@ void SerialPort::UnderlyingSinkClosed() {
 void SerialPort::ContextDestroyed() {
   // Release connection-related resources as quickly as possible.
   port_.reset();
-  if (client_binding_.is_bound())
-    client_binding_.Unbind();
-  readable_ = nullptr;
-  underlying_source_ = nullptr;
-  writable_ = nullptr;
-  underlying_sink_ = nullptr;
 }
 
 void SerialPort::Trace(Visitor* visitor) {
@@ -266,6 +285,7 @@ bool SerialPort::CreateDataPipe(mojo::ScopedDataPipeProducerHandle* producer,
 }
 
 void SerialPort::OnConnectionError() {
+  port_.reset();
   if (open_resolver_) {
     open_resolver_->Reject(MakeGarbageCollected<DOMException>(
         DOMExceptionCode::kNetworkError, kOpenError));
@@ -279,7 +299,8 @@ void SerialPort::OnConnectionError() {
     underlying_sink_->SignalErrorOnClose(MakeGarbageCollected<DOMException>(
         DOMExceptionCode::kNetworkError, kUnexpectedCloseError));
   }
-  ContextDestroyed();
+  if (client_binding_.is_bound())
+    client_binding_.Unbind();
 }
 
 void SerialPort::OnOpen(

third_party/blink/renderer/modules/serial/serial_port.h

@@ -37,11 +37,9 @@ class SerialPort final : public ScriptWrappable,
   ~SerialPort() override;
 
   // Web-exposed functions
-  ReadableStream* readable() const { return readable_; }
-  WritableStream* writable() const { return writable_; }
-
   ScriptPromise open(ScriptState*, const SerialOptions* options);
-  void clearReadError(ScriptState*, ExceptionState&);
+  ReadableStream* readable(ScriptState*, ExceptionState&);
+  WritableStream* writable(ScriptState*, ExceptionState&);
   void close();
 
   const base::UnguessableToken& token() const { return info_->token; }

third_party/blink/renderer/modules/serial/serial_port.idl

@@ -8,15 +8,14 @@
     Exposed(Window Serial,DedicatedWorker Serial),
     SecureContext
 ] interface SerialPort {
+    [CallWith=ScriptState, RaisesException]
     readonly attribute ReadableStream readable;
+    [CallWith=ScriptState, RaisesException]
     readonly attribute WritableStream writable;
 
     [CallWith=ScriptState, MeasureAs=SerialPortOpen]
     Promise<void> open(SerialOptions options);
 
-    [CallWith=ScriptState, RaisesException]
-    void clearReadError();
-
     [MeasureAs=SerialPortClose]
     void close();
 };

third_party/blink/web_tests/serial/serialPort_clearReadError.html → third_party/blink/web_tests/serial/serialPort_close.html

@@ -10,33 +10,15 @@
 serial_test(async (t, fake) => {
   const { port, fakePort } = await getFakeSerialPort(fake);
 
-  assert_throws('InvalidStateError', () => port.clearReadError());
-}, 'clearReadError() not allowed on a closed port.');
+  port.close();
+}, 'A SerialPort can be closed if it was never opened.');
 
 serial_test(async (t, fake) => {
   const { port, fakePort } = await getFakeSerialPort(fake);
 
   await port.open({ baudrate: 9600 });
-  assert_throws('InvalidStateError', () => port.clearReadError());
-}, 'clearReadError() not allowed without a reported error');
-
-serial_test(async (t, fake) => {
-  const { port, fakePort } = await getFakeSerialPort(fake);
-  await port.open({ baudrate: 9600 });
-
-  fakePort.simulateParityError();
-
-  assert_throws('InvalidStateError', () => port.clearReadError());
-}, 'clearReadError() not allowed before readable has been aborted');
-
-serial_test(async (t, fake) => {
-  const { port, fakePort } = await getFakeSerialPort(fake);
-  await port.open({ baudrate: 9600 });
-
-  fakePort.simulateParityError();
-
-  await promise_rejects(t, 'NetworkError', port.readable.getReader().read());
-  port.clearReadError();
-}, 'clearReadError() can be called after readable reports error');
+  port.close();
+  port.close();
+}, 'A SerialPort can be closed multiple times.');
 
 </script>

third_party/blink/web_tests/serial/serialPort_readable.html

@@ -13,28 +13,17 @@ serial_test(async (t, fake) => {
   assert_equals(port.readable, null);
 
   await port.open({ baudrate: 9600 });
-  assert_true(port.readable instanceof ReadableStream);
+  const readable = port.readable;
+  assert_true(readable instanceof ReadableStream);
 
   port.close();
   assert_equals(port.readable, null);
-}, 'open() and close() set and unset SerialPort.readable');
-
-serial_test(async (t, fake) => {
-  const { port, fakePort } = await getFakeSerialPort(fake);
-
-  assert_equals(port.readable, null);
-
-  await port.open({ baudrate: 9600 });
-  assert_true(port.readable instanceof ReadableStream);
-
-  const reader = port.readable.getReader();
-  port.close();
 
+  const reader = readable.getReader();
   const { value, done } = await reader.read();
   assert_true(done);
   assert_equals(value, undefined);
-  assert_equals(port.readable, null);
-}, 'SerialPort.readable closes on port close');
+}, 'SerialPort.readable is set by open() and closes on port close');
 
 serial_test(async (t, fake) => {
   const { port, fakePort } = await getFakeSerialPort(fake);
@@ -81,16 +70,17 @@ serial_test(async (t, fake) => {
   fakePort.write(data);
   fakePort.simulateParityError();
 
-  let reader = port.readable.getReader();
+  let readable = port.readable;
+  let reader = readable.getReader();
   let { value, done } = await reader.read();
   assert_false(done);
   compareArrays(data, value);
 
   await promise_rejects(t, 'NetworkError', reader.read());
-  assert_equals(port.readable, null);
+  assert_not_equals(port.readable, readable);
 
-  port.clearReadError();
-  assert_true(port.readable instanceof ReadableStream);
+  readable = port.readable;
+  assert_true(readable instanceof ReadableStream);
   reader = port.readable.getReader();
 
   await fakePort.waitForErrorCleared();
@@ -101,10 +91,11 @@ serial_test(async (t, fake) => {
   compareArrays(data, value);
 
   port.close();
+  assert_equals(port.readable, null);
+
   ({ value, done } = await reader.read());
   assert_true(done);
   assert_equals(undefined, value);
-}, 'Parity error closes readable, can be cleared and more data received');
+}, 'Parity error closes readable and replaces it with a new stream');
 
 </script>
-

third_party/blink/web_tests/webexposed/global-interface-listing-dedicated-worker-expected.txt

@@ -1202,7 +1202,6 @@ Starting worker: resources/global-interface-listing-worker.js
 [Worker]     attribute @@toStringTag
 [Worker]     getter readable
 [Worker]     getter writable
-[Worker]     method clearReadError
 [Worker]     method close
 [Worker]     method constructor
 [Worker]     method open

third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt

@@ -7448,7 +7448,6 @@ interface SerialPort
     attribute @@toStringTag
     getter readable
     getter writable
-    method clearReadError
     method close
     method constructor
     method open