BinaryValue support for NULL buffer.

* Apply original change from CL 10389088 (https://chromiumcodereview.appspot.com/10389088)

* Change BinaryValue to use "scoped_ptr<char[]>" instead of "scoped_ptr<char>". By contract, the memory owned should be deleted with the "delete[]" operator.

BUG=127630

Review URL: https://chromiumcodereview.appspot.com/11745016

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@175482 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 43 insertions, 61 deletions

diff --git a/base/values.cc b/base/values.cc
index 459d56f..7a21709 100644
--- a/base/values.cc
+++ b/base/values.cc
@@ -306,33 +306,32 @@ bool StringValue::Equals(const Value* other) const {
 
 ///////////////////// BinaryValue ////////////////////
 
-BinaryValue::~BinaryValue() {
-  DCHECK(buffer_);
-  if (buffer_)
-    delete[] buffer_;
+BinaryValue::BinaryValue()
+    : Value(TYPE_BINARY),
+      buffer_(NULL),
+      size_(0) {
 }
 
-// static
-BinaryValue* BinaryValue::Create(char* buffer, size_t size) {
-  if (!buffer)
-    return NULL;
+BinaryValue::BinaryValue(scoped_ptr<char[]> buffer, size_t size)
+    : Value(TYPE_BINARY),
+      buffer_(buffer.Pass()),
+      size_(size) {
+}
 
-  return new BinaryValue(buffer, size);
+BinaryValue::~BinaryValue() {
 }
 
 // static
 BinaryValue* BinaryValue::CreateWithCopiedBuffer(const char* buffer,
                                                  size_t size) {
-  if (!buffer)
-    return NULL;
-
   char* buffer_copy = new char[size];
   memcpy(buffer_copy, buffer, size);
-  return new BinaryValue(buffer_copy, size);
+  scoped_ptr<char[]> scoped_buffer_copy(buffer_copy);
+  return new BinaryValue(scoped_buffer_copy.Pass(), size);
 }
 
 BinaryValue* BinaryValue::DeepCopy() const {
-  return CreateWithCopiedBuffer(buffer_, size_);
+  return CreateWithCopiedBuffer(buffer_.get(), size_);
 }
 
 bool BinaryValue::Equals(const Value* other) const {
@@ -341,14 +340,7 @@ bool BinaryValue::Equals(const Value* other) const {
   const BinaryValue* other_binary = static_cast<const BinaryValue*>(other);
   if (other_binary->size_ != size_)
     return false;
-  return !memcmp(buffer_, other_binary->buffer_, size_);
-}
-
-BinaryValue::BinaryValue(char* buffer, size_t size)
-  : Value(TYPE_BINARY),
-    buffer_(buffer),
-    size_(size) {
-  DCHECK(buffer_);
+  return !memcmp(GetBuffer(), other_binary->GetBuffer(), size_);
 }
 
 ///////////////////// DictionaryValue ////////////////////
diff --git a/base/values.h b/base/values.h
index e105818..d06426c 100644
--- a/base/values.h
+++ b/base/values.h
@@ -29,6 +29,7 @@
 #include "base/base_export.h"
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/string16.h"
 
 // This file declares "using base::Value", etc. at the bottom, so that
@@ -168,33 +169,32 @@ class BASE_EXPORT StringValue : public Value {
 
 class BASE_EXPORT BinaryValue: public Value {
  public:
-  virtual ~BinaryValue();
+  // Creates a BinaryValue with a null buffer and size of 0.
+  BinaryValue();
+
+  // Creates a BinaryValue, taking ownership of the bytes pointed to by
+  // |buffer|.
+  BinaryValue(scoped_ptr<char[]> buffer, size_t size);
 
-  // Creates a Value to represent a binary buffer.  The new object takes
-  // ownership of the pointer passed in, if successful.
-  // Returns NULL if buffer is NULL.
-  static BinaryValue* Create(char* buffer, size_t size);
+  virtual ~BinaryValue();
 
   // For situations where you want to keep ownership of your buffer, this
   // factory method creates a new BinaryValue by copying the contents of the
   // buffer that's passed in.
-  // Returns NULL if buffer is NULL.
   static BinaryValue* CreateWithCopiedBuffer(const char* buffer, size_t size);
 
   size_t GetSize() const { return size_; }
-  char* GetBuffer() { return buffer_; }
-  const char* GetBuffer() const { return buffer_; }
+
+  // May return NULL.
+  char* GetBuffer() { return buffer_.get(); }
+  const char* GetBuffer() const { return buffer_.get(); }
 
   // Overridden from Value:
   virtual BinaryValue* DeepCopy() const OVERRIDE;
   virtual bool Equals(const Value* other) const OVERRIDE;
 
  private:
-  // Constructor is private so that only objects with valid buffer pointers
-  // and size values can be created.
-  BinaryValue(char* buffer, size_t size);
-
-  char* buffer_;
+  scoped_ptr<char[]> buffer_;
   size_t size_;
 
   DISALLOW_COPY_AND_ASSIGN(BinaryValue);
diff --git a/base/values_unittest.cc b/base/values_unittest.cc
index 8a42f78..98bc73c 100644
--- a/base/values_unittest.cc
+++ b/base/values_unittest.cc
@@ -105,23 +105,15 @@ TEST(ValuesTest, List) {
 }
 
 TEST(ValuesTest, BinaryValue) {
-  char* buffer = NULL;
-  // Passing a null buffer pointer doesn't yield a BinaryValue
-  scoped_ptr<BinaryValue> binary(BinaryValue::Create(buffer, 0));
-  ASSERT_FALSE(binary.get());
-
-  // If you want to represent an empty binary value, use a zero-length buffer.
-  buffer = new char[1];
-  ASSERT_TRUE(buffer);
-  binary.reset(BinaryValue::Create(buffer, 0));
+  // Default constructor creates a BinaryValue with a null buffer and size 0.
+  scoped_ptr<BinaryValue> binary(new BinaryValue());
   ASSERT_TRUE(binary.get());
-  ASSERT_TRUE(binary->GetBuffer());
-  ASSERT_EQ(buffer, binary->GetBuffer());
+  ASSERT_EQ(NULL, binary->GetBuffer());
   ASSERT_EQ(0U, binary->GetSize());
 
   // Test the common case of a non-empty buffer
-  buffer = new char[15];
-  binary.reset(BinaryValue::Create(buffer, 15));
+  char* buffer = new char[15];
+  binary.reset(new BinaryValue(scoped_ptr<char[]>(buffer), 15));
   ASSERT_TRUE(binary.get());
   ASSERT_TRUE(binary->GetBuffer());
   ASSERT_EQ(buffer, binary->GetBuffer());
@@ -348,9 +340,9 @@ TEST(ValuesTest, DeepCopy) {
   StringValue* original_string16 = new StringValue(ASCIIToUTF16("hello16"));
   original_dict.Set("string16", original_string16);
 
-  char* original_buffer = new char[42];
-  memset(original_buffer, '!', 42);
-  BinaryValue* original_binary = BinaryValue::Create(original_buffer, 42);
+  scoped_ptr<char[]> original_buffer(new char[42]);
+  memset(original_buffer.get(), '!', 42);
+  BinaryValue* original_binary = new BinaryValue(original_buffer.Pass(), 42);
   original_dict.Set("binary", original_binary);
 
   ListValue* original_list = new ListValue();
@@ -555,9 +547,9 @@ TEST(ValuesTest, DeepCopyCovariantReturnTypes) {
   StringValue* original_string16 = new StringValue(ASCIIToUTF16("hello16"));
   original_dict.Set("string16", original_string16);
 
-  char* original_buffer = new char[42];
-  memset(original_buffer, '!', 42);
-  BinaryValue* original_binary = BinaryValue::Create(original_buffer, 42);
+  scoped_ptr<char[]> original_buffer(new char[42]);
+  memset(original_buffer.get(), '!', 42);
+  BinaryValue* original_binary = new BinaryValue(original_buffer.Pass(), 42);
   original_dict.Set("binary", original_binary);
 
   ListValue* original_list = new ListValue();
diff --git a/chrome/browser/extensions/api/bluetooth/bluetooth_api.cc b/chrome/browser/extensions/api/bluetooth/bluetooth_api.cc
index c983544..9517bc6 100644
--- a/chrome/browser/extensions/api/bluetooth/bluetooth_api.cc
+++ b/chrome/browser/extensions/api/bluetooth/bluetooth_api.cc
@@ -393,7 +393,9 @@ void BluetoothReadFunction::Work() {
 
   if (total_bytes_read > 0) {
     success_ = true;
-    SetResult(base::BinaryValue::Create(all_bytes, total_bytes_read));
+    SetResult(base::BinaryValue::CreateWithCopiedBuffer(all_bytes,
+        total_bytes_read));
+    free(all_bytes);
   } else {
     success_ = (errsv == EAGAIN || errsv == EWOULDBLOCK);
     free(all_bytes);
diff --git a/chrome/browser/extensions/api/socket/socket_api.cc b/chrome/browser/extensions/api/socket/socket_api.cc
index 3afcee4c..d0ae519 100644
--- a/chrome/browser/extensions/api/socket/socket_api.cc
+++ b/chrome/browser/extensions/api/socket/socket_api.cc
@@ -382,9 +382,7 @@ void SocketReadFunction::OnCompleted(int bytes_read,
                 base::BinaryValue::CreateWithCopiedBuffer(io_buffer->data(),
                                                           bytes_read));
   } else {
-    // BinaryValue does not support NULL buffer. Workaround it with new char[1].
-    // http://crbug.com/127630
-    result->Set(kDataKey, base::BinaryValue::Create(new char[1], 0));
+    result->Set(kDataKey, new base::BinaryValue());
   }
   SetResult(result);
 
@@ -465,9 +463,7 @@ void SocketRecvFromFunction::OnCompleted(int bytes_read,
                 base::BinaryValue::CreateWithCopiedBuffer(io_buffer->data(),
                                                           bytes_read));
   } else {
-    // BinaryValue does not support NULL buffer. Workaround it with new char[1].
-    // http://crbug.com/127630
-    result->Set(kDataKey, base::BinaryValue::Create(new char[1], 0));
+    result->Set(kDataKey, new base::BinaryValue());
   }
   result->SetString(kAddressKey, address);
   result->SetInteger(kPortKey, port);