Issue 29760569: Issue 6238 - Download/store notifications.json (Closed)
DescriptionIssue 6238 - Download/store notifications.json
Patch Set 1 : #
Total comments: 13
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #MessagesTotal messages: 19
https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... File adblockplussbrowser/AndroidManifest.xml (right): https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... adblockplussbrowser/AndroidManifest.xml:42: android:permission="android.permission.BIND_JOB_SERVICE" /> should not we add `android:exported="false"` here? https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java (right): https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java:43: is empty line required here? https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java:44: static final String TAG = DownloadJobService.class.getSimpleName(); shouldn't it be `private`? https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java:46: private Engine engine; i think `engine` should be initialized as `null` to be consistent with `downloadJobAsyncTask` (line above) or `downloadJobAsyncTask` not initialized to `null`. https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java (right): https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:70: editor.putLong(context.getString(keyResId), value).apply(); since `apply` is asynchronous can it happen that some other background thread can call it while other background thread just called it and it's saving previous preferences behind the scene? i can see `putString` is working that way too but just curious.
https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... File adblockplussbrowser/AndroidManifest.xml (right): https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... adblockplussbrowser/AndroidManifest.xml:42: android:permission="android.permission.BIND_JOB_SERVICE" /> On 2018/05/03 07:23:20, anton wrote: > should not we add `android:exported="false"` here? The default value of android:exported is false if no intent filters are used for the service. So it's already set to false in this case. https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java (right): https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java:43: On 2018/05/03 07:23:20, anton wrote: > is empty line required here? Acknowledged. https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java:44: static final String TAG = DownloadJobService.class.getSimpleName(); On 2018/05/03 07:23:20, anton wrote: > shouldn't it be `private`? Acknowledged. https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java:46: private Engine engine; On 2018/05/03 07:23:20, anton wrote: > i think `engine` should be initialized as `null` to be consistent with > `downloadJobAsyncTask` (line above) or `downloadJobAsyncTask` not initialized to > `null`. Acknowledged. https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java (right): https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:70: editor.putLong(context.getString(keyResId), value).apply(); On 2018/05/03 07:23:20, anton wrote: > since `apply` is asynchronous can it happen that some other background thread > can call it while other background thread just called it and it's saving > previous preferences behind the scene? > > i can see `putString` is working that way too but just curious. The docs say if you save with apply() and immediately read with any getX-method, the new value will be returned. If you called apply() at some point and it's still executing, any calls to commit() will block until all past apply-calls and the current commit-call are finished.
https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... File adblockplussbrowser/AndroidManifest.xml (right): https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... adblockplussbrowser/AndroidManifest.xml:42: android:permission="android.permission.BIND_JOB_SERVICE" /> On 2018/05/03 07:44:37, jens wrote: > On 2018/05/03 07:23:20, anton wrote: > > should not we add `android:exported="false"` here? > > The default value of android:exported is false if no intent filters are used for > the service. So it's already set to false in this case. Acknowledged. https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java (right): https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:70: editor.putLong(context.getString(keyResId), value).apply(); On 2018/05/03 07:44:37, jens wrote: > On 2018/05/03 07:23:20, anton wrote: > > since `apply` is asynchronous can it happen that some other background thread > > can call it while other background thread just called it and it's saving > > previous preferences behind the scene? > > > > i can see `putString` is working that way too but just curious. > > The docs say if you save with apply() and immediately read with any getX-method, > the new value will be returned. > If you called apply() at some point and it's still executing, any calls to > commit() will block until all past apply-calls and the current commit-call are > finished. yes, but i mean another case: background thread #1 calls `apply` and did not finish until background thread #2 calls `apply` again (so it overwrite/looses changes by thread #1)
https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java (right): https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:70: editor.putLong(context.getString(keyResId), value).apply(); On 2018/05/03 07:56:26, anton wrote: > On 2018/05/03 07:44:37, jens wrote: > > On 2018/05/03 07:23:20, anton wrote: > > > since `apply` is asynchronous can it happen that some other background > thread > > > can call it while other background thread just called it and it's saving > > > previous preferences behind the scene? > > > > > > i can see `putString` is working that way too but just curious. > > > > The docs say if you save with apply() and immediately read with any > getX-method, > > the new value will be returned. > > If you called apply() at some point and it's still executing, any calls to > > commit() will block until all past apply-calls and the current commit-call are > > finished. > > yes, but i mean another case: background thread #1 calls `apply` and did not > finish until background thread #2 calls `apply` again (so it overwrite/looses > changes by thread #1) Yeah, I think in this case #1 will be overwritten by #2 as soon as it finished. Does that make you see potential problems with my changes?
On 2018/05/03 08:09:15, jens wrote: > https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java > (right): > > https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:70: > editor.putLong(context.getString(keyResId), value).apply(); > On 2018/05/03 07:56:26, anton wrote: > > On 2018/05/03 07:44:37, jens wrote: > > > On 2018/05/03 07:23:20, anton wrote: > > > > since `apply` is asynchronous can it happen that some other background > > thread > > > > can call it while other background thread just called it and it's saving > > > > previous preferences behind the scene? > > > > > > > > i can see `putString` is working that way too but just curious. > > > > > > The docs say if you save with apply() and immediately read with any > > getX-method, > > > the new value will be returned. > > > If you called apply() at some point and it's still executing, any calls to > > > commit() will block until all past apply-calls and the current commit-call > are > > > finished. > > > > yes, but i mean another case: background thread #1 calls `apply` and did not > > finish until background thread #2 calls `apply` again (so it overwrite/looses > > changes by thread #1) > > Yeah, I think in this case #1 will be overwritten by #2 as soon as it finished. > Does that make you see potential problems with my changes? May be it makes sense to `commit()` instead of `apply()` if it happens not in UI thread.
On 2018/05/03 08:15:45, anton wrote: > On 2018/05/03 08:09:15, jens wrote: > > > https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... > > File > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java > > (right): > > > > > https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:70: > > editor.putLong(context.getString(keyResId), value).apply(); > > On 2018/05/03 07:56:26, anton wrote: > > > On 2018/05/03 07:44:37, jens wrote: > > > > On 2018/05/03 07:23:20, anton wrote: > > > > > since `apply` is asynchronous can it happen that some other background > > > thread > > > > > can call it while other background thread just called it and it's saving > > > > > previous preferences behind the scene? > > > > > > > > > > i can see `putString` is working that way too but just curious. > > > > > > > > The docs say if you save with apply() and immediately read with any > > > getX-method, > > > > the new value will be returned. > > > > If you called apply() at some point and it's still executing, any calls to > > > > commit() will block until all past apply-calls and the current commit-call > > are > > > > finished. > > > > > > yes, but i mean another case: background thread #1 calls `apply` and did not > > > finish until background thread #2 calls `apply` again (so it > overwrite/looses > > > changes by thread #1) > > > > Yeah, I think in this case #1 will be overwritten by #2 as soon as it > finished. > > Does that make you see potential problems with my changes? > > May be it makes sense to `commit()` instead of `apply()` if it happens not in UI > thread. I think it's safer to use apply(), because we don't have to make sure that this action always happens in a background thread. In addition to that, our thresholds for updating/downloading are long enough to don't get any problems because of a few seconds or milliseconds difference.
On 2018/05/03 08:37:55, jens wrote: > On 2018/05/03 08:15:45, anton wrote: > > On 2018/05/03 08:09:15, jens wrote: > > > > > > https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... > > > File > > > > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java > > > (right): > > > > > > > > > https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... > > > > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:70: > > > editor.putLong(context.getString(keyResId), value).apply(); > > > On 2018/05/03 07:56:26, anton wrote: > > > > On 2018/05/03 07:44:37, jens wrote: > > > > > On 2018/05/03 07:23:20, anton wrote: > > > > > > since `apply` is asynchronous can it happen that some other background > > > > thread > > > > > > can call it while other background thread just called it and it's > saving > > > > > > previous preferences behind the scene? > > > > > > > > > > > > i can see `putString` is working that way too but just curious. > > > > > > > > > > The docs say if you save with apply() and immediately read with any > > > > getX-method, > > > > > the new value will be returned. > > > > > If you called apply() at some point and it's still executing, any calls > to > > > > > commit() will block until all past apply-calls and the current > commit-call > > > are > > > > > finished. > > > > > > > > yes, but i mean another case: background thread #1 calls `apply` and did > not > > > > finish until background thread #2 calls `apply` again (so it > > overwrite/looses > > > > changes by thread #1) > > > > > > Yeah, I think in this case #1 will be overwritten by #2 as soon as it > > finished. > > > Does that make you see potential problems with my changes? > > > > May be it makes sense to `commit()` instead of `apply()` if it happens not in > UI > > thread. > > I think it's safer to use apply(), because we don't have to make sure that this > action always happens in a background thread. In addition to that, our > thresholds for updating/downloading are long enough to don't get any problems > because of a few seconds or milliseconds difference. ok
On 2018/05/03 09:08:38, anton wrote: > On 2018/05/03 08:37:55, jens wrote: > > On 2018/05/03 08:15:45, anton wrote: > > > On 2018/05/03 08:09:15, jens wrote: > > > > > > > > > > https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... > > > > File > > > > > > > > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java > > > > (right): > > > > > > > > > > > > > > https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... > > > > > > > > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:70: > > > > editor.putLong(context.getString(keyResId), value).apply(); > > > > On 2018/05/03 07:56:26, anton wrote: > > > > > On 2018/05/03 07:44:37, jens wrote: > > > > > > On 2018/05/03 07:23:20, anton wrote: > > > > > > > since `apply` is asynchronous can it happen that some other > background > > > > > thread > > > > > > > can call it while other background thread just called it and it's > > saving > > > > > > > previous preferences behind the scene? > > > > > > > > > > > > > > i can see `putString` is working that way too but just curious. > > > > > > > > > > > > The docs say if you save with apply() and immediately read with any > > > > > getX-method, > > > > > > the new value will be returned. > > > > > > If you called apply() at some point and it's still executing, any > calls > > to > > > > > > commit() will block until all past apply-calls and the current > > commit-call > > > > are > > > > > > finished. > > > > > > > > > > yes, but i mean another case: background thread #1 calls `apply` and did > > not > > > > > finish until background thread #2 calls `apply` again (so it > > > overwrite/looses > > > > > changes by thread #1) > > > > > > > > Yeah, I think in this case #1 will be overwritten by #2 as soon as it > > > finished. > > > > Does that make you see potential problems with my changes? > > > > > > May be it makes sense to `commit()` instead of `apply()` if it happens not > in > > UI > > > thread. > > > > I think it's safer to use apply(), because we don't have to make sure that > this > > action always happens in a background thread. In addition to that, our > > thresholds for updating/downloading are long enough to don't get any problems > > because of a few seconds or milliseconds difference. > > ok uploaded another oatch set
On 2018/05/03 11:20:25, jens wrote: > On 2018/05/03 09:08:38, anton wrote: > > On 2018/05/03 08:37:55, jens wrote: > > > On 2018/05/03 08:15:45, anton wrote: > > > > On 2018/05/03 08:09:15, jens wrote: > > > > > > > > > > > > > > > https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... > > > > > File > > > > > > > > > > > > > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.adblockplus.org/29760569/diff/29762606/adblockplussbrowser... > > > > > > > > > > > > > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:70: > > > > > editor.putLong(context.getString(keyResId), value).apply(); > > > > > On 2018/05/03 07:56:26, anton wrote: > > > > > > On 2018/05/03 07:44:37, jens wrote: > > > > > > > On 2018/05/03 07:23:20, anton wrote: > > > > > > > > since `apply` is asynchronous can it happen that some other > > background > > > > > > thread > > > > > > > > can call it while other background thread just called it and it's > > > saving > > > > > > > > previous preferences behind the scene? > > > > > > > > > > > > > > > > i can see `putString` is working that way too but just curious. > > > > > > > > > > > > > > The docs say if you save with apply() and immediately read with any > > > > > > getX-method, > > > > > > > the new value will be returned. > > > > > > > If you called apply() at some point and it's still executing, any > > calls > > > to > > > > > > > commit() will block until all past apply-calls and the current > > > commit-call > > > > > are > > > > > > > finished. > > > > > > > > > > > > yes, but i mean another case: background thread #1 calls `apply` and > did > > > not > > > > > > finish until background thread #2 calls `apply` again (so it > > > > overwrite/looses > > > > > > changes by thread #1) > > > > > > > > > > Yeah, I think in this case #1 will be overwritten by #2 as soon as it > > > > finished. > > > > > Does that make you see potential problems with my changes? > > > > > > > > May be it makes sense to `commit()` instead of `apply()` if it happens not > > in > > > UI > > > > thread. > > > > > > I think it's safer to use apply(), because we don't have to make sure that > > this > > > action always happens in a background thread. In addition to that, our > > > thresholds for updating/downloading are long enough to don't get any > problems > > > because of a few seconds or milliseconds difference. > > > > ok > > uploaded another oatch set LGTM
https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java (right): https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java:185: private int responseCode = 0; No need to initialize it with 0 here, this is done by default https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Engine.java (right): https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:795: if (sub.getURL() != null && sub.shouldUpdate(forced)) this else + if could be converted to a single else if statement, but I wouldn't insist if you feel this way is more readable https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Notification.java (right): https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Notification.java:40: public static final String NOTIFICATION_DATA_FILE_NAME = TAG.toLowerCase() + ".abp"; For me, its not a good thing to tie the filename to the log tag. We could for any reason refactor the log tags in the future and it could lead to an undesired change in the name of the notification filename. I would prefer to have it as a simple string constant "notification.abp"
https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java (right): https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java:185: private int responseCode = 0; On 2018/05/10 13:41:58, diegocarloslima wrote: > No need to initialize it with 0 here, this is done by default Acknowledged. https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Engine.java (right): https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:795: if (sub.getURL() != null && sub.shouldUpdate(forced)) On 2018/05/10 13:41:58, diegocarloslima wrote: > this else + if could be converted to a single else if statement, but I wouldn't > insist if you feel this way is more readable Yeah, right. I have tried a few different things already and came to the conclusion, that this is the most readable way IMHO. But same as you, I don't insist keeping it like this if you think there is a more readable approach. https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Notification.java (right): https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Notification.java:40: public static final String NOTIFICATION_DATA_FILE_NAME = TAG.toLowerCase() + ".abp"; On 2018/05/10 13:41:59, diegocarloslima wrote: > For me, its not a good thing to tie the filename to the log tag. We could for > any reason refactor the log tags in the future and it could lead to an undesired > change in the name of the notification filename. I would prefer to have it as a > simple string constant "notification.abp" Acknowledged.
On 2018/05/15 09:17:02, jens wrote: > https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java > (right): > > https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java:185: > private int responseCode = 0; > On 2018/05/10 13:41:58, diegocarloslima wrote: > > No need to initialize it with 0 here, this is done by default > > Acknowledged. > > https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Engine.java > (right): > > https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:795: > if (sub.getURL() != null && sub.shouldUpdate(forced)) > On 2018/05/10 13:41:58, diegocarloslima wrote: > > this else + if could be converted to a single else if statement, but I > wouldn't > > insist if you feel this way is more readable > > Yeah, right. I have tried a few different things already and came to the > conclusion, that this is the most readable way IMHO. > But same as you, I don't insist keeping it like this if you think there is a > more readable approach. > > https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Notification.java > (right): > > https://codereview.adblockplus.org/29760569/diff/29769555/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Notification.java:40: > public static final String NOTIFICATION_DATA_FILE_NAME = TAG.toLowerCase() + > ".abp"; > On 2018/05/10 13:41:59, diegocarloslima wrote: > > For me, its not a good thing to tie the filename to the log tag. We could for > > any reason refactor the log tags in the future and it could lead to an > undesired > > change in the name of the notification filename. I would prefer to have it as > a > > simple string constant "notification.abp" > > Acknowledged. Uploaded a new patch set
https://codereview.adblockplus.org/29760569/diff/29782558/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java (right): https://codereview.adblockplus.org/29760569/diff/29782558/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java:187: private String responseText = null; what about removing init to `null` here too? (if you've removed 0 above)
https://codereview.adblockplus.org/29760569/diff/29782558/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java (right): https://codereview.adblockplus.org/29760569/diff/29782558/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java:187: private String responseText = null; On 2018/05/15 09:55:56, anton wrote: > what about removing init to `null` here too? (if you've removed 0 above) Acknowledged. I removed it and added a new patch set
On 2018/05/15 09:57:46, jens wrote: > https://codereview.adblockplus.org/29760569/diff/29782558/adblockplussbrowser... > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java > (right): > > https://codereview.adblockplus.org/29760569/diff/29782558/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java:187: > private String responseText = null; > On 2018/05/15 09:55:56, anton wrote: > > what about removing init to `null` here too? (if you've removed 0 above) > > Acknowledged. > I removed it and added a new patch set LGTM
On 2018/05/15 10:37:06, anton wrote: > On 2018/05/15 09:57:46, jens wrote: > > > https://codereview.adblockplus.org/29760569/diff/29782558/adblockplussbrowser... > > File > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java > > (right): > > > > > https://codereview.adblockplus.org/29760569/diff/29782558/adblockplussbrowser... > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java:187: > > private String responseText = null; > > On 2018/05/15 09:55:56, anton wrote: > > > what about removing init to `null` here too? (if you've removed 0 above) > > > > Acknowledged. > > I removed it and added a new patch set > > LGTM LGTM
On 2018/05/22 13:04:27, diegocarloslima wrote: > On 2018/05/15 10:37:06, anton wrote: > > On 2018/05/15 09:57:46, jens wrote: > > > > > > https://codereview.adblockplus.org/29760569/diff/29782558/adblockplussbrowser... > > > File > > > > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java > > > (right): > > > > > > > > > https://codereview.adblockplus.org/29760569/diff/29782558/adblockplussbrowser... > > > > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java:187: > > > private String responseText = null; > > > On 2018/05/15 09:55:56, anton wrote: > > > > what about removing init to `null` here too? (if you've removed 0 above) > > > > > > Acknowledged. > > > I removed it and added a new patch set > > > > LGTM > > LGTM Is it closed? The ticket is still in `reviewing` state.
On 2018/07/02 10:07:41, anton wrote: > On 2018/05/22 13:04:27, diegocarloslima wrote: > > On 2018/05/15 10:37:06, anton wrote: > > > On 2018/05/15 09:57:46, jens wrote: > > > > > > > > > > https://codereview.adblockplus.org/29760569/diff/29782558/adblockplussbrowser... > > > > File > > > > > > > > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java > > > > (right): > > > > > > > > > > > > > > https://codereview.adblockplus.org/29760569/diff/29782558/adblockplussbrowser... > > > > > > > > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/DownloadJobService.java:187: > > > > private String responseText = null; > > > > On 2018/05/15 09:55:56, anton wrote: > > > > > what about removing init to `null` here too? (if you've removed 0 above) > > > > > > > > Acknowledged. > > > > I removed it and added a new patch set > > > > > > LGTM > > > > LGTM > > Is it closed? The ticket is still in `reviewing` state. We waiting for the China release (which will happen in the next days) before we land this changes. 
|
