Issue 5013 - Make more methods const.- introduced JsConstValuePtr and JsConstValueList- JsValue:…

include/AdblockPlus/JsValue.h

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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
@@ skipping 24 matching lines @@
 {
   class JsValue;
   class JsEngine;
 
   typedef std::shared_ptr<JsEngine> JsEnginePtr;
 
   /**
    * Shared smart pointer to a `JsValue` instance.
    */
   typedef std::shared_ptr<JsValue> JsValuePtr;
   typedef std::shared_ptr<const JsValue> JsConstValuePtr;

[sergei, 2017/03/23 19:46:06]

What about ConstJsValuePtr?

 
   /**
    * List of JavaScript values.
    */
   typedef std::vector<AdblockPlus::JsValuePtr> JsValueList;
   typedef std::vector<AdblockPlus::JsConstValuePtr> JsConstValueList;

[sergei, 2017/03/23 19:46:06]

What about ConstJsValueList?

[hub, 2017/03/24 13:45:15]

To answer about here and above, I feel like ConstJsValueList here seems to be
ambiguous by reading the type name. Is it the List that is const or the JsValue.

My thinking for naming the type was that "Js" is a namespace prefix. So it felt
like JsConstValuePtr was appropriate, and then JsConstValueList was obvious as
being a list of JsConstValuePtr.

We don't have other types using const yet, so this somewhat set the precedent.
So, it is as you prefer.

[sergei, 2017/03/24 13:56:47]

Agree. Considering Js as a namespace makes sense here.

 
   /**
    * Wrapper for JavaScript values.
    * See `JsEngine` for creating `JsValue` objects.
    */
   class JsValue
   {
     friend class JsEngine;
   public:
     JsValue(JsValue&& src);
@@ skipping 54 matching lines @@
      * @return Class name of the value.
      */
     std::string GetClass() const;
 
     /**
      * Invokes the value as a function (see `IsFunction()`).
      * @param params Optional list of parameters.
      * @param thisPtr Optional `this` value.
      * @return Value returned by the function.
      */
     JsValuePtr Call(const JsConstValueList& params = JsConstValueList(),

[sergei, 2017/03/23 19:46:06]

This change is a bit questionable, but I think we can let it be for present.

[hub, 2017/03/24 13:45:15]

it is quite essential. The problem boils down to the Getter() function passing a
pointer to this (shared_from_this) as a parameter to Call() which make it
impossible to make the function const.
Note that line 135, the overloaded version just gets a const JsValue& as a
single arugment, so it is actually consistent here.

[sergei, 2017/03/24 13:56:47]

I rather thought about no replacing the existing method but adding yet one for
JsConstValueList but maybe it's even better to have only const
JsConstValueList&.
So, it's OK.

         AdblockPlus::JsValuePtr thisPtr = AdblockPlus::JsValuePtr()) const;
 
     /**
      * Invokes the value as a function (see `IsFunction()`) with single
      * parameter.
      * @param arg A single required parameter.
      * @return Value returned by the function.
      */
-    JsValuePtr Call(const JsValue& arg) const;
+    JsValuePtr Call(const JsConstValuePtr& arg) const;
 
     v8::Local<v8::Value> UnwrapValue() const;
   protected:
     JsEnginePtr jsEngine;
   private:
     JsValue(JsEnginePtr jsEngine, v8::Handle<v8::Value> value);
     void SetProperty(const std::string& name, v8::Handle<v8::Value> val);
     std::unique_ptr<v8::Persistent<v8::Value>> value;
   };
 }
 
 #endif