Issue 303 - Don't load element hiding rules

src/org/adblockplus/android/AndroidWebRequest.java

Index: src/org/adblockplus/android/AndroidWebRequest.java
diff --git a/src/org/adblockplus/android/AndroidWebRequest.java b/src/org/adblockplus/android/AndroidWebRequest.java
index 35102acc610982d0bc7b5aad471bd9cf1431628a..d80e88fdd5f462312ad43dabb1a59126c2b768fd 100644
--- a/src/org/adblockplus/android/AndroidWebRequest.java
+++ b/src/org/adblockplus/android/AndroidWebRequest.java
@@ -17,25 +17,53 @@
 
 package org.adblockplus.android;
 
-import java.io.InputStream;
+import java.io.BufferedReader;
+import java.io.InputStreamReader;
 import java.net.HttpURLConnection;
 import java.net.URL;
+import java.util.HashSet;
 import java.util.List;
 
 import org.adblockplus.libadblockplus.AdblockPlusException;
+import org.adblockplus.libadblockplus.FilterEngine;
 import org.adblockplus.libadblockplus.HeaderEntry;
 import org.adblockplus.libadblockplus.ServerResponse;
-import org.adblockplus.libadblockplus.WebRequest;
 import org.adblockplus.libadblockplus.ServerResponse.NsStatus;
+import org.adblockplus.libadblockplus.WebRequest;
 
 import android.util.Log;
 
 public class AndroidWebRequest extends WebRequest
 {
-  public final String TAG = Utils.getTag(WebRequest.class);
+  public final static String TAG = Utils.getTag(WebRequest.class);
+
+  private final HashSet<String> subscriptionURLs = new HashSet<String>();

[Felix Dahlke, 2014/11/19 10:16:37]

You're not using HashSet specific API, so why not use Set for the field type?

[René Jeschke, 2014/11/24 13:15:57]

Because it it bloating the import statements above and it's not going to change
in the future.

[Felix Dahlke, 2014/11/27 05:41:57]

Ah, fair enough. It's a bit unusual in Java-world and I thought I might be
missing some special HashSet API you're using there.

+
+  private boolean isListedSubscriptionURL(final URL url)

[Felix Dahlke, 2014/11/19 10:16:37]

Seems we're usually capitalising acronyms as "Url" as opposed to "URL" in our
own code, based on some grepping.

[René Jeschke, 2014/11/24 13:15:57]

Right, will change that.

+  {
+    final String toCheck;
+
+    if (url.getQuery() != null)

[Felix Dahlke, 2014/11/19 10:16:37]

Why cut the query string off manually again? As I said last time, url.getPath()
seems to do exactly that for us.

[René Jeschke, 2014/11/24 13:15:57]

'getPath() only returns the path, I need the full URL (protocol, host, port,
path) without query and there's no method for doing this.

+    {
+      final String tmp = url.toString();
+      final int idx = tmp.indexOf('?');
+      toCheck = idx != -1 ? tmp.substring(0, idx) : tmp;
+    }
+    else
+    {
+      toCheck = url.toString();
+    }
+
+    return this.subscriptionURLs.contains(toCheck);
+  }
 
-  private static final int INITIAL_BUFFER_SIZE = 65536;
-  private static final int BUFFER_GROWTH_DELTA = 65536;
+  protected void updateSubscriptionURLs(final FilterEngine engine)
+  {
+    for (final org.adblockplus.libadblockplus.Subscription s : engine.fetchAvailableSubscriptions())

[Felix Dahlke, 2014/11/19 10:16:37]

fetchAvailableSubscriptions will show the list of _recommended_ subscriptions,
that's quite different from the actually configured subscriptions of the user.
It'll be a list of the main blocking list (EasyList) all language-specific
variants. Was that intended?

We can use getListedSubscriptions() to get the actually active subscriptions.
However, that list may change, we would have to call updateSubscriptionURLs from
the filter update callback.

Given how ABP for Android currently only uses listed subscriptions, with the
exception of the whitelist, that could do for now I suppose. Working with the
actually active subscriptions would be more robust though.

[René Jeschke, 2014/11/24 13:15:57]

I really want to avoid connecting this hack with all the guts of the current
App. As we do not have a way to use user defined filter lists currently, and
this hack should be removed in 2015Q2 (latest), I don't think we should put more
effort into this here.

[Felix Dahlke, 2014/11/27 05:41:57]

That's fair enough, yeah. In that case I suggest we check for the whitelist URL
too (there should be a pref with it), then we're covering all possible
subscriptions here.

[René Jeschke, 2015/01/23 13:28:10]

Done.

+    {
+      this.subscriptionURLs.add(s.getProperty("url").toString());
+    }
+  }
 
   @Override
   public ServerResponse httpGET(final String urlStr, final List<HeaderEntry> headers)
@@ -43,7 +71,7 @@ public class AndroidWebRequest extends WebRequest
     try
     {
       final URL url = new URL(urlStr);
-      Log.d(this.TAG, "Downloading from: " + url);
+      Log.d(TAG, "Downloading from: " + url);
 
       final HttpURLConnection connection = (HttpURLConnection) url.openConnection();
       connection.setRequestMethod("GET");
@@ -54,34 +82,47 @@ public class AndroidWebRequest extends WebRequest
 
       if (response.getResponseStatus() == 200)
       {
-        final InputStream in = connection.getInputStream();
-
-        final byte[] buffer = new byte[4096];
+        final BufferedReader reader = new BufferedReader(new InputStreamReader(connection.getInputStream(), "UTF-8"));
+        final StringBuilder sb = new StringBuilder();
 
-        byte[] out = new byte[INITIAL_BUFFER_SIZE];
-
-        int pos = 0;
-        for (;;)
+        if (isListedSubscriptionURL(url))
         {
-          final int read = in.read(buffer);
-          if (read < 0)
+          Log.d(TAG, "Enabling elemhide removal.");

[Felix Dahlke, 2014/11/19 10:16:37]

Maybe rather "Removing element hiding rules from " + url - or something to that
extent?

[René Jeschke, 2014/11/24 13:15:57]

I didn't want to spam the log output so I chose a short version. I will remove
this as it's not really needed.

+
+          String line;
+          while ((line = reader.readLine()) != null)
           {
-            break;
+            // FIXME This is a hack!

[Felix Dahlke, 2014/11/19 10:16:37]

Can we strike the "FIXME" part? :) We shouldn't have a TODO/FIXME/WHATEVER
comment when there's an issue for something.

[René Jeschke, 2014/11/24 13:15:57]

FIXME has the advantage that you get notified about it in your IDE without
having to look through hundreds of issues.
Also, FIXMEs are annoying in your IDE and therefore constantly remind you of
your open tasks.

So, I really would like to keep this.

[Felix Dahlke, 2014/11/27 05:41:57]

Unless you have hundreds of FIXMEs and TODOs, which is from all I've seen the
normal state of projects that use TODO comments :) That's why we don't, we have
an issue tracker for this and use it consistently.
We could make #1541 a P1 issue if it was something we need to be constantly
reminded about, but I frankly don't believe it's all that important.

How I see it, #1541 is P3, and I agree with that priority, maybe P4 even. It'd
be nice, sure, but it wouldn't really _change_ anything.

And no matter whether it's a hack or not, we'll remove it at some not-too-far
point in the future: We _want_ element hiding. We should enable it again as soon
as we fixed the issues and reduced memory usage.

[René Jeschke, 2015/01/23 13:28:10]

Ok, removed the 'FIXME', but left the rest of the comment in place.

[Felix Dahlke, 2015/01/27 08:40:51]

That's quite alright, I'm a fan of this comment. Stuff like this definitely
needs a big comment.

+            //
+            // We're only appending non-element-hiding filters here.
+            //
+            // See:
+            //      https://issues.adblockplus.org/ticket/303
+            //
+            // Follow-up issue for removing this hack:
+            //      https://issues.adblockplus.org/ticket/1541
+            //
+            if (line.indexOf('#') == -1)
+            {
+              sb.append(line);
+              sb.append('\n');
+            }
           }
-          if (pos + read > out.length)
+        }
+        else
+        {
+          int character;
+
+          while ((character = reader.read()) != -1)
           {
-            final byte[] old = out;
-            out = new byte[out.length + BUFFER_GROWTH_DELTA];
-            System.arraycopy(old, 0, out, 0, pos);
+            sb.append((char) character);
           }
-          System.arraycopy(buffer, 0, out, pos, read);
-          pos += read;
         }
 
         connection.disconnect();
 
         response.setStatus(NsStatus.OK);
-        response.setResponse(new String(out, 0, pos, "utf-8"));
+        response.setResponse(sb.toString());
       }
       else
       {