[Android] Native initialization task for BackgroundTaskScheduler

chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java

Index: chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java b/chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java
new file mode 100644
index 0000000000000000000000000000000000000000..827952f006569f183e9b54e0799ae7e5008e1c08
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java
@@ -0,0 +1,181 @@
+// 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.background_task_scheduler;
+
+import android.content.Context;
+import android.support.annotation.IntDef;
+
+import org.chromium.base.Log;
+import org.chromium.base.ThreadUtils;
+import org.chromium.base.library_loader.LibraryProcessType;
+import org.chromium.base.library_loader.ProcessInitException;
+import org.chromium.chrome.browser.init.BrowserParts;
+import org.chromium.chrome.browser.init.ChromeBrowserInitializer;
+import org.chromium.chrome.browser.init.EmptyBrowserParts;
+import org.chromium.components.background_task_scheduler.BackgroundTask;
+import org.chromium.components.background_task_scheduler.TaskParameters;
+import org.chromium.content.browser.BrowserStartupController;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Base class implementing {@link BackgroundTask} that adds native initialization, ensuring that
+ * tasks are run after Chrome is successfully started.
+ * Background tasks that require native parts of browser to be started should extend this class
+ * instead of implementing the interface directly. {@link #onStartTaskWithNative(Context,
+ * TaskParameters, TaskFinishedCallback)} method should be implemented instead of {@link
+ * #onStartTask(Context, TaskParameters, TaskFinishedCallback)}.

[Ted C, 2017/05/23 20:52:55]

I'd argue the last sentence isn't necessary, onStartTaskWithNative is abstract
and onStartTask is final, so it should be pretty self explanatory what a
embedder should do.

[fgorski, 2017/05/23 21:35:02]

Done.

+ */
+public abstract class NativeBackgroundTask implements BackgroundTask {
+    private static final String TAG = "BTS_NativeBkgrdTask";
+
+    /** Specifies which action to take following onStartTaskBeforeNativeLoaded. */
+    @Retention(RetentionPolicy.SOURCE)
+    @IntDef({LOAD_NATIVE, RESCHEDULE, DONE})
+    public @interface StartBeforeNativeResult {}
+    /** Task should continue to load native parts of browser. */
+    public static final int LOAD_NATIVE = 0;
+    /** Task should request rescheduling, without loading native parts of browser. */
+    public static final int RESCHEDULE = 1;
+    /** Task should neither load native parts of browser nor reschedule. */
+    public static final int DONE = 2;
+
+    protected NativeBackgroundTask() {}
+
+    private boolean mTaskStopped;

[nyquist, 2017/05/23 20:14:28]

Nit: Could you add a comment that this must only ever be accessed on the UI
thread?

[fgorski, 2017/05/23 21:35:02]

Done.

+
+    @Override
+    public final boolean onStartTask(
+            Context context, TaskParameters taskParameters, TaskFinishedCallback callback) {
+        @StartBeforeNativeResult
+        int beforeNativeResult = onStartTaskBeforeNativeLoaded(context, taskParameters, callback);
+
+        if (beforeNativeResult == DONE) return false;
+
+        if (beforeNativeResult == RESCHEDULE) {
+            ThreadUtils.postOnUiThread(buildRescheduleRunnable(callback));
+            return true;
+        }
+
+        assert beforeNativeResult == LOAD_NATIVE;
+        runWithNative(context, buildStartWithNativeRunnable(context, taskParameters, callback),
+                buildRescheduleRunnable(callback));
+        return true;
+    }
+
+    @Override
+    public final boolean onStopTask(Context context, TaskParameters taskParameters) {
+        mTaskStopped = true;
+        if (isNativeLoaded()) {
+            return onStopTaskWithNative(context, taskParameters);
+        } else {
+            return onStopTaskBeforeNativeLoaded(context, taskParameters);
+        }
+    }
+
+    /**
+     * Ensure that native is started before running the task. If native fails to start, the task is
+     * going to be rescheduled, by issuing a {@see TaskFinishedCallback} with parameter set to
+     * <c>true</c>.
+     *
+     * @param context the current context
+     * @param startWithNativeRunnable A runnable that will execute #onStartTaskWithNative, after the
+     *    native is loaded.
+     * @param rescheduleRunnable A runnable that will be called to reschedule the task in case
+     *    native initialization fails.
+     */
+    protected final void runWithNative(final Context context,
+            final Runnable startWithNativeRunnable, final Runnable rescheduleRunnable) {
+        if (isNativeLoaded()) {
+            ThreadUtils.postOnUiThread(startWithNativeRunnable);
+            return;
+        }
+
+        final BrowserParts parts = new EmptyBrowserParts() {
+            @Override
+            public void finishNativeInitialization() {
+                startWithNativeRunnable.run();
+            }
+            @Override
+            public void onStartupFailure() {
+                rescheduleRunnable.run();
+            }
+        };
+
+        ThreadUtils.postOnUiThread(new Runnable() {
+            @Override
+            public void run() {
+                try {
+                    ChromeBrowserInitializer.getInstance(context).handlePreNativeStartup(parts);

[Ted C, 2017/05/23 20:52:55]

Should this do:

if (mTaskStopped) return;

[fgorski, 2017/05/23 21:35:02]

I am putting it before the try, presuming you didn't mean for that check to
happen between pre-native and post-native. Let me know if that's incorrect.

[Ted C, 2017/05/23 22:06:43]

Nope, that's right

+                    ChromeBrowserInitializer.getInstance(context).handlePostNativeStartup(
+                            true /* isAsync */, parts);
+                } catch (ProcessInitException e) {
+                    Log.e(TAG, "ProcessInitException while starting the browser process.");
+                    if (!mTaskStopped) {
+                        rescheduleRunnable.run();
+                    }
+                    return;
+                }
+            }
+        });
+    }
+
+    /**
+     * Method that should be implemented in derived classes to provide implementation of {@link
+     * BackgroundTask#onStartTask(Context, TaskParameters, TaskFinishedCallback)} run before native
+     * is loaded. Task implementing the method may decide to not load native if it hasn't been
+     * loaded yet, by returning DONE, meaning no more work is required for the task, or RESCHEDULE,
+     * meaning task needs to be immediately rescheduled.
+     * This method is guaranteed to be called before {@link #onStartTaskWithNative}.
+     */
+    @StartBeforeNativeResult
+    protected abstract int onStartTaskBeforeNativeLoaded(
+            Context context, TaskParameters taskParameters, TaskFinishedCallback callback);
+
+    /**
+     * Method that should be implemented in derived classes to provide implementation of {@link
+     * BackgroundTask#onStartTask(Context, TaskParameters, TaskFinishedCallback)} when native is
+     * loaded.
+     * This method will not be called unless {@link #onStartTaskBeforeNativeLoaded} returns
+     * LOAD_NATIVE.
+     */
+    protected abstract void onStartTaskWithNative(
+            Context context, TaskParameters taskParameters, TaskFinishedCallback callback);
+
+    /** Called by {@link #onStopTask} if native part of browser was not loaded. */
+    protected abstract boolean onStopTaskBeforeNativeLoaded(
+            Context context, TaskParameters taskParameters);
+
+    /** Called by {@link #onStopTask} if native part of browser was loaded. */
+    protected abstract boolean onStopTaskWithNative(Context context, TaskParameters taskParameters);
+
+    /** Builds a runnable rescheduling task. */
+    private Runnable buildRescheduleRunnable(final TaskFinishedCallback callback) {
+        return new Runnable() {
+            @Override
+            public void run() {
+                callback.taskFinished(true);

[Ted C, 2017/05/23 20:52:55]

Same question as above.  Should this do:

if (mTaskStopped) return;

Should we do ThreadUtil.assertOnUiThread here?

[fgorski, 2017/05/23 21:35:02]

Done.
Good point. I went ahead and added checks in both runnables, making sure they
are posted to or invoked from UI thread only.

+            }
+        };
+    }
+
+    /** Builds a runnable starting task with native portion. */
+    private Runnable buildStartWithNativeRunnable(final Context context,
+            final TaskParameters taskParameters, final TaskFinishedCallback callback) {
+        return new Runnable() {
+            @Override
+            public void run() {
+                onStartTaskWithNative(context, taskParameters, callback);

[Ted C, 2017/05/23 20:52:55]

same about mTaskStopped?

[fgorski, 2017/05/23 21:35:02]

Done.

+            }
+        };
+    }
+
+    /** Whether the native part of the browser is loaded. */
+    private boolean isNativeLoaded() {
+        return BrowserStartupController.get(LibraryProcessType.PROCESS_BROWSER)
+                .isStartupSuccessfullyCompleted();
+    }
+}