Issue 98 - Use the libadblockplus update mechanism

Issue 98 - Use the libadblockplus update mechanism

[Felix Dahlke, 2014-09-25 16:01:12]

Here goes. Please keep an eye out for dead code I forgot to remove, and for
potential bugs of course. I haven't tested this exhaustively yet.

I was thinking of removing the JNI wrappers for JsEngine::SetEventCallback while
I'm at it - objections? Clients generally shouldn't use it anymore now that
there is FilterEngine::SetUpdateAvailableCallback.

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/jni/...
File jni/Android.mk (right):

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/jni/...
jni/Android.mk:27: LOCAL_SRC_FILES := JniJsEngine.cpp JniFilterEngine.cpp
JniJsValue.cpp
Changed a bit more than necessary here - I wanted to wrap at 80 columns while
I'm at it.

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
File src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java (right):

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java:37: if
(StringUtils.isNotEmpty(error))
Had to add this, so we don't always show the "no update available" message. In
fact, we will never show that message now. I'm planning to create a follow-up
issue for showing it after manual update checks.

[René Jeschke, 2014-09-26 10:14:58]

On 2014/09/25 16:01:12, Felix H. Dahlke wrote:
> Here goes. Please keep an eye out for dead code I forgot to remove, and for
> potential bugs of course. I haven't tested this exhaustively yet.
> 
> I was thinking of removing the JNI wrappers for JsEngine::SetEventCallback
while
> I'm at it - objections? Clients generally shouldn't use it anymore now that
> there is FilterEngine::SetUpdateAvailableCallback.
> 

We can remove these JNI bindings if we remove the functionality from
libadblockplus' public API. As these are API bindings, we are not to decide
whether we should include a method or not as long as it's part of the public
API.

>
http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/jni/...
> File jni/Android.mk (right):
> 
>
http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/jni/...
> jni/Android.mk:27: LOCAL_SRC_FILES := JniJsEngine.cpp JniFilterEngine.cpp
> JniJsValue.cpp
> Changed a bit more than necessary here - I wanted to wrap at 80 columns while
> I'm at it.
> 
>
http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
> File src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java (right):
> 
>
http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
> src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java:37: if
> (StringUtils.isNotEmpty(error))
> Had to add this, so we don't always show the "no update available" message. In
> fact, we will never show that message now. I'm planning to create a follow-up
> issue for showing it after manual update checks.

[René Jeschke, 2014-09-26 10:24:05]

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/res/...
File res/values/strings.xml (right):

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/res/...
res/values/strings.xml:93: <string name="menu_update">Check for updates</string>
Don't forget: We need localizations for this new string.

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
File src/org/adblockplus/android/Utils.java (right):

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
src/org/adblockplus/android/Utils.java:35: private static final int
UPDATE_NOTIFICATION_ID = R.string.app_name + 1;
I'm not happy with having the ID here. This is a utilities class and it doesn't
feel right to have this ID here.

We could move this to AdblockPlus or ProxyService ... would make more sense
there IMO.

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
src/org/adblockplus/android/Utils.java:72: Log.e(getTag(Utils.class), "Failed to
check for updates: " + error);
1. Please define this as a static variable as this call is costly [getTag(...)].
2. Feels wrong that 'Utils.class' reports update errors.

Maybe we can just remove the logging here? Or do we really gain anything from
logging the error?

[Felix Dahlke, 2014-09-26 11:20:24]

On 2014/09/26 10:14:58, René Jeschke wrote:
> We can remove these JNI bindings if we remove the functionality from
> libadblockplus' public API. As these are API bindings, we are not to decide
> whether we should include a method or not as long as it's part of the public
> API.

Alright. Wasn't sure whether the JNI bindings are intended to be comprehensive
or only cover the stuff we actually use, let's keep it then. (It does seem to be
broken, however, caused a segfault.)

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
File src/org/adblockplus/android/Utils.java (right):

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
src/org/adblockplus/android/Utils.java:35: private static final int
UPDATE_NOTIFICATION_ID = R.string.app_name + 1;
On 2014/09/26 10:24:05, René Jeschke wrote:
> I'm not happy with having the ID here. This is a utilities class and it
doesn't
> feel right to have this ID here.
> 
> We could move this to AdblockPlus or ProxyService ... would make more sense
> there IMO.

It's a local constant - why make it a global one if it's only used here? We
generally try to declare variables as close to their use as possible.

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
src/org/adblockplus/android/Utils.java:72: Log.e(getTag(Utils.class), "Failed to
check for updates: " + error);
On 2014/09/26 10:24:05, René Jeschke wrote:
> 1. Please define this as a static variable as this call is costly
[getTag(...)].
> 2. Feels wrong that 'Utils.class' reports update errors.
> 
> Maybe we can just remove the logging here? Or do we really gain anything from
> logging the error?

How else would we debug this in practice? The alternative would be to include
the message into the notification message, but I'm not sure we want to bother
the user with it. What do you think?

[René Jeschke, 2014-09-26 11:31:50]

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
File src/org/adblockplus/android/Utils.java (right):

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
src/org/adblockplus/android/Utils.java:35: private static final int
UPDATE_NOTIFICATION_ID = R.string.app_name + 1;
On 2014/09/26 11:20:24, Felix H. Dahlke wrote:
> On 2014/09/26 10:24:05, René Jeschke wrote:
> > I'm not happy with having the ID here. This is a utilities class and it
> doesn't
> > feel right to have this ID here.
> > 
> > We could move this to AdblockPlus or ProxyService ... would make more sense
> > there IMO.
> 
> It's a local constant - why make it a global one if it's only used here? We
> generally try to declare variables as close to their use as possible.

Well, then you have to move it into the method itself, as this value isn't even
used in the rest of the class.

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
src/org/adblockplus/android/Utils.java:72: Log.e(getTag(Utils.class), "Failed to
check for updates: " + error);
On 2014/09/26 11:20:24, Felix H. Dahlke wrote:
> On 2014/09/26 10:24:05, René Jeschke wrote:
> > 1. Please define this as a static variable as this call is costly
> [getTag(...)].
> > 2. Feels wrong that 'Utils.class' reports update errors.
> > 
> > Maybe we can just remove the logging here? Or do we really gain anything
from
> > logging the error?
> 
> How else would we debug this in practice? The alternative would be to include
> the message into the notification message, but I'm not sure we want to bother
> the user with it. What do you think?

Point is: the tag does not match the functionality. IMO a multi-purpose class
should not log stuff using it's class name. And to be more specific: such a
class should not log any errors.

We need to rework the logging nevertheless. We are spamming way to much crap
into the logfiles, too much noise.

So, why not just disable this message for now until we have some method for
better logging control? (As the whole updater also currently does nothing
anyways.)

[Felix Dahlke, 2014-09-26 11:39:05]

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
File src/org/adblockplus/android/Utils.java (right):

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
src/org/adblockplus/android/Utils.java:35: private static final int
UPDATE_NOTIFICATION_ID = R.string.app_name + 1;
On 2014/09/26 11:31:50, René Jeschke wrote:
> On 2014/09/26 11:20:24, Felix H. Dahlke wrote:
> > On 2014/09/26 10:24:05, René Jeschke wrote:
> > > I'm not happy with having the ID here. This is a utilities class and it
> > doesn't
> > > feel right to have this ID here.
> > > 
> > > We could move this to AdblockPlus or ProxyService ... would make more
sense
> > > there IMO.
> > 
> > It's a local constant - why make it a global one if it's only used here? We
> > generally try to declare variables as close to their use as possible.
> 
> Well, then you have to move it into the method itself, as this value isn't
even
> used in the rest of the class.

Yeah, I was pondering that, but a local final instead of a static final seems
rather unusual for constants in Java. No strong preference here, would you
prefer that?

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
src/org/adblockplus/android/Utils.java:72: Log.e(getTag(Utils.class), "Failed to
check for updates: " + error);
On 2014/09/26 11:31:50, René Jeschke wrote:
> On 2014/09/26 11:20:24, Felix H. Dahlke wrote:
> > On 2014/09/26 10:24:05, René Jeschke wrote:
> > > 1. Please define this as a static variable as this call is costly
> > [getTag(...)].
> > > 2. Feels wrong that 'Utils.class' reports update errors.
> > > 
> > > Maybe we can just remove the logging here? Or do we really gain anything
> from
> > > logging the error?
> > 
> > How else would we debug this in practice? The alternative would be to
include
> > the message into the notification message, but I'm not sure we want to
bother
> > the user with it. What do you think?
> 
> Point is: the tag does not match the functionality. IMO a multi-purpose class
> should not log stuff using it's class name. And to be more specific: such a
> class should not log any errors.
> 
> We need to rework the logging nevertheless. We are spamming way to much crap
> into the logfiles, too much noise.
> 
> So, why not just disable this message for now until we have some method for
> better logging control? (As the whole updater also currently does nothing
> anyways.)

I would really like to be able to debug network problems we have in production
_somehow_, rather than swallowing the message entirely.

But you're right about this probably not really belonging into Utils. How about
I move the Log.e to AndroidUpdateCheckDoneCallback?

[René Jeschke, 2014-09-26 11:59:17]

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
File src/org/adblockplus/android/Utils.java (right):

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
src/org/adblockplus/android/Utils.java:35: private static final int
UPDATE_NOTIFICATION_ID = R.string.app_name + 1;
On 2014/09/26 11:39:05, Felix H. Dahlke wrote:
> On 2014/09/26 11:31:50, René Jeschke wrote:
> > On 2014/09/26 11:20:24, Felix H. Dahlke wrote:
> > > On 2014/09/26 10:24:05, René Jeschke wrote:
> > > > I'm not happy with having the ID here. This is a utilities class and it
> > > doesn't
> > > > feel right to have this ID here.
> > > > 
> > > > We could move this to AdblockPlus or ProxyService ... would make more
> sense
> > > > there IMO.
> > > 
> > > It's a local constant - why make it a global one if it's only used here?
We
> > > generally try to declare variables as close to their use as possible.
> > 
> > Well, then you have to move it into the method itself, as this value isn't
> even
> > used in the rest of the class.
> 
> Yeah, I was pondering that, but a local final instead of a static final seems
> rather unusual for constants in Java. No strong preference here, would you
> prefer that?

Would make more sense than declaring it class-wide. Also, as this is only used
in one location we wouldn't even need a variable here, but a final local
variable should be ok.

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
src/org/adblockplus/android/Utils.java:72: Log.e(getTag(Utils.class), "Failed to
check for updates: " + error);
On 2014/09/26 11:39:05, Felix H. Dahlke wrote:
> On 2014/09/26 11:31:50, René Jeschke wrote:
> > On 2014/09/26 11:20:24, Felix H. Dahlke wrote:
> > > On 2014/09/26 10:24:05, René Jeschke wrote:
> > > > 1. Please define this as a static variable as this call is costly
> > > [getTag(...)].
> > > > 2. Feels wrong that 'Utils.class' reports update errors.
> > > > 
> > > > Maybe we can just remove the logging here? Or do we really gain anything
> > from
> > > > logging the error?
> > > 
> > > How else would we debug this in practice? The alternative would be to
> include
> > > the message into the notification message, but I'm not sure we want to
> bother
> > > the user with it. What do you think?
> > 
> > Point is: the tag does not match the functionality. IMO a multi-purpose
class
> > should not log stuff using it's class name. And to be more specific: such a
> > class should not log any errors.
> > 
> > We need to rework the logging nevertheless. We are spamming way to much crap
> > into the logfiles, too much noise.
> > 
> > So, why not just disable this message for now until we have some method for
> > better logging control? (As the whole updater also currently does nothing
> > anyways.)
> 
> I would really like to be able to debug network problems we have in production
> _somehow_, rather than swallowing the message entirely.
> 
> But you're right about this probably not really belonging into Utils. How
about
> I move the Log.e to AndroidUpdateCheckDoneCallback?

Yes, that would make more sense IMO. And I will need to open an issue for
logging-refactoring.

[Felix Dahlke, 2014-09-26 13:05:13]

New patch set up.

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
File src/org/adblockplus/android/Utils.java (right):

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
src/org/adblockplus/android/Utils.java:35: private static final int
UPDATE_NOTIFICATION_ID = R.string.app_name + 1;
On 2014/09/26 11:59:17, René Jeschke wrote:
> On 2014/09/26 11:39:05, Felix H. Dahlke wrote:
> > On 2014/09/26 11:31:50, René Jeschke wrote:
> > > On 2014/09/26 11:20:24, Felix H. Dahlke wrote:
> > > > On 2014/09/26 10:24:05, René Jeschke wrote:
> > > > > I'm not happy with having the ID here. This is a utilities class and
it
> > > > doesn't
> > > > > feel right to have this ID here.
> > > > > 
> > > > > We could move this to AdblockPlus or ProxyService ... would make more
> > sense
> > > > > there IMO.
> > > > 
> > > > It's a local constant - why make it a global one if it's only used here?
> We
> > > > generally try to declare variables as close to their use as possible.
> > > 
> > > Well, then you have to move it into the method itself, as this value isn't
> > even
> > > used in the rest of the class.
> > 
> > Yeah, I was pondering that, but a local final instead of a static final
seems
> > rather unusual for constants in Java. No strong preference here, would you
> > prefer that?
> 
> Would make more sense than declaring it class-wide. Also, as this is only used
> in one location we wouldn't even need a variable here, but a final local
> variable should be ok.

Alright, made it a local constant. I disagree about inlining it, makes the code
harder to understand IMO.

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
src/org/adblockplus/android/Utils.java:72: Log.e(getTag(Utils.class), "Failed to
check for updates: " + error);
On 2014/09/26 11:59:17, René Jeschke wrote:
> On 2014/09/26 11:39:05, Felix H. Dahlke wrote:
> > On 2014/09/26 11:31:50, René Jeschke wrote:
> > > On 2014/09/26 11:20:24, Felix H. Dahlke wrote:
> > > > On 2014/09/26 10:24:05, René Jeschke wrote:
> > > > > 1. Please define this as a static variable as this call is costly
> > > > [getTag(...)].
> > > > > 2. Feels wrong that 'Utils.class' reports update errors.
> > > > > 
> > > > > Maybe we can just remove the logging here? Or do we really gain
anything
> > > from
> > > > > logging the error?
> > > > 
> > > > How else would we debug this in practice? The alternative would be to
> > include
> > > > the message into the notification message, but I'm not sure we want to
> > bother
> > > > the user with it. What do you think?
> > > 
> > > Point is: the tag does not match the functionality. IMO a multi-purpose
> class
> > > should not log stuff using it's class name. And to be more specific: such
a
> > > class should not log any errors.
> > > 
> > > We need to rework the logging nevertheless. We are spamming way to much
crap
> > > into the logfiles, too much noise.
> > > 
> > > So, why not just disable this message for now until we have some method
for
> > > better logging control? (As the whole updater also currently does nothing
> > > anyways.)
> > 
> > I would really like to be able to debug network problems we have in
production
> > _somehow_, rather than swallowing the message entirely.
> > 
> > But you're right about this probably not really belonging into Utils. How
> about
> > I move the Log.e to AndroidUpdateCheckDoneCallback?
> 
> Yes, that would make more sense IMO. And I will need to open an issue for
> logging-refactoring.

Done.

[René Jeschke, 2014-09-26 13:10:51]

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
File src/org/adblockplus/android/Utils.java (right):

http://codereview.adblockplus.org/5153282527854592/diff/5668600916475904/src/...
src/org/adblockplus/android/Utils.java:35: private static final int
UPDATE_NOTIFICATION_ID = R.string.app_name + 1;
On 2014/09/26 13:05:13, Felix H. Dahlke wrote:
> On 2014/09/26 11:59:17, René Jeschke wrote:
> > On 2014/09/26 11:39:05, Felix H. Dahlke wrote:
> > > On 2014/09/26 11:31:50, René Jeschke wrote:
> > > > On 2014/09/26 11:20:24, Felix H. Dahlke wrote:
> > > > > On 2014/09/26 10:24:05, René Jeschke wrote:
> > > > > > I'm not happy with having the ID here. This is a utilities class and
> it
> > > > > doesn't
> > > > > > feel right to have this ID here.
> > > > > > 
> > > > > > We could move this to AdblockPlus or ProxyService ... would make
more
> > > sense
> > > > > > there IMO.
> > > > > 
> > > > > It's a local constant - why make it a global one if it's only used
here?
> > We
> > > > > generally try to declare variables as close to their use as possible.
> > > > 
> > > > Well, then you have to move it into the method itself, as this value
isn't
> > > even
> > > > used in the rest of the class.
> > > 
> > > Yeah, I was pondering that, but a local final instead of a static final
> seems
> > > rather unusual for constants in Java. No strong preference here, would you
> > > prefer that?
> > 
> > Would make more sense than declaring it class-wide. Also, as this is only
used
> > in one location we wouldn't even need a variable here, but a final local
> > variable should be ok.
> 
> Alright, made it a local constant. I disagree about inlining it, makes the
code
> harder to understand IMO.

Righty right, thanks.

http://codereview.adblockplus.org/5153282527854592/diff/5693417237512192/src/...
File src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java (right):

http://codereview.adblockplus.org/5153282527854592/diff/5693417237512192/src/...
src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java:40:
Log.e(Utils.getTag(AndroidUpdateCheckDoneCallback.class), "Update check failed:
" + error);
Could you please extract the 'tag' into a private/static/class variable? It's
commonly used in all classes throughout the project, like:

  private static final String TAG =
Utils.getTag(AndroidUpdateCheckDoneCallback.class);

[Felix Dahlke, 2014-09-26 13:15:31]

http://codereview.adblockplus.org/5153282527854592/diff/5693417237512192/src/...
File src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java (right):

http://codereview.adblockplus.org/5153282527854592/diff/5693417237512192/src/...
src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java:40:
Log.e(Utils.getTag(AndroidUpdateCheckDoneCallback.class), "Update check failed:
" + error);
On 2014/09/26 13:10:52, René Jeschke wrote:
> Could you please extract the 'tag' into a private/static/class variable? It's
> commonly used in all classes throughout the project, like:
> 
>   private static final String TAG =
> Utils.getTag(AndroidUpdateCheckDoneCallback.class);

Done.

[René Jeschke, 2014-09-26 13:19:46]

LGTM


We need two follow up issues:

1. Update check notification for manual update
2. Localization for <string name="menu_update">Check for updates</string>

[Felix Dahlke, 2014-09-26 13:31:22]

On 2014/09/26 13:19:46, René Jeschke wrote:
> LGTM
> 
> 
> We need two follow up issues:
> 
> 1. Update check notification for manual update
> 2. Localization for <string name="menu_update">Check for updates</string>

We don't create issues for new translations, we just do a translation round
before the release.

But there's another follow-up we need: Make the no-update-available message show
up for manual update checks.

[René Jeschke, 2014-09-26 13:32:33]

On 2014/09/26 13:31:22, Felix H. Dahlke wrote:
> On 2014/09/26 13:19:46, René Jeschke wrote:
> > LGTM
> > 
> > 
> > We need two follow up issues:
> > 
> > 1. Update check notification for manual update
> > 2. Localization for <string name="menu_update">Check for updates</string>
> 
> We don't create issues for new translations, we just do a translation round
> before the release.
> 

K.

> But there's another follow-up we need: Make the no-update-available message
show
> up for manual update checks.

Yeah, that's what I meant with '1.'.

[Felix Dahlke, 2014-09-26 13:38:19]

On 2014/09/26 13:32:33, René Jeschke wrote:
> On 2014/09/26 13:31:22, Felix H. Dahlke wrote:
> > On 2014/09/26 13:19:46, René Jeschke wrote:
> > > LGTM
> > > 
> > > 
> > > We need two follow up issues:
> > > 
> > > 1. Update check notification for manual update
> > > 2. Localization for <string name="menu_update">Check for updates</string>
> > 
> > We don't create issues for new translations, we just do a translation round
> > before the release.
> > 
> 
> K.
> 
> > But there's another follow-up we need: Make the no-update-available message
> show
> > up for manual update checks.
> 
> Yeah, that's what I meant with '1.'.

Oops, somehow overread that. Well, here goes:
https://issues.adblockplus.org/ticket/1450

[Felix Dahlke, 2014-09-26 15:09:33]

Uploaded another patch set - had to make sure we don't append the revision
number for release builds.

[René Jeschke, 2014-09-26 15:19:05]

LGTM