Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(477)

Issue 29345540: Issue 4030 - Move JNI bindings into separate library project (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 10 months ago by anton
Modified:
2 years, 6 months ago
CC:
René Jeschke
Visibility:
Public.

Description

Issue 4030 - Move JNI bindings into separate library project

Patch Set 1 #

Total comments: 10

Patch Set 2 : Changeset in adblockplusandroid repo #

Total comments: 2

Patch Set 3 : Additional changes to libadblockplus-android with Diego's suggestions #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -2 lines) Patch
M libadblockplus-android/AndroidManifest.xml View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M libadblockplus-android/build.xml View 1 2 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 6
anton
since the things changed (created separate libadblockplus-android repo) i've modified the structure of directories keeping ...
3 years, 10 months ago (2016-06-03 13:49:14 UTC) #1
anton
Assuming patchset #1 is applied in fact (we discussed it with Rene and decided to ...
3 years, 8 months ago (2016-07-22 12:23:36 UTC) #2
diegocarloslima
https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblockplus-android/libadblockplus-android/AndroidManifest.xml File submodules/libadblockplus-android/libadblockplus-android/AndroidManifest.xml (right): https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblockplus-android/libadblockplus-android/AndroidManifest.xml#newcode5 submodules/libadblockplus-android/libadblockplus-android/AndroidManifest.xml:5: android:versionName="1.3" > Since libadblockplus-android will be an independent project ...
3 years, 7 months ago (2016-09-06 01:05:51 UTC) #3
anton
https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblockplus-android/libadblockplus-android/AndroidManifest.xml File submodules/libadblockplus-android/libadblockplus-android/AndroidManifest.xml (right): https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblockplus-android/libadblockplus-android/AndroidManifest.xml#newcode5 submodules/libadblockplus-android/libadblockplus-android/AndroidManifest.xml:5: android:versionName="1.3" > On 2016/09/06 01:05:49, diegocarloslima wrote: > Since ...
3 years, 7 months ago (2016-09-06 05:56:24 UTC) #4
diegocarloslima
https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java File submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java (right): https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java#newcode45 submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java:45: public static String capitalizeString(final String s) On 2016/09/06 05:56:23, ...
3 years, 7 months ago (2016-09-06 14:08:59 UTC) #5
anton
3 years, 6 months ago (2016-09-08 13:43:49 UTC) #6
https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo...
File
submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java
(right):

https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo...
submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java:45:
public static String capitalizeString(final String s)
On 2016/09/06 14:08:58, diegocarloslima wrote:
> On 2016/09/06 05:56:23, anton wrote:
> > On 2016/09/06 01:05:49, diegocarloslima wrote:
> > > Why this method is kept here? It isn't called anywhere in the lib. This
> should
> > > be placed in the application AppUtils
> > 
> > The original task was to split the sources and don't introduce any changes
> like
> > remove (unused) methods. So this should be done in separate task like
> > "clean-up".
> 
> It doesn't make much sense to me, since some changes were already made to move
> utility methods that are only used by the application to AppUtils, e.g.
> showUpdateNotification() and updateSubscriptionStatus().
> 
> This method is only required by AdblockPlus.java and IMHO it makes no sense
for
> an utility method only required by the application part to stay in the lib
part 

In general you are right. But this method is general-purpose and it's not
related to app context. In contrast AppUtils class methods relate to context or
app entities like activities, so i'd prefer to have this function here. Honestly
StringUtils methods should be used instead of creating new method here. Keep it
here for later tasks.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5