[DevTools] Revamp StringBuffer implementations.

This cleans up string-util.h a little. Instead of distinguishing
BinaryStringBuffer from StringBufferImpl (which has a legacy
::adopt() interface), provide two functions for making StringBuffer
from either an 8 bit (std::vector<uint8_t>) or a 16 bit (String16)
source, and keep the classes in the .cc file only. Since it's easy
enough, this adds a class also for the empty string buffer as there's
no need to keep an empty vector / String16 this way.

No public API changes here.

Change-Id: Idb25fe24ea94f27f8001d552cede089e04eacd32
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2016015Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Johannes Henkel <johannes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65964}

src/inspector/inspector_protocol_config.json

@@ -36,7 +36,7 @@
         "string_header": "v8-inspector.h",
         "string_in": "StringView",
         "string_out": "std::unique_ptr<StringBuffer>",
-        "to_string_out": "StringBufferImpl::adopt(%s)",
+        "to_string_out": "StringBufferFrom(std::move(%s))",
         "export_macro": "V8_EXPORT"
     },
 

src/inspector/string-util.cc

@@ -73,14 +73,12 @@ String16 toString16(const StringView& string) {
   if (string.is8Bit())
     return String16(reinterpret_cast<const char*>(string.characters8()),
                     string.length());
-  return String16(reinterpret_cast<const UChar*>(string.characters16()),
-                  string.length());
+  return String16(string.characters16(), string.length());
 }
 
 StringView toStringView(const String16& string) {
   if (string.isEmpty()) return StringView();
-  return StringView(reinterpret_cast<const uint16_t*>(string.characters16()),
-                    string.length());
+  return StringView(string.characters16(), string.length());
 }
 
 bool stringViewStartsWith(const StringView& string, const char* prefix) {
@@ -126,20 +124,63 @@ std::unique_ptr<protocol::Value> StringUtil::parseJSON(const String16& string) {
 }
 }  // namespace protocol
 
+namespace {
+// An empty string buffer doesn't own any string data; its ::string() returns a
+// default-constructed StringView instance.
+class EmptyStringBuffer : public StringBuffer {
+ public:
+  const StringView& string() override { return string_; }
+
+ private:
+  StringView string_;
+};
+
+// Contains LATIN1 text data or CBOR encoded binary data in a vector.
+class StringBuffer8 : public StringBuffer {
+ public:
+  explicit StringBuffer8(std::vector<uint8_t> data)
+      : data_(std::move(data)), string_(data_.data(), data_.size()) {}
+
+  const StringView& string() override { return string_; }
+
+ private:
+  std::vector<uint8_t> data_;
+  StringView string_;
+};
+
+// Contains a 16 bit string (String16).
+class StringBuffer16 : public StringBuffer {
+ public:
+  explicit StringBuffer16(String16 data)
+      : data_(std::move(data)), string_(data_.characters16(), data_.length()) {}
+
+  const StringView& string() override { return string_; }
+
+ private:
+  String16 data_;
+  StringView string_;
+};
+}  // namespace
+
 // static
 std::unique_ptr<StringBuffer> StringBuffer::create(const StringView& string) {
-  String16 owner = toString16(string);
-  return StringBufferImpl::adopt(owner);
+  if (string.length() == 0) return std::make_unique<EmptyStringBuffer>();
+  if (string.is8Bit()) {
+    return std::make_unique<StringBuffer8>(std::vector<uint8_t>(
+        string.characters8(), string.characters8() + string.length()));
+  }
+  return std::make_unique<StringBuffer16>(
+      String16(string.characters16(), string.length()));
 }
 
-// static
-std::unique_ptr<StringBufferImpl> StringBufferImpl::adopt(String16& string) {
-  return std::unique_ptr<StringBufferImpl>(new StringBufferImpl(string));
+std::unique_ptr<StringBuffer> StringBufferFrom(String16 str) {
+  if (str.isEmpty()) return std::make_unique<EmptyStringBuffer>();
+  return std::make_unique<StringBuffer16>(std::move(str));
 }
 
-StringBufferImpl::StringBufferImpl(String16& string) {
-  m_owner.swap(string);
-  m_string = toStringView(m_owner);
+std::unique_ptr<StringBuffer> StringBufferFrom(std::vector<uint8_t> str) {
+  if (str.empty()) return std::make_unique<EmptyStringBuffer>();
+  return std::make_unique<StringBuffer8>(std::move(str));
 }
 
 String16 stackTraceIdToString(uintptr_t id) {

src/inspector/string-util.h

@@ -121,32 +121,13 @@ String16 toString16(const StringView&);
 StringView toStringView(const String16&);
 bool stringViewStartsWith(const StringView&, const char*);
 
-class StringBufferImpl : public StringBuffer {
- public:
-  // Destroys string's content.
-  static std::unique_ptr<StringBufferImpl> adopt(String16&);
-  const StringView& string() override { return m_string; }
-
- private:
-  explicit StringBufferImpl(String16&);
-  String16 m_owner;
-  StringView m_string;
-
-  DISALLOW_COPY_AND_ASSIGN(StringBufferImpl);
-};
+// Creates a string buffer instance which owns |str|, a 16 bit string.
+std::unique_ptr<StringBuffer> StringBufferFrom(String16 str);
 
-class BinaryStringBuffer : public StringBuffer {
- public:
-  explicit BinaryStringBuffer(std::vector<uint8_t> data)
-      : m_data(std::move(data)), m_string(m_data.data(), m_data.size()) {}
-  const StringView& string() override { return m_string; }
-
- private:
-  std::vector<uint8_t> m_data;
-  StringView m_string;
-
-  DISALLOW_COPY_AND_ASSIGN(BinaryStringBuffer);
-};
+// Creates a string buffer instance which owns |str|, an 8 bit string.
+// 8 bit strings are used for LATIN1 text (which subsumes 7 bit ASCII, e.g.
+// our generated JSON), as well as for CBOR encoded binary messages.
+std::unique_ptr<StringBuffer> StringBufferFrom(std::vector<uint8_t> str);
 
 String16 stackTraceIdToString(uintptr_t id);
 

src/inspector/v8-inspector-session-impl.cc

@@ -169,15 +169,19 @@ std::unique_ptr<StringBuffer> V8InspectorSessionImpl::serializeForFrontend(
     std::unique_ptr<protocol::Serializable> message) {
   std::vector<uint8_t> cbor = std::move(*message).TakeSerialized();
   DCHECK(CheckCBORMessage(SpanFrom(cbor)).ok());
-  if (use_binary_protocol_)
-    return std::unique_ptr<StringBuffer>(
-        new BinaryStringBuffer(std::move(cbor)));
+  if (use_binary_protocol_) return StringBufferFrom(std::move(cbor));
   std::vector<uint8_t> json;
   Status status = ConvertCBORToJSON(SpanFrom(cbor), &json);
   DCHECK(status.ok());
   USE(status);
+  // TODO(johannes): It should be OK to make a StringBuffer from |json|
+  // directly, since it's 7 Bit US-ASCII with anything else escaped.
+  // However it appears that the Node.js tests (or perhaps even production)
+  // assume that the StringBuffer is 16 Bit. It probably accesses
+  // characters16() somehwere without checking is8Bit. Until it's fixed
+  // we take a detour via String16 which makes the StringBuffer 16 bit.
   String16 string16(reinterpret_cast<const char*>(json.data()), json.size());
-  return StringBufferImpl::adopt(string16);
+  return StringBufferFrom(std::move(string16));
 }
 
 void V8InspectorSessionImpl::sendProtocolResponse(
@@ -256,13 +260,11 @@ bool V8InspectorSessionImpl::unwrapObject(
   Response response = unwrapObject(toString16(objectId), object, context,
                                    objectGroup ? &objectGroupString : nullptr);
   if (!response.isSuccess()) {
-    if (error) {
-      String16 errorMessage = response.errorMessage();
-      *error = StringBufferImpl::adopt(errorMessage);
-    }
+    if (error) *error = StringBufferFrom(response.errorMessage());
     return false;
   }
-  if (objectGroup) *objectGroup = StringBufferImpl::adopt(objectGroupString);
+  if (objectGroup)
+    *objectGroup = StringBufferFrom(std::move(objectGroupString));
   return true;
 }
 

src/inspector/v8-stack-trace-impl.cc

@@ -153,9 +153,7 @@ std::unique_ptr<StringBuffer> V8StackTraceId::ToString() {
   std::vector<uint8_t> json;
   std::vector<uint8_t> cbor = std::move(*dict).TakeSerialized();
   v8_crdtp::json::ConvertCBORToJSON(v8_crdtp::SpanFrom(cbor), &json);
-  // |json| is 7 bit ASCII (with \uXXXX JSON escapes).
-  // BinaryStringBuffer keeps the JSON and exposes an 8 bit StringView.
-  return std::make_unique<BinaryStringBuffer>(std::move(json));
+  return StringBufferFrom(std::move(json));
 }
 
 StackFrame::StackFrame(v8::Isolate* isolate, v8::Local<v8::StackFrame> v8Frame)
@@ -341,8 +339,7 @@ std::unique_ptr<StringBuffer> V8StackTraceImpl::toString() const {
     stackTrace.append(String16::fromInteger(frame.columnNumber() + 1));
     stackTrace.append(')');
   }
-  String16 string = stackTrace.toString();
-  return StringBufferImpl::adopt(string);
+  return StringBufferFrom(stackTrace.toString());
 }
 
 bool V8StackTraceImpl::isEqualIgnoringTopFrame(