Issue 303 - Don't load element hiding rules

src/org/adblockplus/android/AndroidWebRequest.java

 /*
- * This file is part of Adblock Plus <http://adblockplus.org/>,
- * Copyright (C) 2006-2014 Eyeo GmbH
+ * 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.
  *
@@ skipping 16 matching lines @@
 import org.adblockplus.libadblockplus.ServerResponse;
 import org.adblockplus.libadblockplus.ServerResponse.NsStatus;
 import org.adblockplus.libadblockplus.WebRequest;
 
 import android.util.Log;
 
 public class AndroidWebRequest extends WebRequest
 {
   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.

+  private boolean isListedSubscriptionUrl(final URL url)
   {
-    final String toCheck;
+    String toCheck = url.toString();
 
-    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 int idx = toCheck.indexOf('?');
+    if (idx != -1)
     {
-      final String tmp = url.toString();
-      final int idx = tmp.indexOf('?');
-      toCheck = idx != -1 ? tmp.substring(0, idx) : tmp;
-    }
-    else
-    {
-      toCheck = url.toString();
+      toCheck = toCheck.substring(0, idx);
     }
 
     return this.subscriptionURLs.contains(toCheck);
   }
 
   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());
     }
+    this.subscriptionURLs.add(engine.getPref("subscriptions_exceptionsurl").toString());
   }
 
   @Override
   public ServerResponse httpGET(final String urlStr, final List<HeaderEntry> headers)
   {
     try
     {
       final URL url = new URL(urlStr);
       Log.d(TAG, "Downloading from: " + url);
 
       final HttpURLConnection connection = (HttpURLConnection) url.openConnection();
       connection.setRequestMethod("GET");
       connection.connect();
 
       final ServerResponse response = new ServerResponse();
       response.setResponseStatus(connection.getResponseCode());
 
       if (response.getResponseStatus() == 200)
       {
         final BufferedReader reader = new BufferedReader(new InputStreamReader(connection.getInputStream(), "UTF-8"));
         final StringBuilder sb = new StringBuilder();
 
-        if (isListedSubscriptionURL(url))
+        if (isListedSubscriptionUrl(url))
         {
-          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.

+          Log.d(TAG, "Removing element hiding rules from: '" + url + "'");
 
           String line;
           while ((line = reader.readLine()) != null)
           {
-            // 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)
             {
@@ skipping 22 matching lines @@
         response.setStatus(NsStatus.ERROR_FAILURE);
       }
       return response;
     }
     catch (final Throwable t)
     {
       throw new AdblockPlusException("WebRequest failed", t);
     }
   }
 }