Issue 1280 - Update v8

src/JsValue.cpp

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #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)
-{
-}
-
-AdblockPlus::JsValue::JsValue(AdblockPlus::JsValuePtr value)
-    : jsEngine(value->jsEngine),
-      value(value->value)
-{
-}
-
-AdblockPlus::JsValue::~JsValue()
-{
-}
-
-bool AdblockPlus::JsValue::IsUndefined() const
-{
-  const JsContext context(jsEngine);
-  return value->IsUndefined();
-}
-
-bool AdblockPlus::JsValue::IsNull() const
-{
-  const JsContext context(jsEngine);
-  return value->IsNull();
-}
-
-bool AdblockPlus::JsValue::IsString() const
-{
-  const JsContext context(jsEngine);
+    value(jsEngine->isolate, 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()
+{
+}
+
+bool JsValue::IsUndefined() const
+{
+  const JsContext context(jsEngine);
+  return unwrapValue()->IsUndefined();
+}
+
+bool JsValue::IsNull() const
+{
+  const JsContext context(jsEngine);
+  return unwrapValue()->IsNull();
+}
+
+bool 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
-{
-  const JsContext context(jsEngine);
+bool JsValue::IsNumber() const
+{
+  const JsContext context(jsEngine);
+  auto value = unwrapValue();
   return value->IsNumber() || value->IsNumberObject();
 }
 
-bool AdblockPlus::JsValue::IsBool() const
-{
-  const JsContext context(jsEngine);
+bool JsValue::IsBool() const
+{
+  const JsContext context(jsEngine);
+  auto value = unwrapValue();
   return value->IsBoolean() || value->IsBooleanObject();
 }
 
-bool AdblockPlus::JsValue::IsObject() const
-{
-  const JsContext context(jsEngine);
-  return value->IsObject();
-}
-
-bool AdblockPlus::JsValue::IsArray() const
-{
-  const JsContext context(jsEngine);
-  return value->IsArray();
-}
-
-bool AdblockPlus::JsValue::IsFunction() const
-{
-  const JsContext context(jsEngine);
-  return value->IsFunction();
-}
-
-std::string AdblockPlus::JsValue::AsString() const
-{
-  const JsContext context(jsEngine);
-  return Utils::FromV8String(value);
-}
-
-int64_t AdblockPlus::JsValue::AsInt() const
-{
-  const JsContext context(jsEngine);
-  return value->IntegerValue();
-}
-
-bool AdblockPlus::JsValue::AsBool() const
-{
-  const JsContext context(jsEngine);
-  return value->BooleanValue();
-}
-
-AdblockPlus::JsValueList AdblockPlus::JsValue::AsList() const
+bool JsValue::IsObject() const
+{
+  const JsContext context(jsEngine);
+  return unwrapValue()->IsObject();
+}
+
+bool JsValue::IsArray() const
+{
+  const JsContext context(jsEngine);
+  return unwrapValue()->IsArray();
+}
+
+bool JsValue::IsFunction() const
+{
+  const JsContext context(jsEngine);
+  return unwrapValue()->IsFunction();
+}
+
+std::string JsValue::AsString() const
+{
+  const JsContext context(jsEngine);
+  return Utils::FromV8String(unwrapValue());
+}
+
+int64_t JsValue::AsInt() const
+{
+  const JsContext context(jsEngine);
+  return unwrapValue()->IntegerValue();
+}
+
+bool JsValue::AsBool() const
+{
+  const JsContext context(jsEngine);
+  return unwrapValue()->BooleanValue();
+}
+
+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)));
-}
-
-void AdblockPlus::JsValue::SetProperty(const std::string& name, v8::Handle<v8::Value> val)
+  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 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)
-{
-  const JsContext context(jsEngine);
-  SetProperty(name, Utils::ToV8String(val));
-}
-
-void AdblockPlus::JsValue::SetProperty(const std::string& name, int64_t val)
-{
-  const JsContext context(jsEngine);
-  SetProperty(name, v8::Number::New(val));
-}
-
-void AdblockPlus::JsValue::SetProperty(const std::string& name, JsValuePtr val)
-{
-  const JsContext context(jsEngine);
-  SetProperty(name, val->value);
-}
-
-void AdblockPlus::JsValue::SetProperty(const std::string& name, bool 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(jsEngine->isolate, val));
+}
+
+void JsValue::SetProperty(const std::string& name, int64_t val)
+{
+  const JsContext context(jsEngine);
+  SetProperty(name, v8::Number::New(jsEngine->isolate, val));
+}
+
+void JsValue::SetProperty(const std::string& name, const JsValuePtr& val)
+{
+  const JsContext context(jsEngine);
+  SetProperty(name, val->unwrapValue());
+}
+
+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);
-
-  std::vector<v8::Handle<v8::Value> > argv;
+  auto thisObj = v8::Local<v8::Object>::Cast(thisPtr->unwrapValue());
+
+  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());
-
-  return JsValuePtr(new JsValue(jsEngine, result));
-}
+    throw JsError(tryCatch.Exception(), tryCatch.Message());
+
+  return std::make_shared<JsValue>(jsEngine, result, JsValue::Private::CtrArg());
+}