Issue 6454 - IllegalStateException crash

Issue 6454 - IllegalStateException crash

[jens, 2018-03-08 09:18:32]

On 2018/03/07 20:34:40, diegocarloslima wrote:

I have tested it and it works well! Only one remark in EngineManager.
Did you experience some crashes during testing, or are all the engine != null
checks just a precautionary measure?

[jens, 2018-03-08 09:18:40]

https://codereview.adblockplus.org/29716681/diff/29716682/adblockplussbrowser...
File
adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/EngineManager.java
(right):

https://codereview.adblockplus.org/29716681/diff/29716682/adblockplussbrowser...
adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/EngineManager.java:28:

I didn't want to leave a lot of single comments, but is there a reason that the
method parameters in this class are all not final?

[diegocarloslima, 2018-03-08 11:54:35]

About the nullchecks for the engine, no I didn't experience any crash. I just
added those nullchecks, because they were missing in some parts. Theoretically,
it's possible to have a failed initialization of the Engine due to some
IOException (maybe if the system doesn't allow the creation of files if the
device storage is completely full). We need to further improve this in the
future to show an error dialog and lock out the user from using the app.

https://codereview.adblockplus.org/29716681/diff/29716682/adblockplussbrowser...
File
adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/EngineManager.java
(right):

https://codereview.adblockplus.org/29716681/diff/29716682/adblockplussbrowser...
adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/EngineManager.java:28:

On 2018/03/08 09:18:40, jens wrote:
> I didn't want to leave a lot of single comments, but is there a reason that
the
> method parameters in this class are all not final?

Nope, the reason is just that I don't usually mark the method parameters as
final (only when they're needed in a inner class). But I think we can agree to
when possible, mark everything as final. I think that won't hurt and can
sometimes improve performance.

[jens, 2018-03-08 12:33:55]

https://codereview.adblockplus.org/29716681/diff/29716682/adblockplussbrowser...
File
adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/EngineManager.java
(right):

https://codereview.adblockplus.org/29716681/diff/29716682/adblockplussbrowser...
adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/EngineManager.java:28:

On 2018/03/08 11:54:34, diegocarloslima wrote:
> On 2018/03/08 09:18:40, jens wrote:
> > I didn't want to leave a lot of single comments, but is there a reason that
> the
> > method parameters in this class are all not final?
> 
> Nope, the reason is just that I don't usually mark the method parameters as
> final (only when they're needed in a inner class). But I think we can agree to
> when possible, mark everything as final. I think that won't hurt and can
> sometimes improve performance.

Acknowledged.

[jens, 2018-03-08 16:19:38]

On 2018/03/08 12:33:55, jens wrote:
>
https://codereview.adblockplus.org/29716681/diff/29716682/adblockplussbrowser...
> File
>
adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/EngineManager.java
> (right):
> 
>
https://codereview.adblockplus.org/29716681/diff/29716682/adblockplussbrowser...
>
adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/EngineManager.java:28:
> 
> On 2018/03/08 11:54:34, diegocarloslima wrote:
> > On 2018/03/08 09:18:40, jens wrote:
> > > I didn't want to leave a lot of single comments, but is there a reason
that
> > the
> > > method parameters in this class are all not final?
> > 
> > Nope, the reason is just that I don't usually mark the method parameters as
> > final (only when they're needed in a inner class). But I think we can agree
to
> > when possible, mark everything as final. I think that won't hurt and can
> > sometimes improve performance.
> 
> Acknowledged.

LGTM

[René Jeschke, 2018-03-09 10:11:03]

LGTM

https://codereview.adblockplus.org/29716681/diff/29717591/adblockplussbrowser...
File
adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java
(right):

https://codereview.adblockplus.org/29716681/diff/29717591/adblockplussbrowser...
adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java:129:
else if (sub.getType() == SubscriptionInfo.Type.CUSTOM && engine != null)
NIT: Unrelated change, but I don't mind. It's up to you.