New JNI bindings

jni/JniCallbacks.h

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef JNICALLBACKS_H_

[Felix Dahlke, 2014/03/28 08:29:00]

Our convention is: JNI_CALLBACKS_H (no trailing _, underscores as a word
separator)

Also note that we prepend the namespace, e.g. in this case it'd be:

ADBLOCK_PLUS_ANDROID_JNI_CALLBACKS_H

But, as I wrote below, I think it'd be better not to have a namespace here.

-#define JNICALLBACKS_H_
+#ifndef JNICALLBACKS_H
+#define JNICALLBACKS_H
 
 #include <AdblockPlus.h>
 #include "Utils.h"
 #include "JniJsValue.h"
 
-namespace AdblockPlus
-{
-namespace Android

[Felix Dahlke, 2014/03/28 08:29:00]

Namespace bodies are to be indented:
https://adblockplus.org/en/coding-style

However, since this is part of the application code and not a library, I'd vote
for not having a namespace at all.

-{
-
 class JniCallbackBase
 {
 public:
-  JniCallbackBase(JNIEnv* env, jobject callbackObject)
+  JniCallbackBase(JNIEnv* env, jobject callbackObject);
+  virtual ~JniCallbackBase();
+  void LogException(JNIEnv* env, jthrowable throwable) const;
+  void CheckAndLogJavaException(JNIEnv* env) const;
+
+  JavaVM* GetJavaVM() const
   {
-    env->GetJavaVM(&m_JavaVM);

[Felix Dahlke, 2014/03/28 08:29:00]

IMO, we should move all the function definitions from JniCallbackBase to an
implementation file. It's not much, but beyond trivial.

-    m_CallbackObject.reset(new JniGlobalReference<jobject>(env, callbackObject));
-    m_ExceptionLoggerClass.reset(new JniGlobalReference<jclass>(env, env->FindClass(PKG("JniExceptionHandler"))));
+    return javaVM;
   }
 
-  virtual ~JniCallbackBase()
+  jobject GetCallbackObject() const
   {
-  }
-
-  inline JavaVM* GetJavaVM() const

[Felix Dahlke, 2014/03/28 08:29:00]

inline is redundant for member functions. Same below.

-  {
-    return m_JavaVM;
-  }
-
-  inline jobject GetCallbackObject() const
-  {
-    return m_CallbackObject->get();
-  }
-
-  void LogException(JNIEnv* env, jthrowable throwable) const
-  {
-    jmethodID logMethod = env->GetStaticMethodID(m_ExceptionLoggerClass->get(), "logException", "(Ljava/lang/Throwable;)V");
-    if (logMethod)
-    {
-      env->CallStaticVoidMethod(m_ExceptionLoggerClass->get(), logMethod, throwable);
-    }
-  }
-
-  void CheckAndLogJavaException(JNIEnv* env) const
-  {
-    if (env->ExceptionCheck())
-    {
-      jthrowable throwable = env->ExceptionOccurred();
-      env->ExceptionClear();
-      LogException(env, throwable);
-    }
+    return callbackObject->Get();
   }
 
 private:
-  JavaVM* m_JavaVM;

[Felix Dahlke, 2014/03/28 08:29:00]

From https://adblockplus.org/en/coding-style:
"No hungarian notation, no special variable name prefixes or suffixes denoting
type or scope."

[René Jeschke, 2014/03/28 10:59:15]

I'll change this, but we should think this over again, as having some prefix for
member variables avoids shadowing (which will otherwise result in strange
function argument names or the usage of 'this').

-  JniGlobalReference<jobject>::PTR m_CallbackObject;
-  JniGlobalReference<jclass>::PTR m_ExceptionLoggerClass;
+  JavaVM* javaVM;
+  JniGlobalReference<jobject>::Ptr callbackObject;
+  JniGlobalReference<jclass>::Ptr exceptionLoggerClass;
 };
 
-class JniEventCallback: public JniCallbackBase

[Felix Dahlke, 2014/03/28 08:29:00]

Space before ":" please :) Likewise below.

+class JniEventCallback : public JniCallbackBase
 {
 public:
   JniEventCallback(JNIEnv* env, jobject callbackObject);
-  virtual ~JniEventCallback();
   void Callback(AdblockPlus::JsValueList& params);
 };
 
-class JniUpdaterCallback: public JniCallbackBase
+class JniUpdaterCallback : public JniCallbackBase
 {
 public:
   JniUpdaterCallback(JNIEnv* env, jobject callbackObject);
-  virtual ~JniUpdaterCallback();
   void Callback(const std::string& arg);
 };
 
-class JniFilterChangeCallback: public JniCallbackBase
+class JniFilterChangeCallback : public JniCallbackBase
 {
 public:
   JniFilterChangeCallback(JNIEnv* env, jobject callbackObject);
-  virtual ~JniFilterChangeCallback();
   void Callback(const std::string& arg, const AdblockPlus::JsValuePtr jsValue);
 };
 
-class JniLogSystemCallback: public JniCallbackBase, public AdblockPlus::LogSystem
+class JniLogSystemCallback : public JniCallbackBase, public AdblockPlus::LogSystem
 {
 public:
   JniLogSystemCallback(JNIEnv* env, jobject callbackObject);
-  virtual ~JniLogSystemCallback();
   void operator()(AdblockPlus::LogSystem::LogLevel logLevel, const std::string& message, const std::string& source);
+
 private:

[Felix Dahlke, 2014/03/28 08:29:00]

Whitespace before "private"?

-  JniGlobalReference<jclass>::PTR m_LogLevelClass;
+  JniGlobalReference<jclass>::Ptr logLevelClass;
 };
 
-class JniWebRequest: public JniCallbackBase, public AdblockPlus::WebRequest
+class JniWebRequest : public JniCallbackBase, public AdblockPlus::WebRequest
 {
 public:
   JniWebRequest(JNIEnv* env, jobject callbackObject);
-  virtual ~JniWebRequest();
   AdblockPlus::ServerResponse GET(const std::string& url, const AdblockPlus::HeaderList& requestHeaders) const;
+
 private:

[Felix Dahlke, 2014/03/28 08:29:00]

Whitespace before "private"?

   jobject NewTuple(JNIEnv* env, const std::string& a, const std::string& b) const;
 
-  JniGlobalReference<jclass>::PTR m_TupleClass;
-  JniGlobalReference<jclass>::PTR m_ServerResponseClass;
+  JniGlobalReference<jclass>::Ptr tupleClass;
+  JniGlobalReference<jclass>::Ptr serverResponseClass;
 };
 
-} // namespace Android
-} // namespace AdblockPlus
-
-#endif /* JNICALLBACKS_H_ */
+#endif /* JNICALLBACKS_H */