Issue 29399749: Issue 5081 - Make libadblockplus-android users supply the application and applicationVersion parame… (Closed)
DescriptionIssue 5081 - Make libadblockplus-android users supply the application and applicationVersion parame…
Patch Set 1 #
Total comments: 20
Patch Set 2 : minor reformatting #
Total comments: 1
Patch Set 3 : updated javadocs from .h #Patch Set 4 : application/-Version are required now #
MessagesTotal messages: 11
https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (left): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:90: version += "." + info.versionCode; addonVersion is generated, but we need hardcoded value
https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (left): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:25: import org.adblockplus.libadblockplus.UpdateCheckDoneCallback; not related to the task directly https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:31: import java.util.Map; not related to the task directly - just cleanup
it seems, it should work, just in case few comments. https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java (right): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:50: .setApplicationVersion("app.version"); is "app.version" a correct value? I thought the expected format is a sequence of numbers. https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (left): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:90: version += "." + info.versionCode; On 2017/03/31 13:30:27, anton wrote: > addonVersion is generated, but we need hardcoded value I'm not sure that we should use a hard coded value instead of a generated one. Could we update version values in some manifest, where versionName and versionCode are defined? https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:108: public static AppInfo generateAppInfo(final Context context, boolean developmentBuild) So, currently application and applicationVersion are optional, should not this method be removed to cause a compilation error which should be a notification for others to pass some values?
https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java (right): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:50: .setApplicationVersion("app.version"); On 2017/03/31 13:49:09, sergei wrote: > is "app.version" a correct value? I thought the expected format is a sequence of > numbers. it accepts any value as it's just a string https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (left): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:90: version += "." + info.versionCode; On 2017/03/31 13:49:09, sergei wrote: > On 2017/03/31 13:30:27, anton wrote: > > addonVersion is generated, but we need hardcoded value > > I'm not sure that we should use a hard coded value instead of a generated one. > Could we update version values in some manifest, where versionName and > versionCode are defined? it's not clear from the task. i think the intention was to get fixed expected number. if we don't remove like i did we will get generated (unexpected) version. https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:108: public static AppInfo generateAppInfo(final Context context, boolean developmentBuild) On 2017/03/31 13:49:09, sergei wrote: > So, currently application and applicationVersion are optional, should not this > method be removed to cause a compilation error which should be a notification > for others to pass some values? there will be no compilation errors here
https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java (right): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:50: .setApplicationVersion("app.version"); On 2017/03/31 13:52:25, anton wrote: > On 2017/03/31 13:49:09, sergei wrote: > > is "app.version" a correct value? I thought the expected format is a sequence > of > > numbers. > > it accepts any value as it's just a string Felix, what about server side and statistics script? https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (left): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:90: version += "." + info.versionCode; On 2017/03/31 13:52:26, anton wrote: > On 2017/03/31 13:49:09, sergei wrote: > > On 2017/03/31 13:30:27, anton wrote: > > > addonVersion is generated, but we need hardcoded value > > > > I'm not sure that we should use a hard coded value instead of a generated one. > > Could we update version values in some manifest, where versionName and > > versionCode are defined? > > it's not clear from the task. i think the intention was to get fixed expected > number. if we don't remove like i did we will get generated (unexpected) > version. I thought we control this version because it's our packages, don't we? https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:108: public static AppInfo generateAppInfo(final Context context, boolean developmentBuild) On 2017/03/31 13:52:26, anton wrote: > On 2017/03/31 13:49:09, sergei wrote: > > So, currently application and applicationVersion are optional, should not this > > method be removed to cause a compilation error which should be a notification > > for others to pass some values? > > there will be no compilation errors here Then I think it should be removed.
https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (left): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:90: version += "." + info.versionCode; On 2017/03/31 14:01:52, sergei wrote: > On 2017/03/31 13:52:26, anton wrote: > > On 2017/03/31 13:49:09, sergei wrote: > > > On 2017/03/31 13:30:27, anton wrote: > > > > addonVersion is generated, but we need hardcoded value > > > > > > I'm not sure that we should use a hard coded value instead of a generated > one. > > > Could we update version values in some manifest, where versionName and > > > versionCode are defined? > > > > it's not clear from the task. i think the intention was to get fixed expected > > number. if we don't remove like i did we will get generated (unexpected) > > version. > > I thought we control this version because it's our packages, don't we? no, it's app package, not library https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:108: public static AppInfo generateAppInfo(final Context context, boolean developmentBuild) On 2017/03/31 14:01:52, sergei wrote: > On 2017/03/31 13:52:26, anton wrote: > > On 2017/03/31 13:49:09, sergei wrote: > > > So, currently application and applicationVersion are optional, should not > this > > > method be removed to cause a compilation error which should be a > notification > > > for others to pass some values? > > > > there will be no compilation errors here > > Then I think it should be removed. not clear what you've meant. if the user(app developer) isn't using helper it can still use this method without application and applicationVersion.
LGTM (my comment can be addressed in a separate noissue commit) https://codereview.adblockplus.org/29399749/diff/29399755/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29399749/diff/29399755/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:137: * Used for requests to identify the client Nit: We should probably use more concrete descriptions, e.g. those from: https://github.com/adblockplus/libadblockplus/blob/master/include/AdblockPlus...
https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (left): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:25: import org.adblockplus.libadblockplus.UpdateCheckDoneCallback; On 2017/03/31 13:34:21, anton wrote: > not related to the task directly I would prefer this to be in a separate task, but since this is an urgent code review, I wouldn't insist that much https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:153: return this; By the ticket description, it seems that application and applicationVersion should be mandatory instead of optional. If so, this should be included in init() method https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java (right): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:50: .setApplicationVersion("app.version"); It would be a more intuitive example here to provide a number instead of a string
https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:108: public static AppInfo generateAppInfo(final Context context, boolean developmentBuild) On 2017/03/31 14:05:27, anton wrote: > On 2017/03/31 14:01:52, sergei wrote: > > On 2017/03/31 13:52:26, anton wrote: > > > On 2017/03/31 13:49:09, sergei wrote: > > > > So, currently application and applicationVersion are optional, should not > > this > > > > method be removed to cause a compilation error which should be a > > notification > > > > for others to pass some values? > > > > > > there will be no compilation errors here > > > > Then I think it should be removed. > > not clear what you've meant. > if the user(app developer) isn't using helper it can still use this method > without application and applicationVersion. That was the idea to don't allow to use this method and encourage the user to pass values.
https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:153: return this; On 2017/03/31 14:07:14, diegocarloslima wrote: > By the ticket description, it seems that application and applicationVersion > should be mandatory instead of optional. If so, this should be included in > init() method if they are required then i agree. it's not clear from the task https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... File libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java (right): https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:50: .setApplicationVersion("app.version"); On 2017/03/31 14:07:14, diegocarloslima wrote: > It would be a more intuitive example here to provide a number instead of a > string it's just an example. it's up to app developer to decide in what format he should pass it or up to us not to let him pass invalid values.
On 2017/03/31 14:16:08, anton wrote: > https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... > File > libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java > (right): > > https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... > libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:153: > return this; > On 2017/03/31 14:07:14, diegocarloslima wrote: > > By the ticket description, it seems that application and applicationVersion > > should be mandatory instead of optional. If so, this should be included in > > init() method > > if they are required then i agree. it's not clear from the task > > https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... > File > libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java > (right): > > https://codereview.adblockplus.org/29399749/diff/29399750/libadblockplus-andr... > libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:50: > .setApplicationVersion("app.version"); > On 2017/03/31 14:07:14, diegocarloslima wrote: > > It would be a more intuitive example here to provide a number instead of a > > string > > it's just an example. it's up to app developer to decide in what format he > should pass it or up to us not to let him pass invalid values. LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
