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

src/FilterEngine.cpp

Index: src/FilterEngine.cpp
===================================================================
--- a/src/FilterEngine.cpp
+++ b/src/FilterEngine.cpp
@@ -32,105 +32,105 @@ extern std::string jsSources[];
 
 Filter::Filter(JsValue&& value)
     : JsValue(std::move(value))
 {
   if (!IsObject())
     throw std::runtime_error("JavaScript value is not an object");
 }
 
-Filter::Type Filter::GetType()
+Filter::Type Filter::GetType() const
 {
   std::string className = GetClass();
   if (className == "BlockingFilter")
     return TYPE_BLOCKING;
   else if (className == "WhitelistFilter")
     return TYPE_EXCEPTION;
   else if (className == "ElemHideFilter")
     return TYPE_ELEMHIDE;
   else if (className == "ElemHideException")
     return TYPE_ELEMHIDE_EXCEPTION;
   else if (className == "CommentFilter")
     return TYPE_COMMENT;
   else
     return TYPE_INVALID;
 }
 
-bool Filter::IsListed()
+bool Filter::IsListed() const
 {
   JsValuePtr func = jsEngine->Evaluate("API.isListedFilter");
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(shared_from_this());
   return func->Call(params)->AsBool();

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

There is actually `JsValue::Call(const JsValue& arg)` what about changing in the
same commit the call of the function, `func->Call(*this)->AsBool();`?

It's also valid for many cases below.

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

Now that I'm looking at it, I think that this function should even be changed to
take a const JsConstValuePtr& so that we don't have to unwrap and reallocate a
new JsValue. And replacing all these calls where params has only one element.

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

We don't need to reallocate new JsValue here.

Actually I would like to reduce the number of cases when we use
std::shared_ptr<JsValue> and rather just use JsValue. So, we could add a helper
Call method accepting `const JsConstValuePtr&` but I would like to try without
it and actually see whether we need it.

[hub, 2017/03/24 14:08:03]

In JsValue::Call() we currently do:

params.push_back(JsValuePtr(new JsValue(arg.jsEngine, arg.UnwrapValue())));

Basically UnwrapValue() from const to a new value that we insert.

If we pass a JsConstValuePtr, we no longer need to do that.

[sergei, 2017/03/24 14:24:55]

I think that we should simply change the implementation of JsValue::Call(const
JsValue&) to don't create new JsValue and JsValuePtr. Finally I would like to
get rid of JsValuePtr because on practice there is no need for it.

[hub, 2017/03/24 16:05:06]

Agreed. Issue 3589?

[sergei, 2017/03/24 16:27:52]

I think it should be another issue (https://issues.adblockplus.org/ticket/5034)
and #3589 should be a part of it. So basically changing to `func->Call(*this)`
should be done as #5034.

 }
 
 void Filter::AddToList()
 {
   JsValuePtr func = jsEngine->Evaluate("API.addFilterToList");
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(shared_from_this());
   func->Call(params);
 }
 
 void Filter::RemoveFromList()
 {
   JsValuePtr func = jsEngine->Evaluate("API.removeFilterFromList");
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(shared_from_this());
   func->Call(params);
 }
 
 bool Filter::operator==(const Filter& filter) const
 {
   return GetProperty("text")->AsString() == filter.GetProperty("text")->AsString();
 }
 
 Subscription::Subscription(JsValue&& value)
     : JsValue(std::move(value))
 {
   if (!IsObject())
     throw std::runtime_error("JavaScript value is not an object");
 }
 
-bool Subscription::IsListed()
+bool Subscription::IsListed() const
 {
   JsValuePtr func = jsEngine->Evaluate("API.isListedSubscription");
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(shared_from_this());
   return func->Call(params)->AsBool();
 }
 
 void Subscription::AddToList()
 {
   JsValuePtr func = jsEngine->Evaluate("API.addSubscriptionToList");
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(shared_from_this());
   func->Call(params);
 }
 
 void Subscription::RemoveFromList()
 {
   JsValuePtr func = jsEngine->Evaluate("API.removeSubscriptionFromList");
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(shared_from_this());
   func->Call(params);
 }
 
 void Subscription::UpdateFilters()
 {
   JsValuePtr func = jsEngine->Evaluate("API.updateSubscription");
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(shared_from_this());
   func->Call(params);
 }
 
-bool Subscription::IsUpdating()
+bool Subscription::IsUpdating() const
 {
   JsValuePtr func = jsEngine->Evaluate("API.isSubscriptionUpdating");
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(shared_from_this());
   JsValuePtr result = func->Call(params);
   return result->AsBool();
 }
 
 bool Subscription::operator==(const Subscription& subscription) const
 {
   return GetProperty("url")->AsString() == subscription.GetProperty("url")->AsString();
@@ -284,28 +284,28 @@ FilterEngine::ContentType FilterEngine::
   throw std::invalid_argument("Cannot convert argument to ContentType");
 }
 
 bool FilterEngine::IsFirstRun() const
 {
   return firstRun;
 }
 
-FilterPtr FilterEngine::GetFilter(const std::string& text)
+FilterPtr FilterEngine::GetFilter(const std::string& text) const
 {
   JsValuePtr func = jsEngine->Evaluate("API.getFilterFromText");
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(jsEngine->NewValue(text));
   return FilterPtr(new Filter(std::move(*func->Call(params))));
 }
 
-SubscriptionPtr FilterEngine::GetSubscription(const std::string& url)
+SubscriptionPtr FilterEngine::GetSubscription(const std::string& url) const
 {
   JsValuePtr func = jsEngine->Evaluate("API.getSubscriptionFromUrl");
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(jsEngine->NewValue(url));
   return SubscriptionPtr(new Subscription(std::move(*func->Call(params))));
 }
 
 std::vector<FilterPtr> FilterEngine::GetListedFilters() const
 {
   JsValuePtr func = jsEngine->Evaluate("API.getListedFilters");
   JsValueList values = func->Call()->AsList();
@@ -333,17 +333,17 @@ std::vector<SubscriptionPtr> FilterEngin
   for (JsValueList::iterator it = values.begin(); it != values.end(); it++)
     result.push_back(SubscriptionPtr(new Subscription(std::move(**it))));
   return result;
 }
 
 void FilterEngine::ShowNextNotification(const std::string& url)
 {
   JsValuePtr func = jsEngine->Evaluate("API.showNextNotification");
-  JsValueList params;
+  JsConstValueList params;
   if (!url.empty())
   {
     params.push_back(jsEngine->NewValue(url));
   }
   func->Call(params);
 }
 
 void FilterEngine::SetShowNotificationCallback(const ShowNotificationCallback& value)
@@ -404,61 +404,61 @@ bool FilterEngine::IsElemhideWhitelisted
     return !!GetWhitelistingFilter(url, CONTENT_TYPE_ELEMHIDE, documentUrls);
 }
 
 AdblockPlus::FilterPtr FilterEngine::CheckFilterMatch(const std::string& url,
     ContentTypeMask contentTypeMask,
     const std::string& documentUrl) const
 {
   JsValuePtr func = jsEngine->Evaluate("API.checkFilterMatch");
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(jsEngine->NewValue(url));
   params.push_back(jsEngine->NewValue(contentTypeMask));
   params.push_back(jsEngine->NewValue(documentUrl));
   JsValuePtr result = func->Call(params);
   if (!result->IsNull())
     return FilterPtr(new Filter(std::move(*result)));
   else
     return FilterPtr();
 }
 
 std::vector<std::string> FilterEngine::GetElementHidingSelectors(const std::string& domain) const
 {
   JsValuePtr func = jsEngine->Evaluate("API.getElementHidingSelectors");
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(jsEngine->NewValue(domain));
   JsValueList result = func->Call(params)->AsList();
   std::vector<std::string> selectors;
   for (JsValueList::iterator it = result.begin(); it != result.end(); ++it)
     selectors.push_back((*it)->AsString());
   return selectors;
 }
 
 JsValuePtr FilterEngine::GetPref(const std::string& pref) const
 {
   JsValuePtr func = jsEngine->Evaluate("API.getPref");
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(jsEngine->NewValue(pref));
   return func->Call(params);
 }
 
 void FilterEngine::SetPref(const std::string& pref, JsValuePtr value)
 {
   JsValuePtr func = jsEngine->Evaluate("API.setPref");
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(jsEngine->NewValue(pref));
   if (value)
     params.push_back(value);
   func->Call(params);
 }
 
-std::string FilterEngine::GetHostFromURL(const std::string& url)
+std::string FilterEngine::GetHostFromURL(const std::string& url) const
 {
   JsValuePtr func = jsEngine->Evaluate("API.getHostFromUrl");
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(jsEngine->NewValue(url));
   return func->Call(params)->AsString();
 }
 
 void FilterEngine::SetUpdateAvailableCallback(
     FilterEngine::UpdateAvailableCallback callback)
 {
   jsEngine->SetEventCallback("updateAvailable",
@@ -477,17 +477,17 @@ void FilterEngine::UpdateAvailable(
   if (params.size() >= 1 && !params[0]->IsNull())
     callback(params[0]->AsString());
 }
 
 void FilterEngine::ForceUpdateCheck(
     const FilterEngine::UpdateCheckDoneCallback& callback)
 {
   JsValuePtr func = jsEngine->Evaluate("API.forceUpdateCheck");
-  JsValueList params;
+  JsConstValueList params;
   if (callback)
   {
     std::string eventName = "_updateCheckDone" + std::to_string(++updateCheckId);
     jsEngine->SetEventCallback(eventName, std::bind(&FilterEngine::UpdateCheckDone,
       this, eventName, callback, std::placeholders::_1));
     params.push_back(jsEngine->NewValue(eventName));
   }
   func->Call(params);
@@ -513,17 +513,17 @@ void FilterEngine::RemoveFilterChangeCal
   jsEngine->RemoveEventCallback("filterChange");
 }
 
 void FilterEngine::SetAllowedConnectionType(const std::string* value)
 {
   SetPref("allowed_connection_type", value ? jsEngine->NewValue(*value) : nullptr);
 }
 
-std::unique_ptr<std::string> FilterEngine::GetAllowedConnectionType()
+std::unique_ptr<std::string> FilterEngine::GetAllowedConnectionType() const
 {
    auto prefValue = GetPref("allowed_connection_type");
    if (prefValue->IsUndefined())
      return nullptr;
    return std::unique_ptr<std::string>(new std::string(prefValue->AsString()));
 }
 
 void FilterEngine::FilterChanged(FilterEngine::FilterChangeCallback callback, JsValueList& params)
@@ -542,19 +542,19 @@ void FilterEngine::ShowNotification(cons
   if (!params[0]->IsObject())
   {
     return;
   }
   callback(NotificationPtr(new Notification(std::move(*params[0]))));
 }
 
 
-int FilterEngine::CompareVersions(const std::string& v1, const std::string& v2)
+int FilterEngine::CompareVersions(const std::string& v1, const std::string& v2) const
 {
-  JsValueList params;
+  JsConstValueList params;
   params.push_back(jsEngine->NewValue(v1));
   params.push_back(jsEngine->NewValue(v2));
   JsValuePtr func = jsEngine->Evaluate("API.compareVersions");
   return func->Call(params)->AsInt();
 }
 
 FilterPtr FilterEngine::GetWhitelistingFilter(const std::string& url,
   ContentTypeMask contentTypeMask, const std::string& documentUrl) const