Revert 157025 - Make V8ValueConverter.FromV8Value be more consistent with JSON.stringify: don't

serialize undefined as null, don't serialize functions as objects, omit values
from objects when they don't serialize, and insert null into arrays when they
don't serialize.

It is now possible for FromV8Value to return NULL; previously it would return
Value::CreateNullValue on failure.

This is needed for the Storage API, where we promise that the values that are
passed in are serialized as JSON, yet the value conversion doesn't work in a
way that allows it.

However, the null-stripping behavior needs to be configurable so that existing
extension APIs (which only expect null/undefined to appear for optional values)
still work.

BUG=145081

Review URL: https://codereview.chromium.org/10890002

TBR=kalman@chromium.org
Review URL: https://codereview.chromium.org/10911327

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

5 files changed, 87 insertions, 168 deletions

diff --git a/content/public/renderer/v8_value_converter.h b/content/public/renderer/v8_value_converter.h
index b71b32d..be45ea9 100644
--- a/content/public/renderer/v8_value_converter.h
+++ b/content/public/renderer/v8_value_converter.h
@@ -28,48 +28,31 @@ class CONTENT_EXPORT V8ValueConverter {
 
   virtual ~V8ValueConverter() {}
 
-  // If true, Date objects are converted into DoubleValues with the number of
-  // seconds since Unix epoch.
-  //
-  // Otherwise they are converted into DictionaryValues with whatever additional
-  // properties has been set on them.
-  virtual void SetDateAllowed(bool val) = 0;
+  // Use the following setters to support additional types other than the
+  // default ones.
+  virtual bool GetUndefinedAllowed() const = 0;
+  virtual void SetUndefinedAllowed(bool val) = 0;
 
-  // If true, RegExp objects are converted into StringValues with the regular
-  // expression between / and /, for example "/ab?c/".
-  //
-  // Otherwise they are converted into DictionaryValues with whatever additional
-  // properties has been set on them.
-  virtual void SetRegExpAllowed(bool val) = 0;
+  virtual bool GetDateAllowed() const = 0;
+  virtual void SetDateAllowed(bool val) = 0;
 
-  // If true, Function objects are converted into DictionaryValues with whatever
-  // additional properties has been set on them.
-  //
-  // Otherwise they are treated as unsupported, see FromV8Value.
-  virtual void SetFunctionAllowed(bool val) = 0;
+  virtual bool GetRegexpAllowed() const = 0;
+  virtual void SetRegexpAllowed(bool val) = 0;
 
-  // If true, null values are stripped from objects. This is often useful when
-  // converting arguments to extension APIs.
+  // Gets/sets whether to treat undefined or null in objects as nonexistent.
+  virtual bool GetStripNullFromObjects() const = 0;
   virtual void SetStripNullFromObjects(bool val) = 0;
 
-  // Converts a base::Value to a v8::Value.
-  //
-  // Unsupported types are replaced with null.  If an array or object throws
-  // while setting a value, that property or item is skipped, leaving a hole in
-  // the case of arrays.
+  // Converts Value to v8::Value. Unsupported types are replaced with null.
+  // If an array or object throws while setting a value, that property or item
+  // is skipped, leaving a hole in the case of arrays.
   virtual v8::Handle<v8::Value> ToV8Value(
       const base::Value* value,
       v8::Handle<v8::Context> context) const = 0;
 
-  // Converts a v8::Value to base::Value.
-  //
-  // Unsupported types (unless explicitly configured) are not converted, so
-  // this method may return NULL -- the exception is when converting arrays,
-  // where unsupported types are converted to Value(TYPE_NULL).
-  //
-  // Likewise, if an object throws while converting a property it will not be
-  // converted, whereas if an array throws while converting an item it will be
-  // converted to Value(TYPE_NULL).
+  // Converts v8::Value to Value. Unsupported types are replaced with null.
+  // If an array or object throws while getting a value, that property or item
+  // is replaced with null.
   virtual base::Value* FromV8Value(v8::Handle<v8::Value> value,
                                    v8::Handle<v8::Context> context) const = 0;
 };
diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc
index 224cb16..8e473184 100644
--- a/content/renderer/render_view_impl.cc
+++ b/content/renderer/render_view_impl.cc
@@ -4219,6 +4219,7 @@ WebKit::WebPlugin* RenderViewImpl::CreatePlugin(
         pepper_module.get(), params, pepper_delegate_.AsWeakPtr());
   }
 
+
 #if defined(USE_AURA) && !defined(OS_WIN)
   return NULL;
 #else
@@ -4243,10 +4244,8 @@ void RenderViewImpl::EvaluateScript(const string16& frame_xpath,
       v8::Context::Scope context_scope(context);
       V8ValueConverterImpl converter;
       converter.SetDateAllowed(true);
-      converter.SetRegExpAllowed(true);
-      base::Value* result_value = converter.FromV8Value(result, context);
-      list.Set(0, result_value ? result_value :
-                                 base::Value::CreateNullValue());
+      converter.SetRegexpAllowed(true);
+      list.Set(0, converter.FromV8Value(result, context));
     } else {
       list.Set(0, Value::CreateNullValue());
     }
diff --git a/content/renderer/v8_value_converter_impl.cc b/content/renderer/v8_value_converter_impl.cc
index 4c2130d..8d55ecc 100644
--- a/content/renderer/v8_value_converter_impl.cc
+++ b/content/renderer/v8_value_converter_impl.cc
@@ -27,22 +27,38 @@ V8ValueConverter* V8ValueConverter::create() {
 }  // namespace content
 
 V8ValueConverterImpl::V8ValueConverterImpl()
-    : date_allowed_(false),
-      reg_exp_allowed_(false),
-      function_allowed_(false),
+    : undefined_allowed_(false),
+      date_allowed_(false),
+      regexp_allowed_(false),
       strip_null_from_objects_(false) {
 }
 
+bool V8ValueConverterImpl::GetUndefinedAllowed() const {
+  return undefined_allowed_;
+}
+
+void V8ValueConverterImpl::SetUndefinedAllowed(bool val) {
+  undefined_allowed_ = val;
+}
+
+bool V8ValueConverterImpl::GetDateAllowed() const {
+  return date_allowed_;
+}
+
 void V8ValueConverterImpl::SetDateAllowed(bool val) {
   date_allowed_ = val;
 }
 
-void V8ValueConverterImpl::SetRegExpAllowed(bool val) {
-  reg_exp_allowed_ = val;
+bool V8ValueConverterImpl::GetRegexpAllowed() const {
+  return regexp_allowed_;
+}
+
+void V8ValueConverterImpl::SetRegexpAllowed(bool val) {
+  regexp_allowed_ = val;
 }
 
-void V8ValueConverterImpl::SetFunctionAllowed(bool val) {
-  function_allowed_ = val;
+bool V8ValueConverterImpl::GetStripNullFromObjects() const {
+  return strip_null_from_objects_;
 }
 
 void V8ValueConverterImpl::SetStripNullFromObjects(bool val) {
@@ -184,37 +200,23 @@ Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val,
     return Value::CreateStringValue(std::string(*utf8, utf8.length()));
   }
 
-  if (val->IsUndefined())
-    // JSON.stringify ignores undefined.
-    return NULL;
+  if (undefined_allowed_ && val->IsUndefined())
+    return Value::CreateNullValue();
 
-  if (val->IsDate()) {
-    if (!date_allowed_)
-      // JSON.stringify would convert this to a string, but an object is more
-      // consistent within this class.
-      return FromV8Object(val->ToObject(), unique_set);
+  if (date_allowed_ && val->IsDate()) {
     v8::Date* date = v8::Date::Cast(*val);
     return Value::CreateDoubleValue(date->NumberValue() / 1000.0);
   }
 
-  if (val->IsRegExp()) {
-    if (!reg_exp_allowed_)
-      // JSON.stringify converts to an object.
-      return FromV8Object(val->ToObject(), unique_set);
-    return Value::CreateStringValue(*v8::String::Utf8Value(val->ToString()));
+  if (regexp_allowed_ && val->IsRegExp()) {
+    return Value::CreateStringValue(
+        *v8::String::Utf8Value(val->ToString()));
   }
 
   // v8::Value doesn't have a ToArray() method for some reason.
   if (val->IsArray())
     return FromV8Array(val.As<v8::Array>(), unique_set);
 
-  if (val->IsFunction()) {
-    if (!function_allowed_)
-      // JSON.stringify refuses to convert function(){}.
-      return NULL;
-    return FromV8Object(val->ToObject(), unique_set);
-  }
-
   if (val->IsObject()) {
     BinaryValue* binary_value = FromV8Buffer(val);
     if (binary_value) {
@@ -223,9 +225,8 @@ Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val,
       return FromV8Object(val->ToObject(), unique_set);
     }
   }
-
   LOG(ERROR) << "Unexpected v8 value type encountered.";
-  return NULL;
+  return Value::CreateNullValue();
 }
 
 Value* V8ValueConverterImpl::FromV8Array(v8::Handle<v8::Array> val,
@@ -257,12 +258,9 @@ Value* V8ValueConverterImpl::FromV8Array(v8::Handle<v8::Array> val,
       continue;
 
     Value* child = FromV8ValueImpl(child_v8, unique_set);
-    if (child)
-      result->Append(child);
-    else
-      // JSON.stringify puts null in places where values don't serialize, for
-      // example undefined and functions. Emulate that behavior.
-      result->Append(Value::CreateNullValue());
+    CHECK(child);
+
+    result->Append(child);
   }
   return result;
 }
@@ -337,10 +335,7 @@ Value* V8ValueConverterImpl::FromV8Object(
     }
 
     scoped_ptr<Value> child(FromV8ValueImpl(child_v8, unique_set));
-    if (!child.get())
-      // JSON.stringify skips properties whose values don't serialize, for
-      // example undefined and functions. Emulate that behavior.
-      continue;
+    CHECK(child.get());
 
     // Strip null if asked (and since undefined is turned into null, undefined
     // too). The use case for supporting this is JSON-schema support,
diff --git a/content/renderer/v8_value_converter_impl.h b/content/renderer/v8_value_converter_impl.h
index c6b9511..baf061a 100644
--- a/content/renderer/v8_value_converter_impl.h
+++ b/content/renderer/v8_value_converter_impl.h
@@ -23,9 +23,13 @@ class CONTENT_EXPORT V8ValueConverterImpl : public content::V8ValueConverter {
   V8ValueConverterImpl();
 
   // V8ValueConverter implementation.
+  virtual bool GetUndefinedAllowed() const OVERRIDE;
+  virtual void SetUndefinedAllowed(bool val) OVERRIDE;
+  virtual bool GetDateAllowed() const OVERRIDE;
   virtual void SetDateAllowed(bool val) OVERRIDE;
-  virtual void SetRegExpAllowed(bool val) OVERRIDE;
-  virtual void SetFunctionAllowed(bool val) OVERRIDE;
+  virtual bool GetRegexpAllowed() const OVERRIDE;
+  virtual void SetRegexpAllowed(bool val) OVERRIDE;
+  virtual bool GetStripNullFromObjects() const OVERRIDE;
   virtual void SetStripNullFromObjects(bool val) OVERRIDE;
   virtual v8::Handle<v8::Value> ToV8Value(
       const base::Value* value,
@@ -54,14 +58,14 @@ class CONTENT_EXPORT V8ValueConverterImpl : public content::V8ValueConverter {
   base::Value* FromV8Object(v8::Handle<v8::Object> object,
                             std::set<int>* unique_set) const;
 
+  // If true, we will convert undefined JavaScript values to null.
+  bool undefined_allowed_;
+
   // If true, we will convert Date JavaScript objects to doubles.
   bool date_allowed_;
 
-  // If true, we will convert RegExp JavaScript objects to string.
-  bool reg_exp_allowed_;
-
-  // If true, we will convert Function JavaScript objects to dictionaries.
-  bool function_allowed_;
+  // If true, we will convet RegExp JavaScript objects to string.
+  bool regexp_allowed_;
 
   // If true, undefined and null values are ignored when converting v8 objects
   // into Values.
diff --git a/content/renderer/v8_value_converter_impl_unittest.cc b/content/renderer/v8_value_converter_impl_unittest.cc
index b4b12bd..9146fb3 100644
--- a/content/renderer/v8_value_converter_impl_unittest.cc
+++ b/content/renderer/v8_value_converter_impl_unittest.cc
@@ -10,8 +10,6 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "v8/include/v8.h"
 
-using content::V8ValueConverter;
-
 namespace {
 
 // A dumb getter for an object's named callback.
@@ -114,14 +112,10 @@ class V8ValueConverterImplTest : public testing::Test {
                      base::Value::Type expected_type,
                      scoped_ptr<Value> expected_value) {
     scoped_ptr<Value> raw(converter.FromV8Value(val, context_));
-
-    if (expected_value.get()) {
-      ASSERT_TRUE(raw.get());
+    ASSERT_TRUE(raw.get());
+    EXPECT_EQ(expected_type, raw->GetType());
+    if (expected_value.get())
       EXPECT_TRUE(expected_value->Equals(raw.get()));
-      EXPECT_EQ(expected_type, raw->GetType());
-    } else {
-      EXPECT_FALSE(raw.get());
-    }
 
     v8::Handle<v8::Object> object(v8::Object::New());
     object->Set(v8::String::New("test"), val);
@@ -130,14 +124,11 @@ class V8ValueConverterImplTest : public testing::Test {
             converter.FromV8Value(object, context_)));
     ASSERT_TRUE(dictionary.get());
 
-    if (expected_value.get()) {
-      Value* temp = NULL;
-      ASSERT_TRUE(dictionary->Get("test", &temp));
-      EXPECT_EQ(expected_type, temp->GetType());
+    Value* temp = NULL;
+    ASSERT_TRUE(dictionary->Get("test", &temp));
+    EXPECT_EQ(expected_type, temp->GetType());
+    if (expected_value.get())
       EXPECT_TRUE(expected_value->Equals(temp));
-    } else {
-      EXPECT_FALSE(dictionary->HasKey("test"));
-    }
 
     v8::Handle<v8::Array> array(v8::Array::New());
     array->Set(0, val);
@@ -145,18 +136,10 @@ class V8ValueConverterImplTest : public testing::Test {
         static_cast<ListValue*>(
             converter.FromV8Value(array, context_)));
     ASSERT_TRUE(list.get());
-    if (expected_value.get()) {
-      Value* temp = NULL;
-      ASSERT_TRUE(list->Get(0, &temp));
-      EXPECT_EQ(expected_type, temp->GetType());
+    ASSERT_TRUE(list->Get(0, &temp));
+    EXPECT_EQ(expected_type, temp->GetType());
+    if (expected_value.get())
       EXPECT_TRUE(expected_value->Equals(temp));
-    } else {
-      // Arrays should preserve their length, and convert unconvertible
-      // types into null.
-      Value* temp = NULL;
-      ASSERT_TRUE(list->Get(0, &temp));
-      EXPECT_EQ(Value::TYPE_NULL, temp->GetType());
-    }
   }
 
   // Context for the JavaScript in the test.
@@ -317,29 +300,23 @@ TEST_F(V8ValueConverterImplTest, WeirdTypes) {
       v8::RegExp::New(v8::String::New("."), v8::RegExp::kNone));
 
   V8ValueConverterImpl converter;
-  TestWeirdType(converter,
-                v8::Undefined(),
-                Value::TYPE_NULL,  // Arbitrary type, result is NULL.
+  TestWeirdType(converter, v8::Undefined(), Value::TYPE_NULL,
+                scoped_ptr<Value>(NULL));
+  TestWeirdType(converter, v8::Date::New(1000), Value::TYPE_DICTIONARY,
+                scoped_ptr<Value>(NULL));
+  TestWeirdType(converter, regex, Value::TYPE_DICTIONARY,
+                scoped_ptr<Value>(NULL));
+
+  converter.SetUndefinedAllowed(true);
+  TestWeirdType(converter, v8::Undefined(), Value::TYPE_NULL,
                 scoped_ptr<Value>(NULL));
-  TestWeirdType(converter,
-                v8::Date::New(1000),
-                Value::TYPE_DICTIONARY,
-                scoped_ptr<Value>(new DictionaryValue()));
-  TestWeirdType(converter,
-                regex,
-                Value::TYPE_DICTIONARY,
-                scoped_ptr<Value>(new DictionaryValue()));
 
   converter.SetDateAllowed(true);
-  TestWeirdType(converter,
-                v8::Date::New(1000),
-                Value::TYPE_DOUBLE,
+  TestWeirdType(converter, v8::Date::New(1000), Value::TYPE_DOUBLE,
                 scoped_ptr<Value>(Value::CreateDoubleValue(1)));
 
-  converter.SetRegExpAllowed(true);
-  TestWeirdType(converter,
-                regex,
-                Value::TYPE_STRING,
+  converter.SetRegexpAllowed(true);
+  TestWeirdType(converter, regex, Value::TYPE_STRING,
                 scoped_ptr<Value>(Value::CreateStringValue("/./")));
 }
 
@@ -376,6 +353,7 @@ TEST_F(V8ValueConverterImplTest, StripNullFromObjects) {
   ASSERT_FALSE(object.IsEmpty());
 
   V8ValueConverterImpl converter;
+  converter.SetUndefinedAllowed(true);
   converter.SetStripNullFromObjects(true);
 
   scoped_ptr<DictionaryValue> result(
@@ -507,43 +485,3 @@ TEST_F(V8ValueConverterImplTest, ArrayGetters) {
   ASSERT_TRUE(result.get());
   EXPECT_EQ(2u, result->GetSize());
 }
-
-TEST_F(V8ValueConverterImplTest, UndefinedValueBehavior) {
-  v8::Context::Scope context_scope(context_);
-  v8::HandleScope handle_scope;
-
-  v8::Handle<v8::Object> object;
-  {
-    const char* source = "(function() {"
-        "return { foo: undefined, bar: null, baz: function(){} };"
-        "})();";
-    v8::Handle<v8::Script> script(v8::Script::New(v8::String::New(source)));
-    object = script->Run().As<v8::Object>();
-    ASSERT_FALSE(object.IsEmpty());
-  }
-
-  v8::Handle<v8::Array> array;
-  {
-    const char* source = "(function() {"
-        "return [ undefined, null, function(){} ];"
-        "})();";
-    v8::Handle<v8::Script> script(v8::Script::New(v8::String::New(source)));
-    array = script->Run().As<v8::Array>();
-    ASSERT_FALSE(array.IsEmpty());
-  }
-
-  V8ValueConverterImpl converter;
-
-  DictionaryValue expected_object;
-  expected_object.Set("bar", Value::CreateNullValue());
-  scoped_ptr<Value> actual_object(converter.FromV8Value(object, context_));
-  EXPECT_TRUE(Value::Equals(&expected_object, actual_object.get()));
-
-  ListValue expected_array;
-  // Everything is null because JSON stringification preserves array length.
-  expected_array.Append(Value::CreateNullValue());
-  expected_array.Append(Value::CreateNullValue());
-  expected_array.Append(Value::CreateNullValue());
-  scoped_ptr<Value> actual_array(converter.FromV8Value(array, context_));
-  EXPECT_TRUE(Value::Equals(&expected_array, actual_array.get()));
-}