OfflineContentProvider changes to start service

chrome/android/java/src/org/chromium/chrome/browser/download/items/DownloadNotificationServiceNotifierUi.java

Index: chrome/android/java/src/org/chromium/chrome/browser/download/items/DownloadNotificationServiceNotifierUi.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/download/items/DownloadNotificationServiceNotifierUi.java b/chrome/android/java/src/org/chromium/chrome/browser/download/items/DownloadNotificationServiceNotifierUi.java
new file mode 100644
index 0000000000000000000000000000000000000000..ee963fd1d9e45316b7eeb50ad2868bec19f0ce16
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/download/items/DownloadNotificationServiceNotifierUi.java
@@ -0,0 +1,125 @@
+// 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.download.items;
+
+import android.content.ComponentName;
+import android.content.Context;
+import android.content.Intent;
+import android.content.ServiceConnection;
+import android.os.IBinder;
+
+import org.chromium.base.Log;
+import org.chromium.chrome.browser.download.DownloadNotificationService;
+import org.chromium.chrome.browser.download.items.OfflineContentAggregatorNotifier.NotifierUi;
+import org.chromium.components.offline_items_collection.ContentId;
+import org.chromium.components.offline_items_collection.OfflineItem;
+
+import javax.annotation.Nullable;
+
+/**
+ * A implementation of {@link NotifierUi} that connects to the {@link DownloadNotificationService}.
+ * @see OfflineContentAggregatorNotifier
+ */
+public class DownloadNotificationServiceNotifierUi
+        implements NotifierUi, ServiceConnection, DownloadNotificationService.Observer {
+    private static final String TAG = "DownloadNotifierUi";
+
+    private final Context mApplicationContext;
+
+    /** Whether or not the underlying service is bound or awaiting a bind request result. */
+    private boolean mIsServiceBound;
+
+    /** Could be {@code null} if the service is unbound or hasn't been started. */
+    @Nullable
+    private DownloadNotificationService mBoundService;
+
+    /** The {@link Runnable} that should be called when our service is bound. */
+    @Nullable
+    private Runnable mOnBoundRunnable;
+
+    /**
+     * Creates an instance of the {@link DownloadNotificationServiceNotifierUi}.  Does not start
+     * or bind the {@link android.app.Service}.
+     * @param context The {@link Context} to use to do all {@link android.app.Service} interactions.
+     */
+    public DownloadNotificationServiceNotifierUi(Context context) {
+        mApplicationContext = context.getApplicationContext();

[gone, 2017/03/20 19:03:36]

Do you have a good reason not to use ContextUtils.getApplicationContext()?

[David Trainor- moved to gerrit, 2017/03/25 03:31:13]

Eh just a bit cleaner to pass in dependencies.  I'm fine getting rid of this as
long as the unit tests work nicely :).

+    }
+
+    private void startAndBindService() {
+        DownloadNotificationService.startDownloadNotificationService(mApplicationContext, null);
+        mApplicationContext.bindService(
+                new Intent(mApplicationContext, DownloadNotificationService.class), this,
+                Context.BIND_AUTO_CREATE);
+    }
+
+    private void unbindService() {
+        mApplicationContext.unbindService(this);
+    }
+
+    // OfflineContentAggregatorNotifier.NotifierUi implementation.
+    @Override
+    public boolean onUiNeeded(Runnable onReadyEvent) {
+        if (!mIsServiceBound) startAndBindService();
+        mIsServiceBound = true;

[gone, 2017/03/20 19:03:35]

seems like this boolean should go where you're actually starting and binding the
service

[David Trainor- moved to gerrit, 2017/03/25 03:31:12]

I didn't do it originally because I wanted to make startAndBindService and
unbindService mockable for unit tests.  Ended up not testing this layer (which
may or may not be a great idea...).  Will remove those helpers.

+
+        boolean isReady = mBoundService != null;
+        mOnBoundRunnable = isReady ? onReadyEvent : null;
+
+        return isReady;
+    }
+
+    @Override
+    public void onUiNotNeeded() {

[gone, 2017/03/20 19:03:36]

What if onUiNeeded was called and the mOnBoundRunnable was set?  Should you nuke
it here?

+        if (!mIsServiceBound) return;
+        if (mBoundService != null) mBoundService.removeObserver(this);
+        unbindService();
+        mBoundService = null;

[fgorski, 2017/03/20 20:19:20]

why not reset the values in unbindService?

[David Trainor- moved to gerrit, 2017/03/25 03:31:13]

See above comment on testing.  Will change though :).

+        mIsServiceBound = false;
+    }
+
+    @Override
+    public void updateItem(OfflineItem item) {
+        // TODO(dtrainor): Interact with the DownloadNotificationService.

[David Trainor- moved to gerrit, 2017/03/18 00:45:41]

This and "removeItem" will be implemented in a future patch.

+    }
+
+    @Override
+    public void removeItem(ContentId id) {
+        // TODO(dtrainor): Interact with the DownloadNotificationService.
+    }
+
+    // ServiceConnection implementation.
+    @Override
+    public void onServiceConnected(ComponentName className, IBinder service) {
+        if (!(service instanceof DownloadNotificationService.LocalBinder)) {
+            Log.w(TAG,
+                    "Not from DownloadNotificationService, do not connect."
+                            + " Component name: " + className);
+            assert false;
+            return;
+        }
+        mBoundService = ((DownloadNotificationService.LocalBinder) service).getService();
+        mBoundService.addObserver(this);
+
+        if (mOnBoundRunnable != null) {
+            mOnBoundRunnable.run();
+            mOnBoundRunnable = null;
+        }
+    }
+
+    @Override
+    public void onServiceDisconnected(ComponentName className) {}
+
+    // DownloadNotificationService.Observer implementation.
+    @Override
+    public void onServiceShutdownRequested() {
+        onUiNotNeeded();
+    }
+
+    @Override
+    public void onDownloadCanceled(String guid) {
+        // We rely on the cancel event to forward an update to the UI.
+    }
+}