Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code
Issues
All Issues

Issue 29691555: Issue 6364 - Exclude double initialization of AdblockHelper (Closed)

Created:
Feb. 7, 2018, 9:28 a.m. by anton
Modified:
Feb. 14, 2018, 7:21 a.m.
Reviewers:
diegocarloslima, jens
CC:
René Jeschke
Visibility:
Public.

Description

Issue 6364 - Exclude double initialization of AdblockHelper

Patch Set 1 #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+40 lines, -13 lines) Patch
M README.md View 1 chunk +8 lines, -0 lines 0 comments Download
M libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java View 2 chunks +15 lines, -0 lines 3 comments Download
M libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java View 1 chunk +17 lines, -13 lines 6 comments Download

Messages

Total messages: 5
anton
Feb. 7, 2018, 9:29 a.m. (2018-02-07 09:29:12 UTC) #1
jens
https://codereview.adblockplus.org/29691555/diff/29691556/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29691555/diff/29691556/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java#newcode160 libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:160: public boolean isInit() { for consistency I would probably ...
Feb. 7, 2018, 9:36 a.m. (2018-02-07 09:36:25 UTC) #2
anton
https://codereview.adblockplus.org/29691555/diff/29691556/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29691555/diff/29691556/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java#newcode160 libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:160: public boolean isInit() { On 2018/02/07 09:36:25, jens wrote: ...
Feb. 7, 2018, 9:52 a.m. (2018-02-07 09:52:55 UTC) #3
jens
LGTM, besides the minor remarks https://codereview.adblockplus.org/29691555/diff/29691556/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29691555/diff/29691556/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java#newcode160 libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:160: public boolean isInit() { ...
Feb. 7, 2018, 10:03 a.m. (2018-02-07 10:03:23 UTC) #4
diegocarloslima
Feb. 7, 2018, 2:57 p.m. (2018-02-07 14:57:52 UTC) #5 
On 2018/02/07 10:03:23, jens wrote:
> LGTM, besides the minor remarks
> 
>
https://codereview.adblockplus.org/29691555/diff/29691556/libadblockplus-andr...
> File
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
> (right):
> 
>
https://codereview.adblockplus.org/29691555/diff/29691556/libadblockplus-andr...
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:160:
> public boolean isInit() {
> On 2018/02/07 09:52:55, anton wrote:
> > On 2018/02/07 09:36:25, jens wrote:
> > > for consistency I would probably call this method isInitialized()
> > 
> > initialize method is called `init()` to i've names it `isInit()`.
> > It's undesired to rename `init()` method as it break back compatibility.
> 
> Okay, I don't insist to change it. 
> But it's kind of inconsistent to me that the boolean is called isInitialized
and
> the method is called isInit().
> 
>
https://codereview.adblockplus.org/29691555/diff/29691556/libadblockplus-andr...
> File
>
libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java
> (right):
> 
>
https://codereview.adblockplus.org/29691555/diff/29691556/libadblockplus-andr...
>
libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:61:
> String basePath = getDir(AdblockEngine.BASE_PATH_DIRECTORY,
> Context.MODE_PRIVATE).getAbsolutePath();
> On 2018/02/07 09:52:55, anton wrote:
> > On 2018/02/07 09:36:25, jens wrote:
> > > basePath can be final
> > 
> > agree, but it's unrelated to the task.
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29691555/diff/29691556/libadblockplus-andr...
>
libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:64:
> Map<String, Integer> map = new HashMap<String, Integer>();
> On 2018/02/07 09:52:55, anton wrote:
> > On 2018/02/07 09:36:25, jens wrote:
> > > I would use a more descriptive name like 'subscriptions' for the map an
also
> > > make it final
> > 
> > agree too, but it's unrelated to the task too.
> 
> Acknowledged.

LGTM

Powered by Google App Engine
This is Rietveld