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

include/AdblockPlus/JsValue.h

Index: include/AdblockPlus/JsValue.h
===================================================================
--- a/include/AdblockPlus/JsValue.h
+++ b/include/AdblockPlus/JsValue.h
@@ -37,21 +37,23 @@ namespace AdblockPlus
   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;
@@ -116,17 +118,17 @@ namespace AdblockPlus
     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 JsValueList& params = JsValueList(),
+    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.
      */