Issue 4948 - add possibility to not send data depending on connection properties

libadblockplus-android/jni/JniFilterEngine.cpp

Index: libadblockplus-android/jni/JniFilterEngine.cpp
diff --git a/libadblockplus-android/jni/JniFilterEngine.cpp b/libadblockplus-android/jni/JniFilterEngine.cpp
index f9981f024922c74874f6a70c096c21cb23dc8e6e..bde6005194800dc4f6dd9a2061d791b41e0b4e66 100644
--- a/libadblockplus-android/jni/JniFilterEngine.cpp
+++ b/libadblockplus-android/jni/JniFilterEngine.cpp
@@ -44,12 +44,31 @@ static AdblockPlus::FilterEngine::ContentType ConvertContentType(JNIEnv *env,
   return AdblockPlus::FilterEngine::StringToContentType(value);
 }
 
-static jlong JNICALL JniCtor(JNIEnv* env, jclass clazz, jlong enginePtr)
+static jlong JNICALL JniCtor(JNIEnv* env, jclass clazz, jlong jsEnginePtr, jlong isAllowedConnectionCallbackPtr)
 {
   try
   {
-    AdblockPlus::JsEnginePtr& jsEngine = *JniLongToTypePtr<AdblockPlus::JsEnginePtr>(enginePtr);
-    return JniPtrToLong(new AdblockPlus::FilterEngine(jsEngine));
+    AdblockPlus::JsEnginePtr& jsEngine = *JniLongToTypePtr<AdblockPlus::JsEnginePtr>(jsEnginePtr);
+    AdblockPlus::FilterEnginePtr filterEnginePtr = NULL;

[sergei, 2017/03/15 10:01:38]

smart pointer is already initialized to nullptr, so no need for " = NULL" here.

[sergei, 2017/03/15 10:01:38]

I would just call the variable as filterEngine, without Ptr suffix, especially
there is no need for it, like collision with another variable.

[anton, 2017/03/15 10:13:52]

Acknowledged.

+
+    if (isAllowedConnectionCallbackPtr != 0)
+    {
+      AdblockPlus::FilterEngine::CreateParameters createParameters;
+      JniIsAllowedConnectionTypeCallback* callback =
+        JniLongToTypePtr<JniIsAllowedConnectionTypeCallback>(isAllowedConnectionCallbackPtr);
+
+      AdblockPlus::FilterEngine::IsConnectionAllowedCallback cppCallback =
+        std::bind(&JniIsAllowedConnectionTypeCallback::Callback, callback, std::placeholders::_1);
+      createParameters.isConnectionAllowed = cppCallback;
+
+      filterEnginePtr = AdblockPlus::FilterEngine::Create(jsEngine, createParameters); // here (1) - create engine sync
+    }
+    else
+    {
+      filterEnginePtr = AdblockPlus::FilterEngine::Create(jsEngine);
+    }
+
+    return JniPtrToLong(filterEnginePtr.get());

[anton, 2017/03/10 06:50:40]

for some (unknown) reason new static methods (instead of ctor previously)
returns incorrect pointer. iw as unable to test because of this

[sergei, 2017/03/15 10:01:38]

TL;DR: Actually here is a bug, but my vote is to accept it as is for present.

Long story:
The bug is that instance of FilterEngine is destroyed after finishing of the
method but we continue to use the dangling pointer. It is destroyed because we
exit from the scope, no one keeps the reference to FilterEngine and
FilterEnginePtr destroys it.

There are several ways to figure it out but I think it should be firstly
addressed in libadblockplus and we should not disturb the code of
libadblockplus-android because of bugs in libadblockplus. On the interface level
I plan to not force the user of libadblockplus to use shared_ptr<FilterEngine>
and change it to std::unique_ptr<FilterEngine> (or even to FilterEngine, though
it's unlikely), it will require to change only this particular part.

I'm ready to accept it because currently we have a circular reference and
FilterEngine is not destroyed, so technically it does work now, though it's
incorrect. Secondly, I think that fixing it now in libadblockplus-android will
introduce only numerous changes most of which will be reverted afterwards, and
this actions are unnecessary and error-prone. So for the sake of simplicity I
think we can accept it for present, then fix in libadblockplus and then change
probably only one line here.

[anton, 2017/03/15 10:13:52]

Acknowledged.

   }
   CATCH_THROW_AND_RETURN(env, 0)
 }
@@ -434,9 +453,45 @@ static jstring JNICALL JniGetHostFromURL(JNIEnv* env, jclass clazz, jlong ptr, j
   CATCH_THROW_AND_RETURN(env, 0)
 }
 
+static void JNICALL JniSetAllowedConnectionType(JNIEnv* env, jclass clazz, jlong ptr, jstring jvalue)
+{
+  AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+
+  std::string* value = NULL;
+  if (jvalue != NULL)
+  {
+    std::string stdValue = JniJavaToStdString(env, jvalue);
+    value = &stdValue;
+  }
+
+  try
+  {
+    engine->SetAllowedConnectionType(value);
+  }
+  CATCH_AND_THROW(env)
+}
+
+
+static jstring JNICALL JniGetAllowedConnectionType(JNIEnv* env, jclass clazz, jlong ptr)
+{
+  try
+  {
+    AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+    std::unique_ptr<std::string> value = engine->GetAllowedConnectionType();
+
+    if (value == NULL)
+    {
+      return NULL;
+    }
+
+    return JniStdStringToJava(env, *value.get());
+  }
+  CATCH_THROW_AND_RETURN(env, 0)
+}
+
 static JNINativeMethod methods[] =
 {
-  { (char*)"ctor", (char*)"(J)J", (void*)JniCtor },
+  { (char*)"ctor", (char*)"(JJ)J", (void*)JniCtor },
   { (char*)"isFirstRun", (char*)"(J)Z", (void*)JniIsFirstRun },
   { (char*)"getFilter", (char*)"(JLjava/lang/String;)" TYP("Filter"), (void*)JniGetFilter },
   { (char*)"getListedFilters", (char*)"(J)Ljava/util/List;", (void*)JniGetListedFilters },
@@ -459,6 +514,8 @@ static JNINativeMethod methods[] =
   { (char*)"getPref", (char*)"(JLjava/lang/String;)" TYP("JsValue"), (void*)JniGetPref },
   { (char*)"setPref", (char*)"(JLjava/lang/String;J)V", (void*)JniSetPref },
   { (char*)"getHostFromURL", (char*)"(JLjava/lang/String;)Ljava/lang/String;", (void*)JniGetHostFromURL },
+  { (char*)"setAllowedConnectionType", (char*)"(JLjava/lang/String;)V", (void*)JniSetAllowedConnectionType },
+  { (char*)"getAllowedConnectionType", (char*)"(J)Ljava/lang/String;", (void*)JniGetAllowedConnectionType },
   { (char*)"dtor", (char*)"(J)V", (void*)JniDtor }
 };