Issue 5223 - App is freezing sometimes

Issue 5223 - App is freezing sometimes

[diegocarloslima, 2017-05-25 21:20:45]

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

https://codereview.adblockplus.org/29445587/diff/29445588/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:283:
new CreateAndWriteFileAsyncTask().execute(serviceContext, subscriptions);
I might be wrong, but it seems that the freezing issue isn't related to the need
of running an AsyncTask for writing the filters. From what I could see, this
method is always called by a background thread. The problem seems to lie in the
lock() / unlock() calls. They're being called in the main thread on
Engine.getListedSubscriptions() and Engine.requestUpdateBroadcast(), which
should never happen. You might need to investigate if removing these calls
produces any side effect and if we really need to make the main thread wait for
some of these background operation, in which case we should show a progress
dialog instead of freezing the main thread

[jens, 2017-06-01 15:32:08]

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

https://codereview.adblockplus.org/29445587/diff/29445588/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:283:
new CreateAndWriteFileAsyncTask().execute(serviceContext, subscriptions);
On 2017/05/25 21:20:45, diegocarloslima wrote:
> I might be wrong, but it seems that the freezing issue isn't related to the
need
> of running an AsyncTask for writing the filters. From what I could see, this
> method is always called by a background thread. The problem seems to lie in
the
> lock() / unlock() calls. They're being called in the main thread on
> Engine.getListedSubscriptions() and Engine.requestUpdateBroadcast(), which
> should never happen. You might need to investigate if removing these calls
> produces any side effect and if we really need to make the main thread wait
for
> some of these background operation, in which case we should show a progress
> dialog instead of freezing the main thread

Acknowledged.

[anton, 2017-06-26 13:49:35]

https://codereview.adblockplus.org/29445587/diff/29470652/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java
(right):

https://codereview.adblockplus.org/29445587/diff/29470652/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:266:
dialog.dismiss();
don't we need to `dialog = null` after it's dismissed?

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

https://codereview.adblockplus.org/29445587/diff/29470652/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:192:
public void setSubscriptionUpdateCallback(final SubscriptionUpdateCallback
subscriptionUpdateCallback) {
"{" should be on the next line

https://codereview.adblockplus.org/29445587/diff/29470652/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:809:
public interface SubscriptionUpdateCallback {
"{" should be on the next line

[jens, 2017-06-26 14:05:20]

https://codereview.adblockplus.org/29445587/diff/29470652/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java
(right):

https://codereview.adblockplus.org/29445587/diff/29470652/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:266:
dialog.dismiss();
On 2017/06/26 13:49:34, anton wrote:
> don't we need to `dialog = null` after it's dismissed?

Yeah, you're right. I think it's cleaner. I will use the dismissDialog() method
already exists.

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

https://codereview.adblockplus.org/29445587/diff/29470652/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:192:
public void setSubscriptionUpdateCallback(final SubscriptionUpdateCallback
subscriptionUpdateCallback) {
On 2017/06/26 13:49:34, anton wrote:
> "{" should be on the next line

Acknowledged.

https://codereview.adblockplus.org/29445587/diff/29470652/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:809:
public interface SubscriptionUpdateCallback {
On 2017/06/26 13:49:34, anton wrote:
> "{" should be on the next line

Acknowledged.

[anton, 2017-06-26 14:45:54]

https://codereview.adblockplus.org/29445587/diff/29474598/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java
(right):

https://codereview.adblockplus.org/29445587/diff/29474598/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:257:
this.dialog = ProgressDialog.show(this, null, enabled ?
would it not be better to have it like this ".. enabled
  ? ..
  : .."

not insisting though

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

https://codereview.adblockplus.org/29445587/diff/29474598/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:268:
public void subscriptionStateChanged() {
"{" should be on the next line.
LGTM in general