Create a minimal settings UI

mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java

+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2015 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/>.
+ */
+
+package org.adblockplus.browser;
+
+import android.annotation.SuppressLint;
+import android.os.Handler;
+import android.os.HandlerThread;
+import android.util.Log;
+
+import org.json.JSONException;
+import org.json.JSONObject;
+import org.mozilla.gecko.GeckoAppShell;
+import org.mozilla.gecko.util.GeckoRequest;
+import org.mozilla.gecko.util.NativeJSObject;
+
+@SuppressLint("DefaultLocale")
+public class AddOnBridge
+{
+  private static final String TAG = "AdblockBrowser.AddonBridge";
+  private static final String REQUEST_NAME = "AdblockPlus:Api";
+  // Timeout for ready checking (in seconds)
+  private static final int QUERY_READY_STATE_TIMEOUT = 45;
+  // How long to wait between retries (in milliseconds)
+  private static final int QUERY_READY_STATE_DELAY = 250;
+  // Handler+HandlerThread for posting delayed re-tries without interfering with
+  // other threads (e.g. the UI or Gecko thread)
+  private static final HandlerThread HANDLER_THREAD;
+  private static final Handler HANDLER;
+
+  static
+  {
+    HANDLER_THREAD = new HandlerThread("abp-addon-bridge");

[Felix Dahlke, 2015/03/22 15:51:45]

"abp-bridge" would do. Technically, there's no difference between code included
as part of the browser and code from an extension.

[René Jeschke, 2015/03/22 16:44:55]

Done.

+    HANDLER_THREAD.setDaemon(true);
+    HANDLER_THREAD.start();
+    HANDLER = new Handler(HANDLER_THREAD.getLooper());
+  }
+
+  public static boolean getBooleanFromJSObject(final NativeJSObject obj, final String name,

[Felix Dahlke, 2015/03/22 15:51:45]

Naming Nit: getBooleanFromJsObject, likewise get*FromJsObject below.

[René Jeschke, 2015/03/22 16:44:55]

Done.

+      final boolean defaultValue)
+  {
+    try
+    {
+      return obj.getBoolean(name);
+    }
+    catch (final Exception e)
+    {
+      return defaultValue;
+    }
+  }
+
+  public static String getStringFromJSObject(final NativeJSObject obj, final String name,
+      final String defaultValue)
+  {
+    try
+    {
+      return obj.getString(name);
+    }
+    catch (final Exception e)
+    {
+      return defaultValue;
+    }
+  }
+
+  private static JSONObject createRequestData(final String action)
+  {
+    final JSONObject obj = new JSONObject();
+    try
+    {
+      obj.put("action", action.toLowerCase());
+    }
+    catch (JSONException e)
+    {
+      // we're only adding sane objects
+      Log.e(TAG, "Creating request data failed with: " + e.getMessage(), e);
+    }
+    return obj;
+  }
+
+  private static JSONObject createRequestData(final String action, final boolean enable)

[Felix Dahlke, 2015/03/22 15:51:45]

s/enable/parameter/?

+  {
+    final JSONObject obj = new JSONObject();
+    try
+    {
+      obj.put("action", action.toLowerCase());
+      obj.put("enable", enable);
+    }
+    catch (JSONException e)
+    {
+      // we're only adding sane objects
+      Log.e(TAG, "Creating request data failed with: " + e.getMessage(), e);
+    }
+    return obj;
+  }
+
+  public static void queryBoolean(final AdblockPlusApiCallback callback, final String action)

[Felix Dahlke, 2015/03/22 15:51:45]

IMHO this is a bit confusing: queryBoolean sounds like it'll just return a
boolean for some key, while in reality it's more lower level messaging actually.

Since we don't need anything but setting/getting values yet, how about
simplifying it a bit? We could have:

void queryBoolean(final AdblockPlusApiCallback callback, final String name)

(Or just "getBoolean" analogous to "setBoolean"?)

Which would then call the more lower level "createRequestData" prepending "get"
in this case.

Note that you've updated the actions to be in camel case in the mean time - so
this won't work anymore. I was actually fine with the underscores after reading
the Java code here, makes sense to stick to typical Android naming. And if we
name the checkbox "acceptable_ads_enabled" and the action
"get_acceptable_ads_enabled", it'll be the same thing as
"getAcceptableAdsEnabled", just underscores instead of camel case - which is not
a problem at all IMO.

[René Jeschke, 2015/03/22 16:44:55]

It is called 'query' because it does not 'get' a boolean. It queries ABP for the
state of it, but does not return it. The result is supplied via the callback.

The 'action' name is legacy naming, can change that, though.

It also is not really low_level_messaging, as this method _must_ be asynchronous
anyway. I could have overengineered this thing by returning Futures or Promises,
but I wanted to keep it simple.

Also, 'not needed yet' is not a good reason for simplifying the API, I also had
the case in ABPdroid where I regretted  that I did not insist on keeping the
more complex CommandBridge :p.

Thing is: 'get' is wrong, as it does not return a value. Making it a getter is
also impossible, as this would mean blocking on the UI or gecko thread.

.... but, yes, the action thing is confusing, so I will change that.

[Felix Dahlke, 2015/03/22 17:44:34]

OK, fair enough.

+  {
+    Log.d(TAG, "queryBoolean for " + action);
+    GeckoAppShell.sendRequestToGecko(
+        new ChainedRequest(
+            createRequestData(action),
+            callback));
+  }
+
+  public static void setBoolean(final AdblockPlusApiCallback callback, final String action,
+      final boolean enable)
+  {
+    Log.d(TAG, "setBoolean " + enable + " for " + action);
+    GeckoAppShell.sendRequestToGecko(
+        new ChainedRequest(
+            createRequestData(action, enable),
+            callback));
+  }
+
+  private static class ChainedRequest extends GeckoRequest
+  {
+    private final JSONObject value;
+    private final AdblockPlusApiCallback apiCallback;
+    private final boolean initCheck;
+    private final long creationTime;
+
+    public ChainedRequest(final JSONObject value, final AdblockPlusApiCallback callback,
+        final boolean checkInitState, final long creationTime)

[Felix Dahlke, 2015/03/22 15:51:45]

I don't fully understand this one - 

[René Jeschke, 2015/03/22 16:44:55]

We start an asynchronous chain of requests here:
The value and callback passed to this request are the values for the final
request (or an error).

The first request (when 'checkInitState' is set) will be a check for
'filtersLoaded'
The response of this request is then checked for success and filtersLoaded==true
If this is true, the real request will be issued
otherwise we retry for a while (loop to beginning) and eventually report a
timeout error.

The naming for 'checkInitState' is now wrong, as you wanted to have the action
called 'filtersLoaded' ... so I will change this, too.

Yes, I could extract the logic into a separate place, but this would not make a
lot of a difference, instead we would have a lot more code and duplication.

It is not really an overhead checking the ready state each time we do something,
so I would like to keep it here.

Also, it encapsulates all logic, and one can just send a 'ChainedRequest' and
everything _just works_.

[Felix Dahlke, 2015/03/22 17:44:34]

Alright, get it now - so we're checking for filtersLoaded before doing anything
else. With that renamed callback, I get it :D

+    {
+      super(AddOnBridge.REQUEST_NAME,
+          checkInitState ? createRequestData("query_ready_state") : value);
+      this.value = value;
+      this.apiCallback = callback;
+      this.initCheck = checkInitState;
+      this.creationTime = creationTime;
+    }
+
+    public ChainedRequest(final JSONObject value, final AdblockPlusApiCallback callback)
+    {
+      this(value, callback, true, System.currentTimeMillis());
+    }
+
+    public ChainedRequest cloneForRetry()
+    {
+      return new ChainedRequest(this.value, this.apiCallback, true, this.creationTime);
+    }
+
+    public ChainedRequest cloneForRequest()
+    {
+      return new ChainedRequest(this.value, this.apiCallback, false, this.creationTime);
+    }
+
+    private void callSuccessFunction(final NativeJSObject jsObject)

[Felix Dahlke, 2015/03/22 15:51:45]

"invokeSuccessCallback"?

[René Jeschke, 2015/03/22 16:44:55]

Ok

+    {
+      try
+      {
+        if (this.apiCallback != null)
+        {
+          this.apiCallback.onApiRequestSucceeded(jsObject);
+        }
+      }
+      catch (final Exception e)
+      {
+        Log.e(TAG, "onApiRequestSucceeded threw exception: " + e.getMessage(), e);
+      }
+    }
+
+    private void callFailureFunction(final String msg)
+    {
+      if (this.apiCallback != null)
+      {
+        this.apiCallback.onApiRequestFailed(msg);
+      }
+    }
+
+    private void callFailureFunction(final NativeJSObject jsObject)
+    {
+      callFailureFunction(getStringFromJSObject(jsObject, "error", "unknown error"));
+    }
+
+    private void maybeReTry()

[Felix Dahlke, 2015/03/22 15:51:45]

How about "attemptRetry"?

[René Jeschke, 2015/03/22 16:44:55]

Yup.

+    {
+      if (System.currentTimeMillis() - this.creationTime > (QUERY_READY_STATE_TIMEOUT * 1000))
+      {
+        this.callFailureFunction("query_ready_state timeout");
+      }

[Felix Dahlke, 2015/03/22 15:51:45]

Shouldn't we return here, or am I missing something?

[René Jeschke, 2015/03/22 16:44:55]

Yeah, already changed.

+
+      final ChainedRequest next = this.cloneForRetry();
+      HANDLER.postDelayed(new Runnable()
+      {
+        @Override
+        public void run()
+        {
+          GeckoAppShell.sendRequestToGecko(next);
+        }
+      }, QUERY_READY_STATE_DELAY);
+    }
+
+    @Override
+    public void onError()
+    {
+      if (this.initCheck)
+      {
+        this.maybeReTry();
+      }
+      else
+      {
+        this.callFailureFunction("GeckoRequest error");
+      }
+    }
+
+    @Override
+    public void onResponse(final NativeJSObject jsObject)
+    {
+      if (this.initCheck)
+      {
+        if (getBooleanFromJSObject(jsObject, "success", false)
+            && getBooleanFromJSObject(jsObject, "value", false))
+        {
+          GeckoAppShell.sendRequestToGecko(this.cloneForRequest());

[Felix Dahlke, 2015/03/22 15:51:45]

So.... Not entirely sure I understand what initCheck is for: But does this mean
we're checking the value twice? Surely worth a comment.

[René Jeschke, 2015/03/22 16:44:55]

Yes, we are.

onResponse() happens on the main Gecko Thread, but we definitely have at least
three threads involved here:

the Android UI thread which started this request
the Gecko thread that handles it
the IO thread that fetches the filterSubscription
(or any other lazy-evaluation mechanism)

It is possible that we arrive here, after "initCommunication", but during
"filterLoading", which would mean that we're not yet ready. As loading continues
in the background, we need to re-check some time later.

That's what this class does, asynchronously delays until the extension is ready.

+        }
+        else
+        {
+          this.maybeReTry();
+        }
+      }
+      else
+      {
+        if (getBooleanFromJSObject(jsObject, "success", false))
+        {
+          this.callSuccessFunction(jsObject);
+        }
+        else
+        {
+          this.callFailureFunction(jsObject);
+        }
+      }
+    }
+  }
+}