Android web notifications: Schedule job instead of starting service (N+)

chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java

Index: chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java
index 176e45f0c1971127f3ff9a03fc8bf119f57f6ffd..4d090f9d496a7769f471ec8692315356c2701ca6 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java
@@ -4,11 +4,21 @@
 
 package org.chromium.chrome.browser.notifications;
 
-import android.app.IntentService;
+import android.annotation.TargetApi;
+import android.app.job.JobInfo;
+import android.app.job.JobParameters;
+import android.app.job.JobScheduler;
+import android.app.job.JobService;
 import android.content.BroadcastReceiver;
+import android.content.ComponentName;
 import android.content.Context;
 import android.content.Intent;
+import android.os.Build;
+import android.os.Bundle;
+import android.os.PersistableBundle;
 import android.os.StrictMode;
+import android.support.annotation.NonNull;
+import android.support.annotation.RequiresApi;
 import android.util.Log;
 
 import org.chromium.base.ThreadUtils;
@@ -21,7 +31,8 @@ import org.chromium.chrome.browser.webapps.WebappRegistry;
  * The Notification service receives intents fired as responses to user actions issued on Android
  * notifications displayed in the notification tray.
  */
-public class NotificationService extends IntentService {
+@TargetApi(Build.VERSION_CODES.LOLLIPOP)
+public class NotificationService extends JobService {
     private static final String TAG = NotificationService.class.getSimpleName();
 
     /**
@@ -33,38 +44,85 @@ public class NotificationService extends IntentService {
         @Override
         public void onReceive(Context context, Intent intent) {
             Log.i(TAG, "Received a notification intent in the NotificationService's receiver.");
+            if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.LOLLIPOP_MR1) {

[Peter Beverloo, 2017/01/09 14:28:05]

What's your reasoning for using the Job Scheduler for LMR1+ as opposed to N+?

[awdf, 2017/01/11 14:56:18]

I guess because while GcmTaskService is unusable for our purposes due to its min
30s delay, so far I haven't seen evidence of JobScheduler causing us lag for
notification clicks. (Using an adb command to watch posted tasks, it appears to
execute immediately when I test). And Android already encourages use of
JobScheduler for apps targeting L+ rather than starting services.

But I see your point - the risk of lag exists, so there's no point taking it
without good reason. That good reason could be to gather metrics on one platform
so we know what the impact will be once we are forced to use this on future
platforms. Could we run an experiment on N where only some notification clicks
use JobScheduler, and time both code paths?

For now I will limit this to >=N.

[awdf, 2017/01/12 19:07:33]

Done.

+                PersistableBundle extras = getJobExtrasFromIntent(intent);
+                JobInfo job = new JobInfo
+                                      .Builder(0 /* jobId */,

[Peter Beverloo, 2017/01/09 14:28:05]

I think we'll need a magic value for our job. The documentation says:

[awdf, 2017/01/12 19:07:32]

Done. The only other place we create job scheduler jobs is in
CrashReceiverService, and they've nabbed 42 of course .. wondering if it makes
sense to put the job ids in one central place to decrease risk of future magic
number clash, not sure where though?

+                                              new ComponentName(context, NotificationService.class))
+                                      .setExtras(extras)
+                                      .setOverrideDeadline(1)

[Peter Beverloo, 2017/01/09 14:28:05]

I think we can set this to zero?

[awdf, 2017/01/12 19:07:33]

Ah yes, I think I did this while trying to use the gcm OneOffTaskBuilder since
it requires windowEnd > windowStart, but JobInfo.Builder doesn't seem to have
this restriction.

Done.

+                                      .build();
+                JobScheduler scheduler =
+                        (JobScheduler) context.getSystemService(Context.JOB_SCHEDULER_SERVICE);
 
-            // TODO(peter): Do we need to acquire a wake lock here?
+                scheduler.schedule(job);
+            } else {
+                // TODO(peter): Do we need to acquire a wake lock here?
 
-            intent.setClass(context, NotificationService.class);
-            context.startService(intent);
+                intent.setClass(context, NotificationService.class);
+                context.startService(intent);
+            }
         }
-    }
 
-    public NotificationService() {
-        super(TAG);
+        @TargetApi(Build.VERSION_CODES.LOLLIPOP_MR1)
+        @NonNull

[Peter Beverloo, 2017/01/09 14:28:05]

micro nit: up to you, but I'd prefer to not have @NonNull as well. Right now the
contract in our code is "non-null unless marked @Nullable".

[awdf, 2017/01/12 19:07:32]

Done.

+        private static PersistableBundle getJobExtrasFromIntent(Intent intent) {
+            PersistableBundle bundle = new PersistableBundle();
+            bundle.putString(NotificationConstants.EXTRA_NOTIFICATION_ID,
+                    intent.getStringExtra(NotificationConstants.EXTRA_NOTIFICATION_ID));
+            bundle.putString(NotificationConstants.EXTRA_NOTIFICATION_INFO_ORIGIN,
+                    intent.getStringExtra(NotificationConstants.EXTRA_NOTIFICATION_INFO_ORIGIN));
+            bundle.putString(NotificationConstants.EXTRA_NOTIFICATION_INFO_PROFILE_ID,
+                    intent.getStringExtra(
+                            NotificationConstants.EXTRA_NOTIFICATION_INFO_PROFILE_ID));
+            bundle.putBoolean(NotificationConstants.EXTRA_NOTIFICATION_INFO_PROFILE_INCOGNITO,
+                    intent.getBooleanExtra(
+                            NotificationConstants.EXTRA_NOTIFICATION_INFO_PROFILE_INCOGNITO,
+                            false));
+            bundle.putString(NotificationConstants.EXTRA_NOTIFICATION_INFO_TAG,
+                    intent.getStringExtra(NotificationConstants.EXTRA_NOTIFICATION_INFO_TAG));
+            bundle.putInt(NotificationConstants.EXTRA_NOTIFICATION_INFO_ACTION_INDEX,
+                    intent.getIntExtra(
+                            NotificationConstants.EXTRA_NOTIFICATION_INFO_ACTION_INDEX, -1));
+            bundle.putString(NotificationConstants.EXTRA_NOTIFICATION_INFO_WEBAPK_PACKAGE,
+                    intent.getStringExtra(
+                            NotificationConstants.EXTRA_NOTIFICATION_INFO_WEBAPK_PACKAGE));
+            bundle.putString(NotificationConstants.EXTRA_NOTIFICATION_ACTION, intent.getAction());
+            // Only primitives can be set on a persistable bundle, so extract the raw reply.
+            bundle.putString(NotificationConstants.EXTRA_NOTIFICATION_REPLY,
+                    NotificationPlatformBridge.getNotificationReply(intent));
+            return bundle;
+        }
     }
 
     /**
      * Called when a Notification has been interacted with by the user. If we can verify that
      * the Intent has a notification Id, start Chrome (if needed) on the UI thread.
-     *
-     * @param intent The intent containing the specific information.
      */
+    @RequiresApi(api = Build.VERSION_CODES.LOLLIPOP)
     @Override
-    public void onHandleIntent(final Intent intent) {
-        if (!intent.hasExtra(NotificationConstants.EXTRA_NOTIFICATION_ID)
-                || !intent.hasExtra(NotificationConstants.EXTRA_NOTIFICATION_INFO_ORIGIN)
-                || !intent.hasExtra(NotificationConstants.EXTRA_NOTIFICATION_INFO_TAG)) {
-            return;
+    public boolean onStartJob(final JobParameters params) {
+        PersistableBundle extras = params.getExtras();
+        if (!extras.containsKey(NotificationConstants.EXTRA_NOTIFICATION_ID)
+                || !extras.containsKey(NotificationConstants.EXTRA_NOTIFICATION_INFO_ORIGIN)
+                || !extras.containsKey(NotificationConstants.EXTRA_NOTIFICATION_INFO_TAG)) {
+            return false;
         }
-
+        final Intent intent =
+                new Intent(extras.getString(NotificationConstants.EXTRA_NOTIFICATION_ACTION));
+        intent.putExtras(new Bundle(extras));
         ThreadUtils.runOnUiThread(new Runnable() {
             @Override
             public void run() {
                 dispatchIntentOnUIThread(intent);

[awdf, 2017/01/10 18:10:24]

Oops - I should really be calling onJobFinished here -
https://developer.android.com/reference/android/app/job/JobService.html#jobFi...,
boolean) - will fix 

[awdf, 2017/01/12 19:07:32]

Done.

             }
         });
+        return true;
+    }
+
+    @Override
+    public boolean onStopJob(JobParameters params) {
+        return false;
     }
 
     /**