bindings: Use V8 MaybeLocal<> APIs in Dictionary class

Source/bindings/core/v8/Dictionary.cpp

Index: Source/bindings/core/v8/Dictionary.cpp
diff --git a/Source/bindings/core/v8/Dictionary.cpp b/Source/bindings/core/v8/Dictionary.cpp
index 05fe43e352b69434967c337dabad318385e361c7..82aaa614fca5eb719fd51858eac560e69b495050 100644
--- a/Source/bindings/core/v8/Dictionary.cpp
+++ b/Source/bindings/core/v8/Dictionary.cpp
@@ -107,33 +107,30 @@ bool Dictionary::isUndefinedOrNull() const
 
 bool Dictionary::hasProperty(const String& key) const
 {
-    if (isUndefinedOrNull())
+    v8::Local<v8::Object> object;
+    if (!toObject(&object))
         return false;
-    v8::Local<v8::Object> options = m_options->ToObject(m_isolate);
-    ASSERT(!options.IsEmpty());
 
     ASSERT(m_isolate);
     ASSERT(m_isolate == v8::Isolate::GetCurrent());
     ASSERT(m_exceptionState);
-    v8::Handle<v8::String> v8Key = v8String(m_isolate, key);
-    return v8CallBoolean(options->Has(v8Context(), v8Key));
+    v8::Local<v8::String> v8Key = v8String(m_isolate, key);
+    return v8CallBoolean(object->Has(v8Context(), v8Key));
 }
 
 bool Dictionary::getKey(const String& key, v8::Local<v8::Value>& value) const
 {
-    if (isUndefinedOrNull())
+    v8::Local<v8::Object> object;
+    if (!toObject(&object))
         return false;
-    v8::Local<v8::Object> options = m_options->ToObject(m_isolate);
-    ASSERT(!options.IsEmpty());
 
     ASSERT(m_isolate);
     ASSERT(m_isolate == v8::Isolate::GetCurrent());
     ASSERT(m_exceptionState);
-    v8::Handle<v8::String> v8Key = v8String(m_isolate, key);
-    if (!v8CallBoolean(options->Has(v8Context(), v8Key)))
+    v8::Local<v8::String> v8Key = v8String(m_isolate, key);
+    if (!v8CallBoolean(object->Has(v8Context(), v8Key)))
         return false;
-    value = options->Get(v8Key);
-    return !value.IsEmpty();
+    return object->Get(v8Context(), v8Key).ToLocal(&value);
 }
 
 bool Dictionary::get(const String& key, v8::Local<v8::Value>& value) const
@@ -174,24 +171,33 @@ bool Dictionary::convert(ConversionContext& context, const String& key, Dictiona
     return false;
 }
 
-bool Dictionary::getOwnPropertiesAsStringHashMap(HashMap<String, String>& hashMap) const
+static inline bool propertyKey(v8::Local<v8::Context> v8Context, v8::Local<v8::Array> properties, uint32_t index, v8::Local<v8::String>& key)
 {
-    if (!isObject())
+    v8::Local<v8::Value> property;
+    if (!properties->Get(v8Context, index).ToLocal(&property))
         return false;
+    return property->ToString(v8Context).ToLocal(&key);
+}
 
-    v8::Handle<v8::Object> options = m_options->ToObject(m_isolate);
-    if (options.IsEmpty())
+bool Dictionary::getOwnPropertiesAsStringHashMap(HashMap<String, String>& hashMap) const
+{
+    v8::Local<v8::Object> object;
+    if (!toObject(&object))
         return false;
 
     v8::Local<v8::Array> properties;
-    if (!options->GetOwnPropertyNames(v8Context()).ToLocal(&properties))
+    if (!object->GetOwnPropertyNames(v8Context()).ToLocal(&properties))
         return true;
     for (uint32_t i = 0; i < properties->Length(); ++i) {
-        v8::Local<v8::String> key = properties->Get(i)->ToString(m_isolate);
-        if (!v8CallBoolean(options->Has(v8Context(), key)))
+        v8::Local<v8::String> key;
+        if (!propertyKey(v8Context(), properties, i, key))
+            continue;
+        if (!v8CallBoolean(object->Has(v8Context(), key)))
             continue;
 
-        v8::Local<v8::Value> value = options->Get(key);
+        v8::Local<v8::Value> value;
+        if (!object->Get(v8Context(), key).ToLocal(&value))
+            continue;
         TOSTRING_DEFAULT(V8StringResource<>, stringKey, key, false);
         TOSTRING_DEFAULT(V8StringResource<>, stringValue, value, false);
         if (!static_cast<const String&>(stringKey).isEmpty())
@@ -203,19 +209,18 @@ bool Dictionary::getOwnPropertiesAsStringHashMap(HashMap<String, String>& hashMa
 
 bool Dictionary::getPropertyNames(Vector<String>& names) const
 {
-    if (!isObject())
+    v8::Local<v8::Object> object;
+    if (!toObject(&object))
         return false;
 
-    v8::Handle<v8::Object> options = m_options->ToObject(m_isolate);
-    if (options.IsEmpty())
-        return false;
-
-    v8::Local<v8::Array> properties = options->GetPropertyNames();
-    if (properties.IsEmpty())
+    v8::Local<v8::Array> properties;
+    if (!object->GetPropertyNames(v8Context()).ToLocal(&properties))
         return true;

[haraken, 2015/03/27 03:33:33]

I'm not sure if this should always be true. If the empty handle means that there
is no property name, then it should be true. However, if the empty handle means
that some exception was thrown, then it should be false.

In other words, this V8 API breaks the invariant that an empty handle is
returned if and only if an exception was thrown. I guess we need to redesign the
API?

[bashi, 2015/03/27 03:55:24]

Yeah, I thought it was inconsistent, but kept the current behavior. Let's try to
negate this.

[haraken, 2015/03/27 03:59:24]

I'm not sure if this is right. If there is no property name in the object, I
think you must return true here.

I guess we cannot fix this issue without fixing the V8 API.

[bashi, 2015/03/27 04:34:25]

GetPropertyNames() calls NewJSArray() regardless there is no property name. I
read this as "GetPropertyNames() returns an empty array (not an empty handle)
when there is no property name". So I think this is fine.

@dcanery, could you confirm?

     for (uint32_t i = 0; i < properties->Length(); ++i) {
-        v8::Local<v8::String> key = properties->Get(i)->ToString(m_isolate);
-        if (!v8CallBoolean(options->Has(v8Context(), key)))
+        v8::Local<v8::String> key;
+        if (!propertyKey(v8Context(), properties, i, key))
+            continue;
+        if (!v8CallBoolean(object->Has(v8Context(), key)))
             continue;
         TOSTRING_DEFAULT(V8StringResource<>, stringKey, key, false);
         names.append(stringKey);
@@ -248,4 +253,9 @@ void Dictionary::ConversionContext::throwTypeError(const String& detail)
     exceptionState().throwTypeError(detail);
 }
 
+bool Dictionary::toObject(v8::Local<v8::Object>* object) const

[haraken, 2015/03/27 03:33:33]

Can we change v8::Local<v8::Object>* to v8::Local<v8::Object>? You don't need to
worry about the copying overhead since v8::Local<v8::Object> just holds a
pointer to the v8::Object.

We basically don't want to use v8::Local<v8::Object>* in the binding layer to
avoid complexity.

[bashi, 2015/03/27 03:55:24]

|object| is an out parameter, as ToLocal takes a pointer. We can use reference
here (like other blink code), but I just followed what ToLocal does.

+{
+    return !isUndefinedOrNull() && m_options->ToObject(v8Context()).ToLocal(object);
+}
+
 } // namespace blink