Issue 6680224334872576: Issue 303 - Don't load element hiding rules (Closed)
|
Created:
Nov. 6, 2014, 1:36 p.m. by René Jeschke Modified:
Feb. 4, 2015, 1:07 p.m. Reviewers:
Felix Dahlke CC:
Wladimir Palant Visibility:
Public. |
DescriptionIssue 303 - Don't load element hiding rules
Patch Set 1 #
Total comments: 7
Patch Set 2 : getPath(), char-by-char #Patch Set 3 : Removed mightBeFilterList boolean var. #Patch Set 4 : Added check for valid, listed subscription. #Patch Set 5 : Made HACK obvious. #
Total comments: 18
Patch Set 6 : Naming, FIXME, whitelist-url #
Total comments: 1
Patch Set 7 : URL query removal #
MessagesTotal messages: 19
http://codereview.adblockplus.org/6680224334872576/diff/5629499534213120/src/... File src/org/adblockplus/android/AndroidWebRequest.java (right): http://codereview.adblockplus.org/6680224334872576/diff/5629499534213120/src/... src/org/adblockplus/android/AndroidWebRequest.java:53: filename = filename.substring(0, params); Why not just use url.getPath() instead of manually cutting off the query string? According to the docs, url.getFile() is the same as url.getPath() + url.getQuery(). http://codereview.adblockplus.org/6680224334872576/diff/5629499534213120/src/... src/org/adblockplus/android/AndroidWebRequest.java:56: return filename.toLowerCase(Locale.US).trim().endsWith(".txt"); I think I might have thought of a better way to determine whether the request is for a filter list: We can go through all active subscriptions and check their URL. We'd have to ensure that the subscription is first added to the list of known subscriptions before the request happens on first start. That'd allow us to give this method name a bit more conviction, e.g. "isSubscriptionUrl" or something. http://codereview.adblockplus.org/6680224334872576/diff/5629499534213120/src/... src/org/adblockplus/android/AndroidWebRequest.java:82: while ((line = reader.readLine()) != null) Using BufferedReader instead of System.arraycopy seems fine for all requests we make here, but I think we shouldn't read anything but filter lists line-by-line.
http://codereview.adblockplus.org/6680224334872576/diff/5629499534213120/src/... File src/org/adblockplus/android/AndroidWebRequest.java (right): http://codereview.adblockplus.org/6680224334872576/diff/5629499534213120/src/... src/org/adblockplus/android/AndroidWebRequest.java:53: filename = filename.substring(0, params); On 2014/11/11 09:15:37, Felix H. Dahlke wrote: > Why not just use url.getPath() instead of manually cutting off the query string? > According to the docs, url.getFile() is the same as url.getPath() + > url.getQuery(). Right, done that. http://codereview.adblockplus.org/6680224334872576/diff/5629499534213120/src/... src/org/adblockplus/android/AndroidWebRequest.java:56: return filename.toLowerCase(Locale.US).trim().endsWith(".txt"); On 2014/11/11 09:15:37, Felix H. Dahlke wrote: > I think I might have thought of a better way to determine whether the request is > for a filter list: We can go through all active subscriptions and check their > URL. We'd have to ensure that the subscription is first added to the list of > known subscriptions before the request happens on first start. > > That'd allow us to give this method name a bit more conviction, e.g. > "isSubscriptionUrl" or something. I think that this is a bit much of an overhead for a hack. The Android app does not load anything else than filter lists or update JSONs, so a check for .txt should really be Ok for now. http://codereview.adblockplus.org/6680224334872576/diff/5629499534213120/src/... src/org/adblockplus/android/AndroidWebRequest.java:82: while ((line = reader.readLine()) != null) On 2014/11/11 09:15:37, Felix H. Dahlke wrote: > Using BufferedReader instead of System.arraycopy seems fine for all requests we > make here, but I think we shouldn't read anything but filter lists line-by-line. Ok, we'll read the rest char-by-char.
http://codereview.adblockplus.org/6680224334872576/diff/5629499534213120/src/... File src/org/adblockplus/android/AndroidWebRequest.java (right): http://codereview.adblockplus.org/6680224334872576/diff/5629499534213120/src/... src/org/adblockplus/android/AndroidWebRequest.java:56: return filename.toLowerCase(Locale.US).trim().endsWith(".txt"); On 2014/11/11 11:55:55, René Jeschke wrote: > On 2014/11/11 09:15:37, Felix H. Dahlke wrote: > > I think I might have thought of a better way to determine whether the request > is > > for a filter list: We can go through all active subscriptions and check their > > URL. We'd have to ensure that the subscription is first added to the list of > > known subscriptions before the request happens on first start. > > > > That'd allow us to give this method name a bit more conviction, e.g. > > "isSubscriptionUrl" or something. > > I think that this is a bit much of an overhead for a hack. The Android app does > not load anything else than filter lists or update JSONs, so a check for .txt > should really be Ok for now. It currently doesn't, but I don't think hard coding such assumptions here is a good idea if we can easily avoid it. We might make requests for *.txt files that aren't filter lists. And, more likely, we might make requests for filter lists that don't match *.txt. I believe we should try to be more robust here, if we can. Going over all subscription URLs here should be really negligible overhead-wise - we're only making about one request per day per filter list, slowing that one down by maybe 10ms shouldn't be noticeable at all. On the other hand, we could be 100% sure that it's a filter list request, no matter what.
On 2014/11/11 16:18:30, Felix H. Dahlke wrote: > http://codereview.adblockplus.org/6680224334872576/diff/5629499534213120/src/... > File src/org/adblockplus/android/AndroidWebRequest.java (right): > > http://codereview.adblockplus.org/6680224334872576/diff/5629499534213120/src/... > src/org/adblockplus/android/AndroidWebRequest.java:56: return > filename.toLowerCase(Locale.US).trim().endsWith(".txt"); > On 2014/11/11 11:55:55, René Jeschke wrote: > > On 2014/11/11 09:15:37, Felix H. Dahlke wrote: > > > I think I might have thought of a better way to determine whether the > request > > is > > > for a filter list: We can go through all active subscriptions and check > their > > > URL. We'd have to ensure that the subscription is first added to the list of > > > known subscriptions before the request happens on first start. > > > > > > That'd allow us to give this method name a bit more conviction, e.g. > > > "isSubscriptionUrl" or something. > > > > I think that this is a bit much of an overhead for a hack. The Android app > does > > not load anything else than filter lists or update JSONs, so a check for .txt > > should really be Ok for now. > > It currently doesn't, but I don't think hard coding such assumptions here is a > good idea if we can easily avoid it. We might make requests for *.txt files that > aren't filter lists. And, more likely, we might make requests for filter lists > that don't match *.txt. I believe we should try to be more robust here, if we > can. > > Going over all subscription URLs here should be really negligible overhead-wise > - we're only making about one request per day per filter list, slowing that one > down by maybe 10ms shouldn't be noticeable at all. On the other hand, we could > be 100% sure that it's a filter list request, no matter what. I'm not sure if this will work: 1. JsEngine needs WebRequest 2. FilterEngine needs a JsEngine instance 3. FilterEngine provides the known-subscriptions list So, if FilterEngine does use WebRequest on initialization we could run into: 1. Failed downloads (as we do not have a WebRequest set) 2. False negative of filter lists (so elemhide wouldn't be filtered)
On 2014/11/11 16:34:53, René Jeschke wrote:
> On 2014/11/11 16:18:30, Felix H. Dahlke wrote:
> > Going over all subscription URLs here should be really negligible
> overhead-wise
> > - we're only making about one request per day per filter list, slowing that
> one
> > down by maybe 10ms shouldn't be noticeable at all. On the other hand, we
could
> > be 100% sure that it's a filter list request, no matter what.
>
> I'm not sure if this will work:
> 1. JsEngine needs WebRequest
> 2. FilterEngine needs a JsEngine instance
> 3. FilterEngine provides the known-subscriptions list
>
> So, if FilterEngine does use WebRequest on initialization we could run into:
> 1. Failed downloads (as we do not have a WebRequest set)
> 2. False negative of filter lists (so elemhide wouldn't be filtered)
Yeah, it would be a bad idea to rely on FilterEngine in WebRequest. However,
what about this:
We make WebRequest a bit more generic and make it possible to pass in a filter
object, e.g.:
interface ResponseFilter {
boolean matches(URL requestUrl);
void filterResponse(StringBuffer response);
}
We could set the response filter after initialising WebRequest, JsEngine and
FilterEngine, e.g.:
// Initialise JsEngine, WebRequest etc.
// Initialise FilterEngine
webRequest.setResponseFilter(new ResponseFilter() {
boolean matches(URL requestUrl) {
return filterEngine.matchesAnySubscriptionUrl(requestUrl);
}
void filterResponse(StringBuffer response) {
...
}
});
On 2014/11/13 09:44:21, Felix H. Dahlke wrote:
> On 2014/11/11 16:34:53, René Jeschke wrote:
> > On 2014/11/11 16:18:30, Felix H. Dahlke wrote:
> > > Going over all subscription URLs here should be really negligible
> > overhead-wise
> > > - we're only making about one request per day per filter list, slowing
that
> > one
> > > down by maybe 10ms shouldn't be noticeable at all. On the other hand, we
> could
> > > be 100% sure that it's a filter list request, no matter what.
> >
> > I'm not sure if this will work:
> > 1. JsEngine needs WebRequest
> > 2. FilterEngine needs a JsEngine instance
> > 3. FilterEngine provides the known-subscriptions list
> >
> > So, if FilterEngine does use WebRequest on initialization we could run into:
> > 1. Failed downloads (as we do not have a WebRequest set)
> > 2. False negative of filter lists (so elemhide wouldn't be filtered)
>
> Yeah, it would be a bad idea to rely on FilterEngine in WebRequest. However,
> what about this:
>
> We make WebRequest a bit more generic and make it possible to pass in a filter
> object, e.g.:
>
> interface ResponseFilter {
> boolean matches(URL requestUrl);
> void filterResponse(StringBuffer response);
> }
>
> We could set the response filter after initialising WebRequest, JsEngine and
> FilterEngine, e.g.:
>
> // Initialise JsEngine, WebRequest etc.
> // Initialise FilterEngine
> webRequest.setResponseFilter(new ResponseFilter() {
> boolean matches(URL requestUrl) {
> return filterEngine.matchesAnySubscriptionUrl(requestUrl);
> }
>
> void filterResponse(StringBuffer response) {
> ...
> }
> });
You overlooked my main concern: if the FilterEngine does any subscription
updates on creation/startup, then we will not have the chance to filter out
element hiding filters because our filter is not yet initialized.
On 2014/11/17 10:15:14, René Jeschke wrote:
> On 2014/11/13 09:44:21, Felix H. Dahlke wrote:
> > On 2014/11/11 16:34:53, René Jeschke wrote:
> > > On 2014/11/11 16:18:30, Felix H. Dahlke wrote:
> > > > Going over all subscription URLs here should be really negligible
> > > overhead-wise
> > > > - we're only making about one request per day per filter list, slowing
> that
> > > one
> > > > down by maybe 10ms shouldn't be noticeable at all. On the other hand, we
> > could
> > > > be 100% sure that it's a filter list request, no matter what.
> > >
> > > I'm not sure if this will work:
> > > 1. JsEngine needs WebRequest
> > > 2. FilterEngine needs a JsEngine instance
> > > 3. FilterEngine provides the known-subscriptions list
> > >
> > > So, if FilterEngine does use WebRequest on initialization we could run
into:
> > > 1. Failed downloads (as we do not have a WebRequest set)
> > > 2. False negative of filter lists (so elemhide wouldn't be filtered)
> >
> > Yeah, it would be a bad idea to rely on FilterEngine in WebRequest. However,
> > what about this:
> >
> > We make WebRequest a bit more generic and make it possible to pass in a
filter
> > object, e.g.:
> >
> > interface ResponseFilter {
> > boolean matches(URL requestUrl);
> > void filterResponse(StringBuffer response);
> > }
> >
> > We could set the response filter after initialising WebRequest, JsEngine and
> > FilterEngine, e.g.:
> >
> > // Initialise JsEngine, WebRequest etc.
> > // Initialise FilterEngine
> > webRequest.setResponseFilter(new ResponseFilter() {
> > boolean matches(URL requestUrl) {
> > return filterEngine.matchesAnySubscriptionUrl(requestUrl);
> > }
> >
> > void filterResponse(StringBuffer response) {
> > ...
> > }
> > });
>
> You overlooked my main concern: if the FilterEngine does any subscription
> updates on creation/startup, then we will not have the chance to filter out
> element hiding filters because our filter is not yet initialized.
I didn't, but I don't see why that should concern us here. The request happens
asynchronously - unless we do something stupid/blocking in FilterEngine after
initiating the filter list update, we'll register the ResponseFilter before the
response can arrive.
And anyway, the initial delay for downloading filter lists is 6 minutes.
On 2014/11/17 13:34:08, Felix H. Dahlke wrote:
> On 2014/11/17 10:15:14, René Jeschke wrote:
> > On 2014/11/13 09:44:21, Felix H. Dahlke wrote:
> > > On 2014/11/11 16:34:53, René Jeschke wrote:
> > > > On 2014/11/11 16:18:30, Felix H. Dahlke wrote:
> > > > > Going over all subscription URLs here should be really negligible
> > > > overhead-wise
> > > > > - we're only making about one request per day per filter list, slowing
> > that
> > > > one
> > > > > down by maybe 10ms shouldn't be noticeable at all. On the other hand,
we
> > > could
> > > > > be 100% sure that it's a filter list request, no matter what.
> > > >
> > > > I'm not sure if this will work:
> > > > 1. JsEngine needs WebRequest
> > > > 2. FilterEngine needs a JsEngine instance
> > > > 3. FilterEngine provides the known-subscriptions list
> > > >
> > > > So, if FilterEngine does use WebRequest on initialization we could run
> into:
> > > > 1. Failed downloads (as we do not have a WebRequest set)
> > > > 2. False negative of filter lists (so elemhide wouldn't be filtered)
> > >
> > > Yeah, it would be a bad idea to rely on FilterEngine in WebRequest.
However,
> > > what about this:
> > >
> > > We make WebRequest a bit more generic and make it possible to pass in a
> filter
> > > object, e.g.:
> > >
> > > interface ResponseFilter {
> > > boolean matches(URL requestUrl);
> > > void filterResponse(StringBuffer response);
> > > }
> > >
> > > We could set the response filter after initialising WebRequest, JsEngine
and
> > > FilterEngine, e.g.:
> > >
> > > // Initialise JsEngine, WebRequest etc.
> > > // Initialise FilterEngine
> > > webRequest.setResponseFilter(new ResponseFilter() {
> > > boolean matches(URL requestUrl) {
> > > return filterEngine.matchesAnySubscriptionUrl(requestUrl);
> > > }
> > >
> > > void filterResponse(StringBuffer response) {
> > > ...
> > > }
> > > });
> >
> > You overlooked my main concern: if the FilterEngine does any subscription
> > updates on creation/startup, then we will not have the chance to filter out
> > element hiding filters because our filter is not yet initialized.
>
> I didn't, but I don't see why that should concern us here. The request happens
> asynchronously - unless we do something stupid/blocking in FilterEngine after
> initiating the filter list update, we'll register the ResponseFilter before
the
> response can arrive.
>
Aha! This is what I wanted to know:
> And anyway, the initial delay for downloading filter lists is 6 minutes.
So, righty right, I will implement this check.
On 2014/11/17 13:36:48, René Jeschke wrote:
> On 2014/11/17 13:34:08, Felix H. Dahlke wrote:
> > On 2014/11/17 10:15:14, René Jeschke wrote:
> > > On 2014/11/13 09:44:21, Felix H. Dahlke wrote:
> > > > On 2014/11/11 16:34:53, René Jeschke wrote:
> > > > > On 2014/11/11 16:18:30, Felix H. Dahlke wrote:
> > > > > > Going over all subscription URLs here should be really negligible
> > > > > overhead-wise
> > > > > > - we're only making about one request per day per filter list,
slowing
> > > that
> > > > > one
> > > > > > down by maybe 10ms shouldn't be noticeable at all. On the other
hand,
> we
> > > > could
> > > > > > be 100% sure that it's a filter list request, no matter what.
> > > > >
> > > > > I'm not sure if this will work:
> > > > > 1. JsEngine needs WebRequest
> > > > > 2. FilterEngine needs a JsEngine instance
> > > > > 3. FilterEngine provides the known-subscriptions list
> > > > >
> > > > > So, if FilterEngine does use WebRequest on initialization we could run
> > into:
> > > > > 1. Failed downloads (as we do not have a WebRequest set)
> > > > > 2. False negative of filter lists (so elemhide wouldn't be filtered)
> > > >
> > > > Yeah, it would be a bad idea to rely on FilterEngine in WebRequest.
> However,
> > > > what about this:
> > > >
> > > > We make WebRequest a bit more generic and make it possible to pass in a
> > filter
> > > > object, e.g.:
> > > >
> > > > interface ResponseFilter {
> > > > boolean matches(URL requestUrl);
> > > > void filterResponse(StringBuffer response);
> > > > }
> > > >
> > > > We could set the response filter after initialising WebRequest, JsEngine
> and
> > > > FilterEngine, e.g.:
> > > >
> > > > // Initialise JsEngine, WebRequest etc.
> > > > // Initialise FilterEngine
> > > > webRequest.setResponseFilter(new ResponseFilter() {
> > > > boolean matches(URL requestUrl) {
> > > > return filterEngine.matchesAnySubscriptionUrl(requestUrl);
> > > > }
> > > >
> > > > void filterResponse(StringBuffer response) {
> > > > ...
> > > > }
> > > > });
> > >
> > > You overlooked my main concern: if the FilterEngine does any subscription
> > > updates on creation/startup, then we will not have the chance to filter
out
> > > element hiding filters because our filter is not yet initialized.
> >
> > I didn't, but I don't see why that should concern us here. The request
happens
> > asynchronously - unless we do something stupid/blocking in FilterEngine
after
> > initiating the filter list update, we'll register the ResponseFilter before
> the
> > response can arrive.
> >
>
> Aha! This is what I wanted to know:
> > And anyway, the initial delay for downloading filter lists is 6 minutes.
>
> So, righty right, I will implement this check.
I decided to go a slightly different route here and just enabled a check for
listed subscription URLs.
There is no reason to implement a generic response filtering mechanism for a
hack that's gonna be replaced anyway.
And: first reading the full response and then working on it is a waste of
memory.
Still, now we can be sure that only valid subscription downloads get the
elemhide-removal applied.
Yeah, I prefer this to my suggestion actually, was a bit overgeneralised... Some comments though. http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... File src/org/adblockplus/android/AndroidWebRequest.java (right): http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:40: private final HashSet<String> subscriptionURLs = new HashSet<String>(); You're not using HashSet specific API, so why not use Set for the field type? http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:42: private boolean isListedSubscriptionURL(final URL url) Seems we're usually capitalising acronyms as "Url" as opposed to "URL" in our own code, based on some grepping. http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:46: if (url.getQuery() != null) Why cut the query string off manually again? As I said last time, url.getPath() seems to do exactly that for us. http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:62: for (final org.adblockplus.libadblockplus.Subscription s : engine.fetchAvailableSubscriptions()) 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. http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:90: Log.d(TAG, "Enabling elemhide removal."); Maybe rather "Removing element hiding rules from " + url - or something to that extent? http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:95: // FIXME This is a hack! Can we strike the "FIXME" part? :) We shouldn't have a TODO/FIXME/WHATEVER comment when there's an issue for something.
http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... File src/org/adblockplus/android/AndroidWebRequest.java (right): http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:40: private final HashSet<String> subscriptionURLs = new HashSet<String>(); On 2014/11/19 10:16:37, Felix H. Dahlke wrote: > You're not using HashSet specific API, so why not use Set for the field type? Because it it bloating the import statements above and it's not going to change in the future. http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:42: private boolean isListedSubscriptionURL(final URL url) On 2014/11/19 10:16:37, Felix H. Dahlke wrote: > Seems we're usually capitalising acronyms as "Url" as opposed to "URL" in our > own code, based on some grepping. Right, will change that. http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:46: if (url.getQuery() != null) On 2014/11/19 10:16:37, Felix H. Dahlke wrote: > Why cut the query string off manually again? As I said last time, url.getPath() > seems to do exactly that for us. 'getPath() only returns the path, I need the full URL (protocol, host, port, path) without query and there's no method for doing this. http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:62: for (final org.adblockplus.libadblockplus.Subscription s : engine.fetchAvailableSubscriptions()) On 2014/11/19 10:16:37, Felix H. Dahlke wrote: > 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. 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. http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:90: Log.d(TAG, "Enabling elemhide removal."); On 2014/11/19 10:16:37, Felix H. Dahlke wrote: > Maybe rather "Removing element hiding rules from " + url - or something to that > extent? 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. http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:95: // FIXME This is a hack! On 2014/11/19 10:16:37, Felix H. Dahlke wrote: > Can we strike the "FIXME" part? :) We shouldn't have a TODO/FIXME/WHATEVER > comment when there's an issue for something. 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.
http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... File src/org/adblockplus/android/AndroidWebRequest.java (right): http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:40: private final HashSet<String> subscriptionURLs = new HashSet<String>(); On 2014/11/24 13:15:57, René Jeschke wrote: > On 2014/11/19 10:16:37, Felix H. Dahlke wrote: > > You're not using HashSet specific API, so why not use Set for the field type? > > Because it it bloating the import statements above and it's not going to change > in the future. 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. http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:62: for (final org.adblockplus.libadblockplus.Subscription s : engine.fetchAvailableSubscriptions()) On 2014/11/24 13:15:57, René Jeschke wrote: > On 2014/11/19 10:16:37, Felix H. Dahlke wrote: > > 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. > > 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. 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. http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:95: // FIXME This is a hack! On 2014/11/24 13:15:57, René Jeschke wrote: > On 2014/11/19 10:16:37, Felix H. Dahlke wrote: > > Can we strike the "FIXME" part? :) We shouldn't have a TODO/FIXME/WHATEVER > > comment when there's an issue for something. > > FIXME has the advantage that you get notified about it in your IDE without > having to look through hundreds of issues. 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. > Also, FIXMEs are annoying in your IDE and therefore constantly remind you of > your open tasks. 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.
http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... File src/org/adblockplus/android/AndroidWebRequest.java (right): http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:62: for (final org.adblockplus.libadblockplus.Subscription s : engine.fetchAvailableSubscriptions()) On 2014/11/27 05:41:57, Felix H. Dahlke wrote: > On 2014/11/24 13:15:57, René Jeschke wrote: > > On 2014/11/19 10:16:37, Felix H. Dahlke wrote: > > > 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. > > > > 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. > > 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. Done. http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:95: // FIXME This is a hack! On 2014/11/27 05:41:57, Felix H. Dahlke wrote: > On 2014/11/24 13:15:57, René Jeschke wrote: > > On 2014/11/19 10:16:37, Felix H. Dahlke wrote: > > > Can we strike the "FIXME" part? :) We shouldn't have a TODO/FIXME/WHATEVER > > > comment when there's an issue for something. > > > > FIXME has the advantage that you get notified about it in your IDE without > > having to look through hundreds of issues. > > 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. > > > Also, FIXMEs are annoying in your IDE and therefore constantly remind you of > > your open tasks. > > 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. Ok, removed the 'FIXME', but left the rest of the comment in place.
http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... File src/org/adblockplus/android/AndroidWebRequest.java (right): http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... src/org/adblockplus/android/AndroidWebRequest.java:95: // FIXME This is a hack! On 2015/01/23 13:28:10, René Jeschke wrote: > On 2014/11/27 05:41:57, Felix H. Dahlke wrote: > > On 2014/11/24 13:15:57, René Jeschke wrote: > > > On 2014/11/19 10:16:37, Felix H. Dahlke wrote: > > > > Can we strike the "FIXME" part? :) We shouldn't have a TODO/FIXME/WHATEVER > > > > comment when there's an issue for something. > > > > > > FIXME has the advantage that you get notified about it in your IDE without > > > having to look through hundreds of issues. > > > > 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. > > > > > Also, FIXMEs are annoying in your IDE and therefore constantly remind you of > > > your open tasks. > > > > 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. > > Ok, removed the 'FIXME', but left the rest of the comment in place. That's quite alright, I'm a fan of this comment. Stuff like this definitely needs a big comment. http://codereview.adblockplus.org/6680224334872576/diff/5631943370604544/src/... File src/org/adblockplus/android/AndroidWebRequest.java (right): http://codereview.adblockplus.org/6680224334872576/diff/5631943370604544/src/... src/org/adblockplus/android/AndroidWebRequest.java:46: if (url.getQuery() != null) It seems to me this conditional logic is pointless - lines 48-50 can handle the case where there's no `?` in the URL, so it seems we can replace this if/else block by these three lines.
On 2015/01/27 08:40:51, Felix H. Dahlke wrote: > http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... > File src/org/adblockplus/android/AndroidWebRequest.java (right): > > http://codereview.adblockplus.org/6680224334872576/diff/5126842331693056/src/... > src/org/adblockplus/android/AndroidWebRequest.java:95: // FIXME This is a hack! > On 2015/01/23 13:28:10, René Jeschke wrote: > > On 2014/11/27 05:41:57, Felix H. Dahlke wrote: > > > On 2014/11/24 13:15:57, René Jeschke wrote: > > > > On 2014/11/19 10:16:37, Felix H. Dahlke wrote: > > > > > Can we strike the "FIXME" part? :) We shouldn't have a > TODO/FIXME/WHATEVER > > > > > comment when there's an issue for something. > > > > > > > > FIXME has the advantage that you get notified about it in your IDE without > > > > having to look through hundreds of issues. > > > > > > 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. > > > > > > > Also, FIXMEs are annoying in your IDE and therefore constantly remind you > of > > > > your open tasks. > > > > > > 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. > > > > Ok, removed the 'FIXME', but left the rest of the comment in place. > > That's quite alright, I'm a fan of this comment. Stuff like this definitely > needs a big comment. > > http://codereview.adblockplus.org/6680224334872576/diff/5631943370604544/src/... > File src/org/adblockplus/android/AndroidWebRequest.java (right): > > http://codereview.adblockplus.org/6680224334872576/diff/5631943370604544/src/... > src/org/adblockplus/android/AndroidWebRequest.java:46: if (url.getQuery() != > null) > It seems to me this conditional logic is pointless - lines 48-50 can handle the > case where there's no `?` in the URL, so it seems we can replace this if/else > block by these three lines. *gnah* If you insist^^ ... changed.
LGTM I'm currently assuming we can just get rid of this hack once we've fixed the element hiding issues on Android. With Typed Objects, the memory overhead caused by element hiding should hopefully be non-significant. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
