Issue 1108 - Support notifications

Issue 1108 - Support notifications

[René Jeschke, 2015-02-17 13:59:26]

There are two changes included, that might be considered unrelated but are
crucial in my opinion:

1. I've moved all scattered NOTIFICATION_ID declarations into one place
(AdblockPlus.java). As we now have 3 different types of notifications we needed
to get some order into the previous chaos

2. I refactored the NewJni... methods in Utils.h that were responsible for
creating Filter/Subscription Java objects. we now have a 3rd creation method
(Notification) which made it necessary to refactor these methods to avoid code
duplication.

Otherwise: this is a feature-freeze-last-minute review. I'm still investigating
if the crash on device startup with ABP enabled still exists.

[Felix Dahlke, 2015-02-18 12:49:56]

Looks good all in all, doesn't make sense to me to separate the refactorings
necessary for this.

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/jni/...
File jni/JniFilterEngine.cpp (right):

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/jni/...
jni/JniFilterEngine.cpp:121: static jobject JNICALL
JniGetNotificationToShow(JNIEnv* env, jclass clazz, jlong ptr, jstring jUrl)
Should be called JniGetNextNotificationToShow, in line with
FilterEngine::GetNextNotificationToShow.

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/jni/...
jni/JniFilterEngine.cpp:125: if (jUrl)
Nit: JniJavaToStdString already does convert a null string to an empty one, so
we could shorten the code here a bit.

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/jni/...
File jni/JniNotification.cpp (right):

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/jni/...
jni/JniNotification.cpp:22: // TODO (rje) apply local_ref
Ah, TODO comment! :) Sounds like something that needs an issue - I'm not
entirely sure what is to be done here frankly.

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
File src/org/adblockplus/android/ProxyService.java (right):

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/ProxyService.java:625: private static class
NotificationWatcher implements Runnable
This seems to be way too much code for what we want to do here - can't we just
use Handler.postDelayed? I'm pretty sure that should work even if the app is
killed.

If not, we could use AlarmManager.

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
File src/org/adblockplus/android/updater/UpdaterService.java (right):

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/updater/UpdaterService.java:187:
notificationManager.notify(AdblockPlus.UPDATE_NOTIFICATION_ID, notification);
Any idea why this used a different ID than notificationId before? Doesn't seem
to make sense at first glance.

[René Jeschke, 2015-02-18 13:13:01]

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/jni/...
File jni/JniFilterEngine.cpp (right):

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/jni/...
jni/JniFilterEngine.cpp:121: static jobject JNICALL
JniGetNotificationToShow(JNIEnv* env, jclass clazz, jlong ptr, jstring jUrl)
On 2015/02/18 12:49:56, Felix H. Dahlke wrote:
> Should be called JniGetNextNotificationToShow, in line with
> FilterEngine::GetNextNotificationToShow.

Done.

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/jni/...
jni/JniFilterEngine.cpp:125: if (jUrl)
On 2015/02/18 12:49:56, Felix H. Dahlke wrote:
> Nit: JniJavaToStdString already does convert a null string to an empty one, so
> we could shorten the code here a bit.

Hehe, ehem, sure ... I've written the function ... *sigh*

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/jni/...
File jni/JniNotification.cpp (right):

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/jni/...
jni/JniNotification.cpp:22: // TODO (rje) apply local_ref
On 2015/02/18 12:49:56, Felix H. Dahlke wrote:
> Ah, TODO comment! :) Sounds like something that needs an issue - I'm not
> entirely sure what is to be done here frankly.

Nah, was written during the local_ref review to remind me of wrapping local refs
... which I forgot.

Fixed now.

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
File src/org/adblockplus/android/ProxyService.java (right):

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/ProxyService.java:625: private static class
NotificationWatcher implements Runnable
On 2015/02/18 12:49:56, Felix H. Dahlke wrote:
> This seems to be way too much code for what we want to do here - can't we just
> use Handler.postDelayed? I'm pretty sure that should work even if the app is
> killed.
> 
> If not, we could use AlarmManager.

AlarmManager: Why use a system-wide, scheduling service for a simple delay?

Handler: This uses message passing to ensure things get executed on the EDT. We
used Handler for the no-traffic checks ... which never really worked (and I do
not want to handle this on the EDT).

I would really like to keep the Thread. And the LOC saving by using a Handler
isn't that high.

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
File src/org/adblockplus/android/updater/UpdaterService.java (right):

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/updater/UpdaterService.java:187:
notificationManager.notify(AdblockPlus.UPDATE_NOTIFICATION_ID, notification);
On 2015/02/18 12:49:56, Felix H. Dahlke wrote:
> Any idea why this used a different ID than notificationId before? Doesn't seem
> to make sense at first glance.

Yeah, was thinking about this, also. I did not find a good reason, why this used
a different it. It also doesn't make sense, as we cancel 'notificationId'
anyway, so I thought it's save to reduce this to UPDATE_NOTIFICATION_ID.

[Felix Dahlke, 2015-02-18 14:10:07]

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
File src/org/adblockplus/android/ProxyService.java (right):

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/ProxyService.java:625: private static class
NotificationWatcher implements Runnable
On 2015/02/18 13:13:02, René Jeschke wrote:
> On 2015/02/18 12:49:56, Felix H. Dahlke wrote:
> > This seems to be way too much code for what we want to do here - can't we
just
> > use Handler.postDelayed? I'm pretty sure that should work even if the app is
> > killed.
> > 
> > If not, we could use AlarmManager.
> 
> AlarmManager: Why use a system-wide, scheduling service for a simple delay?
> 
> Handler: This uses message passing to ensure things get executed on the EDT.
We
> used Handler for the no-traffic checks ... which never really worked (and I do
> not want to handle this on the EDT).
> 
> I would really like to keep the Thread. And the LOC saving by using a Handler
> isn't that high.

Well, if this doesn't work reliably on the EDT, fair enough. Surprises me
frankly.

That leaves AlarmManager. That's indeed not much less code, and it _might_ get
fiddly to have an alarm that just runs code and doesn't bring any activity to
front.

Then I'll review the polling thread below.

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/ProxyService.java:631: private final int
firstPollInterval;
How about initialDelay? It's not really an interval if it's just once :P

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/ProxyService.java:632: private final int
nextPollIntervals;
Assuming firstPollInterval is renamed, this could just be pollingInterval.

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/ProxyService.java:685: while (notification != null)
We should actually just show a single notification here, not all in a row.
GetNextNotificationToShow is going to return only one notification - the one
we're supposed to show. If there's a critical one but also some information
ones, we'll never see the latter really.

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/ProxyService.java:688:
this.proxyService.getApplicationContext())
Shouldn't we pass the app's context here rather than the service's?

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/ProxyService.java:698: notification.markAsShown();
We actually only need to call this for question notifications - which aren't
supported yet. Note that it's currently not technically possible to actually GET
a question notification from the server, they're only created internally on ABP
for Firefox/Chrome/Safari/Opera.

[René Jeschke, 2015-02-18 14:45:20]

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
File src/org/adblockplus/android/ProxyService.java (right):

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/ProxyService.java:631: private final int
firstPollInterval;
On 2015/02/18 14:10:07, Felix H. Dahlke wrote:
> How about initialDelay? It's not really an interval if it's just once :P

Done.

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/ProxyService.java:632: private final int
nextPollIntervals;
On 2015/02/18 14:10:07, Felix H. Dahlke wrote:
> Assuming firstPollInterval is renamed, this could just be pollingInterval.

Done.

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/ProxyService.java:685: while (notification != null)
On 2015/02/18 14:10:07, Felix H. Dahlke wrote:
> We should actually just show a single notification here, not all in a row.
> GetNextNotificationToShow is going to return only one notification - the one
> we're supposed to show. If there's a critical one but also some information
> ones, we'll never see the latter really.

It is guaranteed that there aren't any two notifications waiting? Why is the
method then called 'get_NEXT_NotificationToShow'?

But if you can guarantee that there will never be two notifications pending,
then I'll remove this. Can you? :-)

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/ProxyService.java:688:
this.proxyService.getApplicationContext())
On 2015/02/18 14:10:07, Felix H. Dahlke wrote:
> Shouldn't we pass the app's context here rather than the service's?

I still am working on the service separation. The 'application' will then no
longer be available, and the Service already has a Context, so why use a foreign
context when we have a native one?

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/ProxyService.java:698: notification.markAsShown();
On 2015/02/18 14:10:07, Felix H. Dahlke wrote:
> We actually only need to call this for question notifications - which aren't
> supported yet. Note that it's currently not technically possible to actually
GET
> a question notification from the server, they're only created internally on
ABP
> for Firefox/Chrome/Safari/Opera.

Ah, Ok. I thought this has something to do with 'feedback'. Will remove this
then.

[Felix Dahlke, 2015-02-18 14:55:04]

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
File src/org/adblockplus/android/ProxyService.java (right):

http://codereview.adblockplus.org/5327480814567424/diff/5724160613416960/src/...
src/org/adblockplus/android/ProxyService.java:685: while (notification != null)
On 2015/02/18 14:45:20, René Jeschke wrote:
> On 2015/02/18 14:10:07, Felix H. Dahlke wrote:
> > We should actually just show a single notification here, not all in a row.
> > GetNextNotificationToShow is going to return only one notification - the one
> > we're supposed to show. If there's a critical one but also some information
> > ones, we'll never see the latter really.
> 
> It is guaranteed that there aren't any two notifications waiting? Why is the
> method then called 'get_NEXT_NotificationToShow'?
> 
> But if you can guarantee that there will never be two notifications pending,
> then I'll remove this. Can you? :-)

There can be an arbitrary number of notifications waiting, but we always only
deliberately show one. Some notifications (e.g. emergency) always have
precedence and will keep showing up forever (even when internally marked as
shown). If there's just information notifications, we'll show one on each
startup, until there aren't any anymore.

Once we do this periodically rather then on startup, it's gonna get a bit more
complicated. We don't want to have emergency notifications show up at fixed
intervals e.g. We'll also have notifications that will be triggered by certain
filters matching (e.g. by visiting a site).

http://codereview.adblockplus.org/5327480814567424/diff/5718998062727168/src/...
File src/org/adblockplus/android/ProxyService.java (right):

http://codereview.adblockplus.org/5327480814567424/diff/5718998062727168/src/...
src/org/adblockplus/android/ProxyService.java:628: public static final int
DEFAULT_POLL_INTERVALS = 0; /* seconds */
Should be POLL_INTERVAL now, huh?

http://codereview.adblockplus.org/5327480814567424/diff/5718998062727168/src/...
src/org/adblockplus/android/ProxyService.java:640: this.pollDelay = Math.max(0,
firstPollInterval);
Might as well rename the parameters while at it :P

http://codereview.adblockplus.org/5327480814567424/diff/5718998062727168/src/...
src/org/adblockplus/android/ProxyService.java:685: while (notification != null)
Maybe, just while we're at it, we should ignore question notifications here. So
we can keep the while loop. (Which to me really looks like it could be a for
loop :P)

[René Jeschke, 2015-02-18 15:01:25]

http://codereview.adblockplus.org/5327480814567424/diff/5718998062727168/src/...
File src/org/adblockplus/android/ProxyService.java (right):

http://codereview.adblockplus.org/5327480814567424/diff/5718998062727168/src/...
src/org/adblockplus/android/ProxyService.java:628: public static final int
DEFAULT_POLL_INTERVALS = 0; /* seconds */
On 2015/02/18 14:55:04, Felix H. Dahlke wrote:
> Should be POLL_INTERVAL now, huh?

Done.

http://codereview.adblockplus.org/5327480814567424/diff/5718998062727168/src/...
src/org/adblockplus/android/ProxyService.java:640: this.pollDelay = Math.max(0,
firstPollInterval);
On 2015/02/18 14:55:04, Felix H. Dahlke wrote:
> Might as well rename the parameters while at it :P

Done.

http://codereview.adblockplus.org/5327480814567424/diff/5718998062727168/src/...
src/org/adblockplus/android/ProxyService.java:685: while (notification != null)
On 2015/02/18 14:55:04, Felix H. Dahlke wrote:
> Maybe, just while we're at it, we should ignore question notifications here.
So
> we can keep the while loop. (Which to me really looks like it could be a for
> loop :P)

Stayed a 'while', ignoring QUESTION/INVALID now.

[Felix Dahlke, 2015-02-18 15:36:55]

http://codereview.adblockplus.org/5327480814567424/diff/5110435556622336/src/...
File src/org/adblockplus/android/ProxyService.java (right):

http://codereview.adblockplus.org/5327480814567424/diff/5110435556622336/src/...
src/org/adblockplus/android/ProxyService.java:699: idx++;
We don't really need the index. More importantly, we need to exit the loop here
once we show any notification. We just need to keep looping until we have a
non-question notification.

[René Jeschke, 2015-02-18 15:42:58]

http://codereview.adblockplus.org/5327480814567424/diff/5110435556622336/src/...
File src/org/adblockplus/android/ProxyService.java (right):

http://codereview.adblockplus.org/5327480814567424/diff/5110435556622336/src/...
src/org/adblockplus/android/ProxyService.java:699: idx++;
On 2015/02/18 15:36:55, Felix H. Dahlke wrote:
> We don't really need the index. More importantly, we need to exit the loop
here
> once we show any notification. We just need to keep looping until we have a
> non-question notification.

Done.

[Felix Dahlke, 2015-02-18 16:06:45]

LGTM