Pass isolate to ScriptPromise and ScriptString constructors

Source/bindings/v8/ScriptValue.h

 /*
  * Copyright (C) 2008, 2009 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:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 39 matching lines @@
 namespace WebCore {
 
 class JSONValue;
 class MessagePort;
 class SerializedScriptValue;
 typedef Vector<RefPtr<MessagePort>, 1> MessagePortArray;
 typedef Vector<RefPtr<WTF::ArrayBuffer>, 1> ArrayBufferArray;
 
 class ScriptValue {
 public:
-    ScriptValue() { }
+    ScriptValue()
+        : m_isolate(0)

[haraken, 2013/09/18 00:25:12]

Is there any reason why we can't call Isolate::GetCurrent() here and ensure that
m_isolate is not 0 ?

[do-not-use, 2013/09/18 06:35:15]

My understanding was that we wanted to avoid calls to Isolate::GetCurrent() as
much as possible. I expect there are many cases where we construct an empty
StringValue and don't need an isolate at all.

[haraken, 2013/09/18 07:02:15]

Understood.

+    { }
+
     virtual ~ScriptValue();
 
     ScriptValue(v8::Handle<v8::Value> value, v8::Isolate* isolate)
-        : m_value(value.IsEmpty() ? 0 : SharedPersistent<v8::Value>::create(value, isolate))
+        : m_isolate(isolate)
+        , m_value(value.IsEmpty() ? 0 : SharedPersistent<v8::Value>::create(value, isolate))
     {
     }
 
     ScriptValue(const ScriptValue& value)
-        : m_value(value.m_value)
+        : m_isolate(value.m_isolate)
+        , m_value(value.m_value)
     {
     }
 
+    v8::Isolate* isolate() const { return m_isolate; }

[haraken, 2013/09/18 00:25:12]

Even if we cannot ensure that m_isolate is not 0 for some reason, we want to
ensure that isolate() doesn't return 0. Hence we want to write:

  v8::Isolate* isolate() const { if(!m_isolate) m_isolate =
Isolate::GetCurrent(); return m_isolate; }

[do-not-use, 2013/09/18 06:35:15]

Yes, this sounds like a good idea.

[do-not-use, 2013/09/18 07:18:46]

Done.

+
     static ScriptValue createNull()
     {
         v8::Isolate* isolate = v8::Isolate::GetCurrent();
         return ScriptValue(v8::Null(isolate), isolate);
     }
     static ScriptValue createBoolean(bool b)
     {
         v8::Isolate* isolate = v8::Isolate::GetCurrent();
         return ScriptValue(b ? v8::True(isolate) : v8::False(isolate), isolate);
     }
 
     ScriptValue& operator=(const ScriptValue& value)
     {
-        if (this != &value)
+        if (this != &value) {
             m_value = value.m_value;
+            m_isolate = value.m_isolate;
+        }
         return *this;
     }
 
     bool operator==(const ScriptValue& value) const
     {
         if (hasNoValue())
             return value.hasNoValue();
         if (value.hasNoValue())
             return false;
         return *m_value == *value.m_value;
@@ skipping 50 matching lines @@
         return !m_value.get() || m_value->isEmpty();
     }
 
     PassRefPtr<SerializedScriptValue> serialize(ScriptState*);
     PassRefPtr<SerializedScriptValue> serialize(ScriptState*, MessagePortArray*, ArrayBufferArray*, bool&);
     static ScriptValue deserialize(ScriptState*, SerializedScriptValue*);
 
     void clear()
     {
         m_value = 0;
+        m_isolate = 0;

[haraken, 2013/09/18 00:25:12]

Do we need to clear an isolate? In other words, is it possible that we reuse the
same ScriptPromise in different isolates?

[do-not-use, 2013/09/18 06:35:15]

I honestly have no idea. I chose the "safe" approach and reset everything.

[haraken, 2013/09/18 07:02:15]

I believe you can remove m_isolate=0. V8 heaps are completely separated among
isolates, so it's not possible to share one ScriptValue among isolates.

[do-not-use, 2013/09/18 07:18:46]

Done.

     }
 
     v8::Handle<v8::Value> v8Value() const
     {
-        return m_value.get() ? m_value->newLocal(v8::Isolate::GetCurrent()) : v8::Handle<v8::Value>();
+        return m_value.get() ? m_value->newLocal(m_isolate) : v8::Handle<v8::Value>();
     }
 
-    bool getString(ScriptState* scriptState, String& result) const { return getString(result, scriptState->isolate()); }
-    bool getString(String& result) const { return getString(result, v8::Isolate::GetCurrent()); }
-    bool getString(String& result, v8::Isolate*) const;
-    String toString(ScriptState*) const;
+    bool getString(String& result) const;
+    String toString() const;
 
     PassRefPtr<JSONValue> toJSONValue(ScriptState*) const;
 
 private:
+    v8::Isolate* m_isolate;
     RefPtr<SharedPersistent<v8::Value> > m_value;
 };
 
 } // namespace WebCore
 
 #endif // ScriptValue_h