Usage of new API, cleanups (reduced)

src/org/adblockplus/android/ABPEngine.java

 /*
  * 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
@@ skipping 20 matching lines @@
 import org.adblockplus.libadblockplus.WebRequest;
 
 import android.content.Context;
 import android.content.pm.PackageInfo;
 import android.content.pm.PackageManager.NameNotFoundException;
 import android.os.Build.VERSION;
 import android.util.Log;
 
 public final class ABPEngine
 {
-  private final static String TAG = Utils.getTag(ABPEngine.class);
+  private static final String TAG = Utils.getTag(ABPEngine.class);
 
   private final Context context;
 
+  /*
+   * The fields below are volatile because:
+   *
+   * I encountered JNI related bugs/crashes caused by JNI backed Java objects. It seemed that under
+   * certain conditions the objects were optimized away which resulted in crashes when trying to
+   * release the object, sometimes even on access.
+   *
+   * The only solution that really worked was to declare the variables holding the references
+   * volatile, this seems to prevent the JNI from 'optimizing away' those objects (as a volatile
+   * variable might be changed at any time from any thread).
+   */
   private volatile JsEngine jsEngine;

[Felix Dahlke, 2014/04/16 15:24:25]

Why do these have to be volatile? How I understand it, volatile in Java only
ensures that no cached values of a variable are used. And since they are set
only once, in "create", it doesn't seem to me like there's more than one thread
involved at that point. Did I get that right?

[René Jeschke, 2014/04/16 17:51:47]

I have encountered problems with JNI backed Java objects and the JIT compiler.
Seemed like the JIT removed the object underneath and inlined the rest. The
solution to this is to declare this stuff 'volatile' (which somehow prevents the
JIT from removing the objects).

So, please, let's keep those volatile for now.
 

   private volatile FilterEngine filterEngine;
   private volatile LogSystem logSystem;
   private volatile WebRequest webRequest;
   private volatile EventCallback updateCallback;
   private volatile UpdaterCallback updaterCallback;
   private volatile FilterChangeCallback filterChangeCallback;
 
   private ABPEngine(final Context context)
   {
     this.context = context;
@@ skipping 12 matching lines @@
       Log.e(TAG, "Failed to get the application version number", e);
     }
     final String sdkVersion = String.valueOf(VERSION.SDK_INT);
     final String locale = context.getResources().getConfiguration().locale.toString();
     final boolean developmentBuild = !context.getResources().getBoolean(R.bool.def_release);
 
     return AppInfo.builder()
         .setVersion(version)
         .setApplicationVersion(sdkVersion)
         .setLocale(locale)
         .setDevelopmentBuild(developmentBuild)

[Felix Dahlke, 2014/04/16 15:24:25]

Looks like trailing whitespace, can you make sure not to add trailing
whitespace? Also below.

[René Jeschke, 2014/04/16 17:51:47]

Don't have any trailing whitespaces in my editor for those lines.

         .build();
   }
 
   public static ABPEngine create(final Context context, final AppInfo appInfo, final String basePath)
   {
     final ABPEngine engine = new ABPEngine(context);
 
     engine.jsEngine = new JsEngine(appInfo);
     engine.jsEngine.setDefaultFileSystem(basePath);
 
@@ skipping 59 matching lines @@
       this.filterChangeCallback.dispose();
       this.filterChangeCallback = null;
     }
   }
 
   public boolean isFirstRun()
   {
     return this.filterEngine.isFirstRun();
   }
 
-  private static org.adblockplus.android.Subscription convertJsSubscription(final Subscription sub)
-  {
-    final org.adblockplus.android.Subscription ret = new org.adblockplus.android.Subscription();

[Felix Dahlke, 2014/04/16 15:24:25]

How about calling this "subscription" (or "sub") and the parameters
"jsSubscription" (or "jsSub")?

[René Jeschke, 2014/04/16 17:51:47]

Done.

-
-    ret.title = sub.getProperty("title").toString();
-    ret.url = sub.getProperty("url").toString();
-
-    return ret;
-  }
-
-  private static org.adblockplus.android.Subscription[] convertJsSubscriptions(final List<Subscription> subs)
-  {
-    final org.adblockplus.android.Subscription[] ret = new org.adblockplus.android.Subscription[subs.size()];
-
-    for (int i = 0; i < ret.length; i++)
-    {
-      ret[i] = convertJsSubscription(subs.get(i));
-    }
-
-    return ret;
+  private static org.adblockplus.android.Subscription convertJsSubscription(final Subscription jsSubscription)
+  {
+    final org.adblockplus.android.Subscription subscription = new org.adblockplus.android.Subscription();
+
+    subscription.title = jsSubscription.getProperty("title").toString();
+    subscription.url = jsSubscription.getProperty("url").toString();
+
+    return subscription;
+  }
+
+  private static org.adblockplus.android.Subscription[] convertJsSubscriptions(final List<Subscription> jsSubscriptions)
+  {
+    final org.adblockplus.android.Subscription[] subscriptions = new org.adblockplus.android.Subscription[jsSubscriptions.size()];
+
+    for (int i = 0; i < subscriptions.length; i++)
+    {
+      subscriptions[i] = convertJsSubscription(jsSubscriptions.get(i));
+    }
+
+    return subscriptions;
   }
 
   public org.adblockplus.android.Subscription[] getRecommendedSubscriptions()
   {
     return convertJsSubscriptions(this.filterEngine.fetchAvailableSubscriptions());
   }
 
   public org.adblockplus.android.Subscription[] getListedSubscriptions()
   {
     return convertJsSubscriptions(this.filterEngine.getListedSubscriptions());
@@ skipping 73 matching lines @@
 
   public void updateSubscriptionStatus(final String url)
   {
     final Subscription sub = this.filterEngine.getSubscription(url);
     if (sub != null)
     {
       Utils.updateSubscriptionStatus(this.context, sub);
     }
   }
 }