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 24 matching lines @@
 import android.content.pm.PackageManager.NameNotFoundException;
 import android.os.Build.VERSION;
 import android.util.Log;
 
 public final class ABPEngine
 {
   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/28 07:29:01]

I know we discussed this in the last path set, but I still think it's not really
obvious why these are volatile. Maybe it makes sense to add a comment describing
the problem?

[René Jeschke, 2014/04/28 08:34:32]

Done.

   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/28 07:29:01]

Is this trailing whitespace? I can't really tell from the diff, looks weird.
Same below.

[René Jeschke, 2014/04/28 08:34:32]

Nope, Editor->Save actions->Remove trailing whitespace ... if I touched the
file, there are no trailing whitespace anymore^^

[Felix Dahlke, 2014/04/28 10:09:41]

Ah, so I guess the diff is telling me that there WAS trailing whitespace then.
Pretty weird, but OK :)

[René Jeschke, 2014/04/28 10:18:34]

Done.

         .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 69 matching lines @@
   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> subs)

[Felix Dahlke, 2014/04/28 07:29:01]

How about renaming "subs" to "jsSubscription" and "ret" to "subscription" like
above?

[René Jeschke, 2014/04/28 08:34:32]

Done.

-  {
-    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[] 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);
     }
   }
 }