Issue 1280 - Update v8

src/JsValue.cpp

Index: src/JsValue.cpp
===================================================================
--- a/src/JsValue.cpp
+++ b/src/JsValue.cpp
@@ -16,208 +16,220 @@
  */
 
 #include <vector>
+#include <v8.h>
 #include <AdblockPlus.h>
 
 #include "JsContext.h"
 #include "JsError.h"
 #include "Utils.h"
 
-AdblockPlus::JsValue::JsValue(AdblockPlus::JsEnginePtr jsEngine,
-      v8::Handle<v8::Value> value)
+using namespace AdblockPlus;
+
+JsValue::JsValue(const JsEnginePtr& jsEngine, const v8::Handle<v8::Value> value, JsValue::PrivateCtrArg)
     : jsEngine(jsEngine),
-      value(jsEngine->isolate, value)
+    value(jsEngine->isolate, value)
 {
 }
 
-AdblockPlus::JsValue::JsValue(AdblockPlus::JsValuePtr value)
-    : jsEngine(value->jsEngine),
-      value(value->value)
+JsValue::JsValue(const JsValuePtr& value)
+  : jsEngine(value->jsEngine)

[Felix Dahlke, 2014/10/31 05:31:57]

Why change the initialiser list here? It's unrelated, and now inconsistent. Same
for the new empty line below. It's hard to figure out what was really changed by
the commit if there are unrelated style changes.

[sergei, 2014/10/31 11:47:01]

Sorry, fixed.

+  , value(value->value)
+{
+
+}
+
+JsValue::~JsValue()
 {
 }
 
-AdblockPlus::JsValue::~JsValue()
+bool JsValue::IsUndefined() const
 {
+  const JsContext context(jsEngine);
+  return unwrapValue()->IsUndefined();
 }
 
-bool AdblockPlus::JsValue::IsUndefined() const
+bool JsValue::IsNull() const
 {
   const JsContext context(jsEngine);
-  return value->IsUndefined();
+  return unwrapValue()->IsNull();
 }
 
-bool AdblockPlus::JsValue::IsNull() const
+bool JsValue::IsString() const
 {
   const JsContext context(jsEngine);
-  return value->IsNull();
-}
-
-bool AdblockPlus::JsValue::IsString() const
-{
-  const JsContext context(jsEngine);
+  auto value = unwrapValue();

[Felix Dahlke, 2014/10/31 05:31:57]

We do not use C++11 in libadblockplus yet.

[sergei, 2014/10/31 11:47:01]

Is it a real problem or only artificial restriction? What are the exact versions
of compilers we use to build libadblockplus?

BTW, the recent v8 requires C++11, so does it make sense to get rid of auto
here?

[Felix Dahlke, 2014/10/31 15:07:51]

More of an artificial restriction in that sense. The plan is to move to C++11 in
libadblockplus actually: https://issues.adblockplus.org/ticket/768

However, libadblockplus is a library, and we want to support a reasonably wide
range of platforms and compilers. Hence using a new C++11 feature needs some
review and testing. I'm all for that, but we shouldn't mix it in with an
important issue. And IMHO it would make more sense to get rid of TR1 for good
before we start to use anything from C++11.

[sergei, 2014/10/31 16:13:35]

fixed

   return value->IsString() || value->IsStringObject();
 }
 
-bool AdblockPlus::JsValue::IsNumber() const
+bool JsValue::IsNumber() const
 {
   const JsContext context(jsEngine);
+  auto value = unwrapValue();
   return value->IsNumber() || value->IsNumberObject();
 }
 
-bool AdblockPlus::JsValue::IsBool() const
+bool JsValue::IsBool() const
 {
   const JsContext context(jsEngine);
+  auto value = unwrapValue();
   return value->IsBoolean() || value->IsBooleanObject();
 }
 
-bool AdblockPlus::JsValue::IsObject() const
+bool JsValue::IsObject() const
 {
   const JsContext context(jsEngine);
-  return value->IsObject();
+  return unwrapValue()->IsObject();
 }
 
-bool AdblockPlus::JsValue::IsArray() const
+bool JsValue::IsArray() const
 {
   const JsContext context(jsEngine);
-  return value->IsArray();
+  return unwrapValue()->IsArray();
 }
 
-bool AdblockPlus::JsValue::IsFunction() const
+bool JsValue::IsFunction() const
 {
   const JsContext context(jsEngine);
-  return value->IsFunction();
+  return unwrapValue()->IsFunction();
 }
 
-std::string AdblockPlus::JsValue::AsString() const
+std::string JsValue::AsString() const
 {
   const JsContext context(jsEngine);
-  return Utils::FromV8String(value);
+  return Utils::FromV8String(unwrapValue());
 }
 
-int64_t AdblockPlus::JsValue::AsInt() const
+int64_t JsValue::AsInt() const
 {
   const JsContext context(jsEngine);
-  return value->IntegerValue();
+  return unwrapValue()->IntegerValue();
 }
 
-bool AdblockPlus::JsValue::AsBool() const
+bool JsValue::AsBool() const
 {
   const JsContext context(jsEngine);
-  return value->BooleanValue();
+  return unwrapValue()->BooleanValue();
 }
 
-AdblockPlus::JsValueList AdblockPlus::JsValue::AsList() const
+JsValueList JsValue::AsList() const
 {
   if (!IsArray())
     throw std::runtime_error("Cannot convert a non-array to list");
 
   const JsContext context(jsEngine);
   JsValueList result;
-  v8::Persistent<v8::Array> array = v8::Persistent<v8::Array>::Cast<v8::Value>(value);
+  auto array = v8::Local<v8::Array>::Cast(unwrapValue());
   uint32_t length = array->Length();
   for (uint32_t i = 0; i < length; i++)
   {
     v8::Local<v8::Value> item = array->Get(i);
-    result.push_back(JsValuePtr(new JsValue(jsEngine, item)));
+    result.push_back(std::make_shared<JsValue>(jsEngine, item, PrivateCtrArg()));
   }
   return result;
 }
 
-std::vector<std::string> AdblockPlus::JsValue::GetOwnPropertyNames() const
+std::vector<std::string> JsValue::GetOwnPropertyNames() const
 {
   if (!IsObject())
     throw new std::runtime_error("Attempting to get propert list for a non-object");
 
   const JsContext context(jsEngine);
-  const v8::Persistent<v8::Object> object = v8::Persistent<v8::Object>::Cast<v8::Value>(value);
-  JsValueList properties = JsValuePtr(new JsValue(jsEngine, object->GetOwnPropertyNames()))->AsList();
   std::vector<std::string> result;
+  auto object = v8::Local<v8::Object>::Cast(unwrapValue());
+  JsValueList properties = std::make_shared<JsValue>(jsEngine, object->GetOwnPropertyNames(), PrivateCtrArg())->AsList();
   for (JsValueList::iterator it = properties.begin(); it != properties.end(); ++it)
     result.push_back((*it)->AsString());
   return result;
 }
 
 
-AdblockPlus::JsValuePtr AdblockPlus::JsValue::GetProperty(const std::string& name) const
+JsValuePtr JsValue::GetProperty(const std::string& name) const
 {
   if (!IsObject())
     throw new std::runtime_error("Attempting to get property of a non-object");
 
   const JsContext context(jsEngine);
-  v8::Local<v8::String> property = Utils::ToV8String(name);
-  v8::Persistent<v8::Object> obj = v8::Persistent<v8::Object>::Cast<v8::Value>(value);
-  return JsValuePtr(new JsValue(jsEngine, obj->Get(property)));
+  v8::Local<v8::String> property = Utils::ToV8String(jsEngine->isolate, name);
+  auto obj = v8::Local<v8::Object>::Cast(unwrapValue());
+  return std::make_shared<JsValue>(jsEngine, obj->Get(property), PrivateCtrArg());
 }
 
-void AdblockPlus::JsValue::SetProperty(const std::string& name, v8::Handle<v8::Value> val)
+void JsValue::SetProperty(const std::string& name, const v8::Handle<v8::Value> val)
 {
   if (!IsObject())
     throw new std::runtime_error("Attempting to set property on a non-object");
 
-  v8::Local<v8::String> property = Utils::ToV8String(name);
-  v8::Persistent<v8::Object> obj = v8::Persistent<v8::Object>::Cast<v8::Value>(value);
+  v8::Local<v8::String> property = Utils::ToV8String(jsEngine->isolate, name);
+  auto obj = v8::Local<v8::Object>::Cast(unwrapValue());
   obj->Set(property, val);
 }
 
-void AdblockPlus::JsValue::SetProperty(const std::string& name, const std::string& val)
+v8::Local<v8::Value> JsValue::unwrapValue() const
+{
+  return v8::Local<v8::Value>::New(jsEngine->isolate, value);
+}
+
+void JsValue::SetProperty(const std::string& name, const std::string& val)
 {
   const JsContext context(jsEngine);
-  SetProperty(name, Utils::ToV8String(val));
+  SetProperty(name, Utils::ToV8String(jsEngine->isolate, val));
 }
 
-void AdblockPlus::JsValue::SetProperty(const std::string& name, int64_t val)
+void JsValue::SetProperty(const std::string& name, int64_t val)
 {
   const JsContext context(jsEngine);
-  SetProperty(name, v8::Number::New(val));
+  SetProperty(name, v8::Number::New(jsEngine->isolate, val));
 }
 
-void AdblockPlus::JsValue::SetProperty(const std::string& name, JsValuePtr val)
+void JsValue::SetProperty(const std::string& name, const JsValuePtr& val)
 {
   const JsContext context(jsEngine);
-  SetProperty(name, val->value);
+  SetProperty(name, val->unwrapValue());
 }
 
-void AdblockPlus::JsValue::SetProperty(const std::string& name, bool val)
+void JsValue::SetProperty(const std::string& name, bool val)
 {
   const JsContext context(jsEngine);
   SetProperty(name, v8::Boolean::New(val));
 }
 
-std::string AdblockPlus::JsValue::GetClass() const
+std::string JsValue::GetClass() const
 {
   if (!IsObject())
     throw new std::runtime_error("Cannot get constructor of a non-object");
 
   const JsContext context(jsEngine);
-  v8::Persistent<v8::Object> obj = v8::Persistent<v8::Object>::Cast<v8::Value>(value);
+  auto obj = v8::Local<v8::Object>::Cast(unwrapValue());
   return Utils::FromV8String(obj->GetConstructorName());
 }
 
-AdblockPlus::JsValuePtr AdblockPlus::JsValue::Call(
-    const JsValueList& params,
-    AdblockPlus::JsValuePtr thisPtr) const
+JsValuePtr JsValue::Call(const JsValueList& params, JsValuePtr thisPtr) const
 {
+  const JsContext context(jsEngine);
+
   if (!IsFunction())
     throw new std::runtime_error("Attempting to call a non-function");
 
-  const JsContext context(jsEngine);
-
   if (!thisPtr)
-    thisPtr = JsValuePtr(new JsValue(jsEngine, jsEngine->context->Global()));
+  {
+    auto localContext = v8::Local<v8::Context>::New(jsEngine->isolate, jsEngine->context);
+    thisPtr = std::make_shared<JsValue>(jsEngine, localContext->Global(), PrivateCtrArg());
+  }
   if (!thisPtr->IsObject())
     throw new std::runtime_error("`this` pointer has to be an object");
-  v8::Persistent<v8::Object> thisObj = v8::Persistent<v8::Object>::Cast<v8::Value>(thisPtr->value);
+  auto thisObj = v8::Local<v8::Object>::Cast(thisPtr->unwrapValue());
 
-  std::vector<v8::Handle<v8::Value> > argv;
+  std::vector<v8::Handle<v8::Value>> argv;
   for (JsValueList::const_iterator it = params.begin(); it != params.end(); ++it)
-    argv.push_back((*it)->value);
+    argv.push_back((*it)->unwrapValue());
 
   const v8::TryCatch tryCatch;
-  v8::Persistent<v8::Function> func = v8::Persistent<v8::Function>::Cast<v8::Value>(value);
+  auto func = v8::Local<v8::Function>::Cast(unwrapValue());
   v8::Local<v8::Value> result = func->Call(thisObj, argv.size(),
       argv.size() ? &argv.front() : 0);
 
   if (tryCatch.HasCaught())
-    throw AdblockPlus::JsError(tryCatch.Exception(), tryCatch.Message());
+    throw JsError(tryCatch.Exception(), tryCatch.Message());
 
-  return JsValuePtr(new JsValue(jsEngine, result));
+  return std::make_shared<JsValue>(jsEngine, result, JsValue::Private::CtrArg());
 }