Noissue - Lint adjustments and optimizations

Noissue - Lint adjustments and optimizations

[anton, 2017-06-02 07:39:56]

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/ListedSubscriptionsPreferenceCategory.java
(right):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/ListedSubscriptionsPreferenceCategory.java:194:
final boolean enabled = ((Boolean) newValue);
minor: double brackets are not required, it could be just `= (Boolean)
newValue;`

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java
(right):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java:150:
final boolean enabled = ((Boolean) newValue);
minor: double brackets are not required, it could be just `= (Boolean)
newValue;

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
(right):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:405:
try (final DataOutputStream metaOut = new DataOutputStream(new
BufferedOutputStream(
here and below: what's the purpose of buffering before gzipping?
As for me it seems reasonable to buffer before writing to file to eliminate file
io routines (as it was before).
What am i missing?

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java
(right):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:73:
final File file = new File(this.cacheFolder, String.format(Locale.ENGLISH,
"tmp-%d.txt",
what's a purpose of setting locale explicitly?

[jens, 2017-06-02 10:50:01]

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/ContentBlockerContentProvider.java
(left):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/ContentBlockerContentProvider.java:69:
final boolean applicationActived = getBooleanPref(prefs, key, false);
minor, but 'applicationActived' -> this typo could be fixed

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java
(right):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:47:
import org.adblockplus.sbrowser.contentblocker.MainPreferences;
Am I wrong, or is this an unused import?

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:536:
continue;
I think this continue statement can be deleted.

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/SubscriptionInfo.java
(right):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/SubscriptionInfo.java:45:
SubscriptionInfo(final Type type, final String id, final String url, final
String title, final boolean enabled, final long lastUpdate)
I think this constructor can be private, as we only use the static create()
method.

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java
(left):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:53:
private final boolean wasUnitialized;
This member and the corresponding getter have a typo and should be named
'wasUninitialized', right?

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:93:
void getSubscriptions(final List<Subscription> list)
I think the method name has a typo and should be changed to
"addSubscriptions(final List<Subscription> list)"

[diegocarloslima, 2017-06-02 21:03:42]

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/ContentBlockerContentProvider.java
(left):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/ContentBlockerContentProvider.java:69:
final boolean applicationActived = getBooleanPref(prefs, key, false);
On 2017/06/02 10:50:00, jens wrote:
> minor, but 'applicationActived' -> this typo could be fixed 

Acknowledged.

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/ListedSubscriptionsPreferenceCategory.java
(right):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/ListedSubscriptionsPreferenceCategory.java:194:
final boolean enabled = ((Boolean) newValue);
On 2017/06/02 07:39:56, anton wrote:
> minor: double brackets are not required, it could be just `= (Boolean)
> newValue;`

Acknowledged.

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java
(right):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java:150:
final boolean enabled = ((Boolean) newValue);
On 2017/06/02 07:39:56, anton wrote:
> minor: double brackets are not required, it could be just `= (Boolean)
> newValue;

Acknowledged.

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java
(right):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:47:
import org.adblockplus.sbrowser.contentblocker.MainPreferences;
On 2017/06/02 10:50:01, jens wrote:
> Am I wrong, or is this an unused import?

Yes, but this will be removed by the other NoIssue review, which cleans all
unused stuff

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:536:
continue;
On 2017/06/02 10:50:01, jens wrote:
> I think this continue statement can be deleted.

Yeah, this one will be removed in the cleanup Noissue review as well

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
(right):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:405:
try (final DataOutputStream metaOut = new DataOutputStream(new
BufferedOutputStream(
On 2017/06/02 07:39:56, anton wrote:
> here and below: what's the purpose of buffering before gzipping?
> As for me it seems reasonable to buffer before writing to file to eliminate
file
> io routines (as it was before).
> What am i missing?

Basically, it's because GZIP Streams already hold a buffer inside them, but they
don't expose this buffer. More info here:
https://stackoverflow.com/questions/4438085/seeking-out-the-optimum-size-for-...

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/SubscriptionInfo.java
(right):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/SubscriptionInfo.java:45:
SubscriptionInfo(final Type type, final String id, final String url, final
String title, final boolean enabled, final long lastUpdate)
On 2017/06/02 10:50:01, jens wrote:
> I think this constructor can be private, as we only use the static create()
> method.

Acknowledged.

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java
(left):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:53:
private final boolean wasUnitialized;
On 2017/06/02 10:50:01, jens wrote:
> This member and the corresponding getter have a typo and should be named
> 'wasUninitialized', right? 

Acknowledged.

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:93:
void getSubscriptions(final List<Subscription> list)
On 2017/06/02 10:50:01, jens wrote:
> I think the method name has a typo and should be changed to
> "addSubscriptions(final List<Subscription> list)"

Hmm, maybe loadSubscriptions makes more sense, since it will load the
subscriptions in the provided list, instead of adding the Subscriptions in the
provided list into another (internal) list

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java
(right):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:73:
final File file = new File(this.cacheFolder, String.format(Locale.ENGLISH,
"tmp-%d.txt",
On 2017/06/02 07:39:56, anton wrote:
> what's a purpose of setting locale explicitly?

Mainly because it produces a Lint warning: Implicitly using the default locale
is a common source of bugs: Use String.format(Locale, ...) instead. For this
specific line of code AFAIK, there's no problem for using the default locale,
it's just to get rid of the warning.

[jens, 2017-06-06 09:39:56]

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java
(right):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:47:
import org.adblockplus.sbrowser.contentblocker.MainPreferences;
On 2017/06/02 21:03:42, diegocarloslima wrote:
> On 2017/06/02 10:50:01, jens wrote:
> > Am I wrong, or is this an unused import?
> 
> Yes, but this will be removed by the other NoIssue review, which cleans all
> unused stuff

Acknowledged.

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:536:
continue;
On 2017/06/02 21:03:42, diegocarloslima wrote:
> On 2017/06/02 10:50:01, jens wrote:
> > I think this continue statement can be deleted.
> 
> Yeah, this one will be removed in the cleanup Noissue review as well

Acknowledged.

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java
(left):

https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:93:
void getSubscriptions(final List<Subscription> list)
On 2017/06/02 21:03:42, diegocarloslima wrote:
> On 2017/06/02 10:50:01, jens wrote:
> > I think the method name has a typo and should be changed to
> > "addSubscriptions(final List<Subscription> list)"
> 
> Hmm, maybe loadSubscriptions makes more sense, since it will load the
> subscriptions in the provided list, instead of adding the Subscriptions in the
> provided list into another (internal) list

Acknowledged.

[jens, 2017-06-14 10:43:22]

On 2017/06/06 09:39:56, jens wrote:
>
https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
> File
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java
> (right):
> 
>
https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:47:
> import org.adblockplus.sbrowser.contentblocker.MainPreferences;
> On 2017/06/02 21:03:42, diegocarloslima wrote:
> > On 2017/06/02 10:50:01, jens wrote:
> > > Am I wrong, or is this an unused import?
> > 
> > Yes, but this will be removed by the other NoIssue review, which cleans all
> > unused stuff
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:536:
> continue;
> On 2017/06/02 21:03:42, diegocarloslima wrote:
> > On 2017/06/02 10:50:01, jens wrote:
> > > I think this continue statement can be deleted.
> > 
> > Yeah, this one will be removed in the cleanup Noissue review as well
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
> File
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java
> (left):
> 
>
https://codereview.adblockplus.org/29453722/diff/29453723/adblockplussbrowser...
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:93:
> void getSubscriptions(final List<Subscription> list)
> On 2017/06/02 21:03:42, diegocarloslima wrote:
> > On 2017/06/02 10:50:01, jens wrote:
> > > I think the method name has a typo and should be changed to
> > > "addSubscriptions(final List<Subscription> list)"
> > 
> > Hmm, maybe loadSubscriptions makes more sense, since it will load the
> > subscriptions in the provided list, instead of adding the Subscriptions in
the
> > provided list into another (internal) list
> 
> Acknowledged.

LGTM

[anton, 2017-06-16 13:38:44]

https://codereview.adblockplus.org/29453722/diff/29464683/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
(right):

https://codereview.adblockplus.org/29453722/diff/29464683/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:96:
Collections.addAll(ALLOWED_META_KEYS, ALLOWED_META_KEYS_ARRAY);
Can we replace:
```
private static final HashSet<String> ALLOWED_META_KEYS = new HashSet<>();
...
Collections.addAll(ALLOWED_META_KEYS, ALLOWED_META_KEYS_ARRAY);
```

with just:
```
private static final HashSet<String> ALLOWED_META_KEYS = new
HashSet<>(ALLOWED_META_KEYS_ARRAY);
```
?

[diegocarloslima, 2017-07-19 16:39:24]

https://codereview.adblockplus.org/29453722/diff/29464683/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
(right):

https://codereview.adblockplus.org/29453722/diff/29464683/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:96:
Collections.addAll(ALLOWED_META_KEYS, ALLOWED_META_KEYS_ARRAY);
On 2017/06/16 13:38:44, anton wrote:
> Can we replace:
> ```
> private static final HashSet<String> ALLOWED_META_KEYS = new HashSet<>();
> ...
> Collections.addAll(ALLOWED_META_KEYS, ALLOWED_META_KEYS_ARRAY);
> ```
> 
> with just:
> ```
> private static final HashSet<String> ALLOWED_META_KEYS = new
> HashSet<>(ALLOWED_META_KEYS_ARRAY);
> ```
> ?

Yeah its possible to initialize the HashSet without the static block. Just need
a few more adjustments. I'll upload a new patch

[anton, 2017-07-20 05:39:29]

On 2017/07/19 16:39:24, diegocarloslima wrote:
>
https://codereview.adblockplus.org/29453722/diff/29464683/adblockplussbrowser...
> File
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
> (right):
> 
>
https://codereview.adblockplus.org/29453722/diff/29464683/adblockplussbrowser...
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:96:
> Collections.addAll(ALLOWED_META_KEYS, ALLOWED_META_KEYS_ARRAY);
> On 2017/06/16 13:38:44, anton wrote:
> > Can we replace:
> > ```
> > private static final HashSet<String> ALLOWED_META_KEYS = new HashSet<>();
> > ...
> > Collections.addAll(ALLOWED_META_KEYS, ALLOWED_META_KEYS_ARRAY);
> > ```
> > 
> > with just:
> > ```
> > private static final HashSet<String> ALLOWED_META_KEYS = new
> > HashSet<>(ALLOWED_META_KEYS_ARRAY);
> > ```
> > ?
> 
> Yeah its possible to initialize the HashSet without the static block. Just
need
> a few more adjustments. I'll upload a new patch

If it was patch 3 to init arrays in ctor then it contains too much other
changes. Was it intentionally?

[diegocarloslima, 2017-07-20 11:42:36]

On 2017/07/20 05:39:29, anton wrote:
> On 2017/07/19 16:39:24, diegocarloslima wrote:
> >
>
https://codereview.adblockplus.org/29453722/diff/29464683/adblockplussbrowser...
> > File
> >
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29453722/diff/29464683/adblockplussbrowser...
> >
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:96:
> > Collections.addAll(ALLOWED_META_KEYS, ALLOWED_META_KEYS_ARRAY);
> > On 2017/06/16 13:38:44, anton wrote:
> > > Can we replace:
> > > ```
> > > private static final HashSet<String> ALLOWED_META_KEYS = new HashSet<>();
> > > ...
> > > Collections.addAll(ALLOWED_META_KEYS, ALLOWED_META_KEYS_ARRAY);
> > > ```
> > > 
> > > with just:
> > > ```
> > > private static final HashSet<String> ALLOWED_META_KEYS = new
> > > HashSet<>(ALLOWED_META_KEYS_ARRAY);
> > > ```
> > > ?
> > 
> > Yeah its possible to initialize the HashSet without the static block. Just
> need
> > a few more adjustments. I'll upload a new patch
> 
> If it was patch 3 to init arrays in ctor then it contains too much other
> changes. Was it intentionally?

This is because the patch 2 was a while a go. I was unsure on which revision it
was made the diff, so I rebased the changes to the 'master' revision, that's why
some files have changed. But if you click on the diffs individually, you'll see
that the only change from patch 2 to 3 made by this review was on Subscription
class

[anton, 2017-07-20 11:44:25]

On 2017/07/20 11:42:36, diegocarloslima wrote:
> On 2017/07/20 05:39:29, anton wrote:
> > On 2017/07/19 16:39:24, diegocarloslima wrote:
> > >
> >
>
https://codereview.adblockplus.org/29453722/diff/29464683/adblockplussbrowser...
> > > File
> > >
> >
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.adblockplus.org/29453722/diff/29464683/adblockplussbrowser...
> > >
> >
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:96:
> > > Collections.addAll(ALLOWED_META_KEYS, ALLOWED_META_KEYS_ARRAY);
> > > On 2017/06/16 13:38:44, anton wrote:
> > > > Can we replace:
> > > > ```
> > > > private static final HashSet<String> ALLOWED_META_KEYS = new
HashSet<>();
> > > > ...
> > > > Collections.addAll(ALLOWED_META_KEYS, ALLOWED_META_KEYS_ARRAY);
> > > > ```
> > > > 
> > > > with just:
> > > > ```
> > > > private static final HashSet<String> ALLOWED_META_KEYS = new
> > > > HashSet<>(ALLOWED_META_KEYS_ARRAY);
> > > > ```
> > > > ?
> > > 
> > > Yeah its possible to initialize the HashSet without the static block. Just
> > need
> > > a few more adjustments. I'll upload a new patch
> > 
> > If it was patch 3 to init arrays in ctor then it contains too much other
> > changes. Was it intentionally?
> 
> This is because the patch 2 was a while a go. I was unsure on which revision
it
> was made the diff, so I rebased the changes to the 'master' revision, that's
why
> some files have changed. But if you click on the diffs individually, you'll
see
> that the only change from patch 2 to 3 made by this review was on Subscription
> class

Got it. LGTM then