bindings: Use V8 MaybeLocal<> APIs in Dictionary class

Source/bindings/core/v8/Dictionary.cpp

 /*
  * Copyright (C) 2010 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 89 matching lines @@
 
 bool Dictionary::isUndefinedOrNull() const
 {
     if (m_options.IsEmpty())
         return true;
     return blink::isUndefinedOrNull(m_options);
 }
 
 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
 {
     return getKey(key, value);
 }
 
 bool Dictionary::get(const String& key, Dictionary& value) const
 {
     v8::Local<v8::Value> v8Value;
@@ skipping 20 matching lines @@
     if (v8Value->IsObject())
         return get(key, value);
 
     if (context.isNullable() && blink::isUndefinedOrNull(v8Value))
         return true;
 
     context.throwTypeError(ExceptionMessages::incorrectPropertyType(key, "does not have a Dictionary type."));
     return false;
 }
 
+static inline bool propertyKey(v8::Local<v8::Context> v8Context, v8::Local<v8::Array> properties, uint32_t index, v8::Local<v8::String>& key)
+{
+    v8::Local<v8::Value> property;
+    if (!properties->Get(v8Context, index).ToLocal(&property))
+        return false;
+    return property->ToString(v8Context).ToLocal(&key);
+}
+
 bool Dictionary::getOwnPropertiesAsStringHashMap(HashMap<String, String>& hashMap) const
 {
-    if (!isObject())
-        return false;
-
-    v8::Handle<v8::Object> options = m_options->ToObject(m_isolate);
-    if (options.IsEmpty())
+    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())
             hashMap.set(stringKey, stringValue);
     }
 
     return true;
 }
 
 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);
     }
 
     return true;
 }
 
 void Dictionary::ConversionContext::resetPerPropertyContext()
 {
@@ skipping 12 matching lines @@
     m_propertyTypeName = typeName;
 
     return *this;
 }
 
 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