Issue 4772 - Add temporary fix for removing unsupported filters

Issue 4772 - Add temporary fix for removing unsupported filters

[anton, 2017-01-09 09:54:43]

https://codereview.adblockplus.org/29370856/diff/29370857/src/org/adblockplus...
File src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java
(right):

https://codereview.adblockplus.org/29370856/diff/29370857/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:194: if
(!isFilterUnsupported(filter))
it would be better to have positive condition instead of double negative, f.e.
isFilterSupported()

[diegocarloslima, 2017-01-19 13:16:42]

https://codereview.adblockplus.org/29370856/diff/29370857/src/org/adblockplus...
File src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java
(right):

https://codereview.adblockplus.org/29370856/diff/29370857/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:194: if
(!isFilterUnsupported(filter))
On 2017/01/09 09:54:43, anton wrote:
> it would be better to have positive condition instead of double negative, f.e.
> isFilterSupported()

Acknowledged.

[anton, 2017-01-23 06:50:38]

On 2017/01/19 13:16:42, diegocarloslima wrote:
>
https://codereview.adblockplus.org/29370856/diff/29370857/src/org/adblockplus...
> File src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java
> (right):
> 
>
https://codereview.adblockplus.org/29370856/diff/29370857/src/org/adblockplus...
> src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:194: if
> (!isFilterUnsupported(filter))
> On 2017/01/09 09:54:43, anton wrote:
> > it would be better to have positive condition instead of double negative,
f.e.
> > isFilterSupported()
> 
> Acknowledged.

LGTM

[Felix Dahlke, 2017-01-30 16:24:48]

https://codereview.adblockplus.org/29370856/diff/29372674/src/org/adblockplus...
File src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java
(right):

https://codereview.adblockplus.org/29370856/diff/29372674/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:44:
private static final String[] UNSUPPORTED_FILTERS_REGEX = {
Why is this an array? Looks like there's just a single string, so I'd say let's
simplify it. If we need to check for multiple different regexps later (I hope we
can avoid it though), we can still turn this into an array.

[diegocarloslima, 2017-01-30 17:12:33]

https://codereview.adblockplus.org/29370856/diff/29372674/src/org/adblockplus...
File src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java
(right):

https://codereview.adblockplus.org/29370856/diff/29372674/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:44:
private static final String[] UNSUPPORTED_FILTERS_REGEX = {
On 2017/01/30 16:24:47, Felix Dahlke wrote:
> Why is this an array? Looks like there's just a single string, so I'd say
let's
> simplify it. If we need to check for multiple different regexps later (I hope
we
> can avoid it though), we can still turn this into an array.

Acknowledged.

[Felix Dahlke, 2017-02-07 15:34:16]

Almost there :D

https://codereview.adblockplus.org/29370856/diff/29373742/src/org/adblockplus...
File src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java
(right):

https://codereview.adblockplus.org/29370856/diff/29373742/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:44: //
Filters that begin with '|$' , '||$' , '@@|$' or '@@||$'
Nit: Maybe reference the issue? e.g.:

// Filters that begin with ... need to be removed,
// see https://issues.adblockplus.org/ticket/1337

https://codereview.adblockplus.org/29370856/diff/29373742/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:212: if
(filter.matches(UNSUPPORTED_FILTERS_REGEX))
Seems we can simplify the function to:

return !filter.matches(UNSUPPORTED_FILTERS_REGEX);

Might as well inline it, but up to you.

[Felix Dahlke, 2017-02-20 12:45:21]

LGTM!