Noissue - move work with a raw jni pointer to a separate function

libadblockplus-android/jni/JniFilterEngine.cpp

 /*
  * 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 27 matching lines @@
   JniLocalReference<jclass> contentTypeClass(env,
       env->GetObjectClass(jContentType));
   jmethodID nameMethod = env->GetMethodID(*contentTypeClass, "name",
       "()Ljava/lang/String;");
   JniLocalReference<jstring> jValue(env,
       (jstring) env->CallObjectMethod(jContentType, nameMethod));
   const std::string value = JniJavaToStdString(env, *jValue);
   return AdblockPlus::FilterEngine::StringToContentType(value);
 }
 
 namespace

[anton, 2017/06/15 05:16:53]

can we have the same code style like we use in
https://github.com/adblockplus/libadblockplus-android/blob/master/libadblockp...
like
`static AdblockPlus::FilterEnginePtr& GetFilterEnginePtrRef(jlong ptr) { ... }`

[sergei, 2017/06/16 10:44:36]

Yes we can but this function is related to JniFilterEngine (another codereview)
and I thought that it should be in the same namespace. Should it be static?

In general, "static" here is used merely for internal linkage, as far as I
understand and I'm not sure how it behaves when there is the same method
declaration in a header, for instance. I personally prefer to have all code in
proper namespaces, e.g. "abp" and anonymous, because they are more descriptive,
explicit and can have some other benefits. So, my opinion is to continue to use
static to stay consistent with the existing code when it makes sense but
consider switching from it.

 {
   AdblockPlus::FilterEnginePtr& GetFilterEnginePtrRef(jlong ptr)
   {
     return *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
   }
 }
 
 static jlong JNICALL JniCtor(JNIEnv* env, jclass clazz, jlong jsEnginePtr, jlong isAllowedConnectionCallbackPtr)
 {
   try
@@ skipping 299 matching lines @@
               *JniLocalReference<jstring>(env,
                   static_cast<jstring>(
                       env->GetObjectArrayElement(jArray, i)))));
     }
   }
 }
 
 static jobject JNICALL JniMatchesMany(JNIEnv* env, jclass clazz, jlong ptr,
     jstring jUrl, jobject jContentType, jobjectArray jDocumentUrls)
 {
-  AdblockPlus::FilterEnginePtr& engine =

[jens, 2017/07/05 09:27:53]

Minor one, but why don't have the definition of engine in only one line, like in
all other cases?

[anton, 2017/07/05 09:30:32]

yes, i believe previously the line was too long to fit into 100 characters.
now we can (like it's done in other places) and i'd say we should.

[sergei, 2017/07/05 13:46:20]

Right, I guess it's been simply overlooked when I was reverting other changes.

-      GetFilterEnginePtrRef(ptr);
+  AdblockPlus::FilterEnginePtr& engine = GetFilterEnginePtrRef(ptr);
 
   std::string url = JniJavaToStdString(env, jUrl);
   AdblockPlus::FilterEngine::ContentType contentType =
       ConvertContentType(env, jContentType);
 
   std::vector<std::string> documentUrls;
   JavaStringArrayToStringVector(env, jDocumentUrls, documentUrls);
 
   try
   {
@@ skipping 177 matching lines @@
   { (char*)"setAcceptableAdsEnabled", (char*)"(JZ)V", (void*)JniSetAcceptableAdsEnabled },
   { (char*)"isAcceptableAdsEnabled", (char*)"(J)Z", (void*)JniIsAcceptableAdsEnabled },
   { (char*)"getAcceptableAdsSubscriptionURL", (char*)"(J)Ljava/lang/String;", (void*)JniGetAcceptableAdsSubscriptionURL },
   { (char*)"dtor", (char*)"(J)V", (void*)JniDtor }
 };
 
 extern "C" JNIEXPORT void JNICALL Java_org_adblockplus_libadblockplus_FilterEngine_registerNatives(JNIEnv *env, jclass clazz)
 {
   env->RegisterNatives(clazz, methods, sizeof(methods) / sizeof(methods[0]));
 }