Convert DOMWrapperMap to use PersistentValueMap

Source/bindings/v8/DOMWrapperMap.h

Index: Source/bindings/v8/DOMWrapperMap.h
diff --git a/Source/bindings/v8/DOMWrapperMap.h b/Source/bindings/v8/DOMWrapperMap.h
index 20fe7625f0422e27be900690c3e6efb90f0c9a62..c2a29bcd746067b25bd0860993127b19f933ad16 100644
--- a/Source/bindings/v8/DOMWrapperMap.h
+++ b/Source/bindings/v8/DOMWrapperMap.h
@@ -32,104 +32,171 @@
 #define DOMWrapperMap_h
 
 #include "bindings/v8/UnsafePersistent.h"
+#include "bindings/v8/V8NPObject.h"
 #include "bindings/v8/WrapperTypeInfo.h"
 #include <v8.h>
 #include "wtf/HashMap.h"
+#include <stdio.h>
 
 namespace WebCore {
 
 template<class KeyType>
 class DOMWrapperMap {
 public:
-    typedef HashMap<KeyType*, UnsafePersistent<v8::Object> > MapType;
-
     explicit DOMWrapperMap(v8::Isolate* isolate)
         : m_isolate(isolate)
+        , m_map(isolate)
     {
     }
 
     v8::Handle<v8::Object> newLocal(KeyType* key, v8::Isolate* isolate)
     {
-        return m_map.get(key).newLocal(isolate);
+        return v8::Local<v8::Object>::New(isolate, m_map.Get(key));

[dcarney, 2014/03/07 09:38:24]

Get already returns a local, no need to double wrap

[vogelheim, 2014/03/07 18:01:34]

Done.

     }
 
     bool setReturnValueFrom(v8::ReturnValue<v8::Value> returnValue, KeyType* key)
     {
-        typename MapType::iterator it = m_map.find(key);
-        if (it == m_map.end())
-            return false;
-        returnValue.Set(*(it->value.persistent()));
-        return true;
+        v8::Local<v8::Object> value = m_map.Get(key);

[dcarney, 2014/03/07 09:38:24]

we don't want to create a local for setting a return value - too expensive. 
you'll need to push setReturnValue down to PersistentValueMap

[vogelheim, 2014/03/07 18:01:34]

Done.

+        bool hasValue = !value.IsEmpty();
+        if (hasValue) {
+            returnValue.Set(value);
+        }
+        return hasValue;
     }
 
     void setReference(const v8::Persistent<v8::Object>& parent, KeyType* key, v8::Isolate* isolate)
     {
-        m_map.get(key).setReferenceFrom(parent, isolate);
+        v8::Persistent<v8::Object> ref(isolate, m_map.Get(key));

[dcarney, 2014/03/07 09:38:24]

push to PersistentValueMap

[vogelheim, 2014/03/07 18:01:34]

Done.

+        isolate->SetReference(parent, ref);
     }
 
     bool containsKey(KeyType* key)
     {
-        return m_map.find(key) != m_map.end();
+        return !m_map.Get(key).IsEmpty();

[dcarney, 2014/03/07 09:38:24]

too expensive - push to PersistentValueMap

[vogelheim, 2014/03/07 18:01:34]

Done.

     }
 
     bool containsKeyAndValue(KeyType* key, v8::Handle<v8::Object> value)
     {
-        typename MapType::iterator it = m_map.find(key);
-        if (it == m_map.end())
-            return false;
-        return *(it->value.persistent()) == value;
+        v8::Local<v8::Object> mapValue = m_map.Get(key);

[dcarney, 2014/03/07 09:38:24]

this function is stupid and only used in an assert in npobject - move it there,
maybe in another quick cl first

[vogelheim, 2014/03/07 18:01:34]

Done.

Actually, it drops out with this change, since that code that calls it no longer
gets to see the values its currently asserting on.

+        return !mapValue.IsEmpty() && (mapValue == value);
     }
 
     void set(KeyType* key, v8::Handle<v8::Object> wrapper, const WrapperConfiguration& configuration)
     {
         ASSERT(static_cast<KeyType*>(toNative(wrapper)) == key);
-        v8::Persistent<v8::Object> persistent(m_isolate, wrapper);
-        configuration.configureWrapper(&persistent);
-        persistent.SetWeak(this, &setWeakCallback);
-        typename MapType::AddResult result = m_map.add(key, UnsafePersistent<v8::Object>());
-        ASSERT(result.isNewEntry);
-        // FIXME: Stop handling this case once duplicate wrappers are guaranteed not to be created.
-        if (!result.isNewEntry)
-            result.storedValue->value.dispose();
-        result.storedValue->value = UnsafePersistent<v8::Object>(persistent);
+        ASSERT(!wrapper.IsEmpty());

[dcarney, 2014/03/07 09:38:24]

this is asserted in the line above, drop

[vogelheim, 2014/03/07 18:01:34]

Done.

+        m_map.SetWithConfig(key, v8::Local<v8::Object>::New(m_isolate, wrapper),

[dcarney, 2014/03/07 09:38:24]

no need for local::new

[vogelheim, 2014/03/07 18:01:34]

Done.

+            configuration.classId, configuration.lifetime);
     }
 
     void clear()
     {
-        v8::HandleScope scope(m_isolate);
-        while (!m_map.isEmpty()) {
-            // Swap out m_map on each iteration to ensure any wrappers added due to side effects of the loop are cleared.
-            MapType map;
-            map.swap(m_map);
-            for (typename MapType::iterator it = map.begin(); it != map.end(); ++it) {
-                releaseObject(it->value.newLocal(m_isolate));
-                it->value.dispose();
-            }
-        }
+        m_map.Clear();
     }
 
     void removeAndDispose(KeyType* key)
     {
-        typename MapType::iterator it = m_map.find(key);
-        ASSERT_WITH_SECURITY_IMPLICATION(it != m_map.end());
-        it->value.dispose();
-        m_map.remove(it);
+        m_map.Remove(key);
     }
 
 private:
-    static void setWeakCallback(const v8::WeakCallbackData<v8::Object, DOMWrapperMap<KeyType> >&);
+    class PersistentValueMapTraits {
+    public:
+        typedef HashMap<KeyType*, v8::PersistentContainerValue> Impl;
+        typedef typename Impl::iterator Iterator;
+
+        class WeakCallbackDataType {

[dcarney, 2014/03/07 09:38:24]

WeakCallbackDataType should just be a point to the DOMWrapperMap.  everything
else is available in the callback

[vogelheim, 2014/03/07 18:01:34]

Done.

I find this a weird design, though. This client - DOMWrapperMap - can derive
keys from values and vice versa. It mostly pretends that keys and values are
separate things; but the assert in set(..) ensures this behaviour.

The way it's implemented now, this decision is imported into PersitentValueMap:
Traits::KeyFromWeakCallbackData - which contains the isolate, the impl, and the
value, but not the key - is only implementable for maps which have the same
property.

Am I overlooking something?

+        public:
+            WeakCallbackDataType(Impl* impl, KeyType* key, v8::Local<v8::Object>& value)
+                : m_impl(impl)
+                , m_key(key)
+                , m_value(value)
+            { }
+            Impl* m_impl;
+            KeyType* m_key;
+            v8::Local<v8::Object> m_value;
+        };
+
+        static const bool kIsWeak = true;
+
+        // Container API: Size/Empty/Swap/Begin/End/Value/Set/Get/Remove
+        static size_t Size(const Impl* impl) { return impl->size(); }
+        static bool Empty(Impl* impl) { return impl->isEmpty(); }
+        static void Swap(Impl& impl, Impl& other) { impl.swap(other); }
+
+        static Iterator Begin(Impl* impl) { return impl->begin(); }
+        static Iterator End(Impl* impl) { return impl->end(); }
+        static v8::PersistentContainerValue Value(Iterator& iter)
+        {
+            return iter->value;
+        }
+        static KeyType* Key(Iterator& iter) { return iter->key; }
+
+        static v8::PersistentContainerValue Set(
+            Impl* impl, KeyType* key, v8::PersistentContainerValue value)
+        {
+            v8::PersistentContainerValue oldValue = Get(impl, key);

[dcarney, 2014/03/07 09:38:24]

this function seems inefficient - is there a way to do it more direrctly?

[vogelheim, 2014/03/07 18:01:34]

Done.

+            impl->add(key, value);
+            return oldValue;
+        }
+
+        static v8::PersistentContainerValue Get(const Impl* impl, KeyType* key)
+        {
+            return impl->get(key);
+        }
+
+        static v8::PersistentContainerValue Remove(Impl* impl, KeyType* key)
+        {
+            return impl->take(key);
+        }
+
+        // Dispose callbacks:
+        static void DisposeByValue(v8::UniquePersistent<v8::Object> value, v8::Isolate* isolate) { }
+        static void DisposeByKey(Impl* impl, KeyType* key) { }
+
+        // WeakCallbackDataType, create/dispose, conversion, callback.
+        static WeakCallbackDataType* WeakCallbackParameter(Impl* impl, KeyType* key, v8::Local<v8::Object>& value)
+        {
+            // ??? value is unused?
+            return new WeakCallbackDataType(impl, key, value);

[dcarney, 2014/03/07 09:38:24]

just return 'this'

[vogelheim, 2014/03/07 18:01:34]

Done.

(Actually impl, but hat's probably what you meant.)

+        }
+
+        static void DisposeCallbackData(WeakCallbackDataType* callbackData)
+        {
+            delete callbackData;

[dcarney, 2014/03/07 09:38:24]

should be empty

[vogelheim, 2014/03/07 18:01:34]

Done.

+        }
+
+        static Impl* ImplFromWeakCallbackData(
+            v8::WeakCallbackData<v8::Object, WeakCallbackDataType> data)
+        {
+            return data.GetParameter()->m_impl;
+        }
+
+        static KeyType* KeyFromWeakCallbackData(
+            v8::WeakCallbackData<v8::Object, WeakCallbackDataType> data)
+        {
+            return data.GetParameter()->m_key;
+        }
+    };
 
     v8::Isolate* m_isolate;
-    MapType m_map;
+    v8::PersistentValueMap<KeyType*, v8::Object, PersistentValueMapTraits> m_map;
 };
 
-template<>
-inline void DOMWrapperMap<void>::setWeakCallback(const v8::WeakCallbackData<v8::Object, DOMWrapperMap<void> >& data)
+template <>
+inline void DOMWrapperMap<void>::PersistentValueMapTraits::DisposeByValue(
+    v8::UniquePersistent<v8::Object> value,
+    v8::Isolate* isolate)
+{
+    releaseObject(v8::Local<v8::Object>::New(isolate, value));
+}
+
+template <>
+inline void DOMWrapperMap<NPObject>::PersistentValueMapTraits::DisposeByKey(
+    DOMWrapperMap<NPObject>::PersistentValueMapTraits::Impl* impl,
+    NPObject* key)

[dcarney, 2014/03/07 09:38:24]

this needs to go to the file where it's currently used

[vogelheim, 2014/03/07 18:01:34]

Done.

For my understanding: Why? I thought it rather surprising to have a partial
implementation for DOMWrapperMap in a different file; hence my change.

 {
-    void* key = static_cast<void*>(toNative(data.GetValue()));
-    ASSERT(*data.GetParameter()->m_map.get(key).persistent() == data.GetValue());
-    data.GetParameter()->removeAndDispose(key);
-    releaseObject(data.GetValue());
+    DOMWrapperMapDisposeHelper(key);
 }
 
 } // namespace WebCore