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

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

Index: chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java
new file mode 100644
index 0000000000000000000000000000000000000000..10bb8872fa1845f82d6d0d5e2db4dfd1aec05e08
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java
@@ -0,0 +1,91 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.notifications;
+
+import android.annotation.TargetApi;
+import android.app.job.JobParameters;
+import android.app.job.JobService;
+import android.content.Intent;
+import android.os.Build;
+import android.os.Bundle;
+import android.os.PersistableBundle;
+
+import org.chromium.base.ThreadUtils;
+
+/**
+ * Processes jobs scheduled when user actions are issued on web notifications.
+ * We use this instead of starting the NotificationService on N+.
+ */
+@TargetApi(Build.VERSION_CODES.N)
+public class NotificationJobService extends JobService {
+    // We don't need to distinguish between jobs sent to this service, so we can reuse one job id.
+    // But it ought to be distinct from job ids used with other JobService classes in our code.
+    static final int JOB_ID = 21;
+
+    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.
+     *
+     * We get a wakelock for our process for the duration of this method.
+     *
+     * @return True if there is more work to be done to handle the job, to signal we would like our
+     * wakelock extended until we call {@link #jobFinished}. False if we have finished handling the
+     * job.
+     */
+    @Override
+    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;

[nyquist, 2017/01/25 17:14:56]

Nit: If this ever happens, should we log it? Or is this expected to happen every
so often and isn't really interesting?

[awdf, 2017/01/25 17:57:23]

Afaict, this shouldn't happen as our broadcast receiver which starts jobs when
it receives intents is not exported, and the only place we send it intents is in
NotificationPlatformBridge where these fields are always set. So yes we should
probably either remove this check or log this failure case, here and in
NotificationService.onHandleIntent. Perhaps this check should even be pushed
earlier to NotificationService.BroadcastReceiver.onReceive before the paths
diverge. I'll file a bug.

+        }
+
+        Intent intent =
+                new Intent(extras.getString(NotificationConstants.EXTRA_NOTIFICATION_ACTION));
+        intent.putExtras(new Bundle(extras));
+
+        ThreadUtils.assertOnUiThread();
+        NotificationService.dispatchIntentOnUIThread(this, intent);
+
+        // TODO(crbug.com/685197): Return true here and call jobFinished to release the wake
+        // lock only after the event has been completely handled by the service worker.
+        return false;
+    }
+
+    @Override
+    public boolean onStopJob(JobParameters params) {
+        // As it stands, all our job processing is done synchronously in onStartJob so there is
+        // nothing to do here. Even once we include further async processing in our jobs
+        // (crbug.com/685197) it may by infeasible to cancel this halfway through.

[awdf, 2017/01/25 16:16:01]

perhaps this is too pessimistic - peter@ ?

[nyquist, 2017/01/25 17:14:56]

Looks OK to me.

[Peter Beverloo, 2017/01/25 18:12:19]

+1

It is important to note here that the synchronous job processing in onStartJob
solely cares about the Android side of things. Chrome will have plenty of work
to do after that method finishes.

I have no idea what we'd do when onStopJob() is called. Theoretically we could
kill the SW/renderer and abort things immediately, but we'll have to carefully
think about the impact of that.

[awdf, 2017/01/25 18:23:49]

yeah that's what I was thinking.

+        // TODO(crbug.com/685197): Check what we can safely do here and update comment.
+        return false;
+    }
+}