[Android] A prototype of the interactive media notification.

chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java

+// Copyright 2015 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.media.ui;
+
+import android.app.Notification;
+import android.app.PendingIntent;
+import android.app.Service;
+import android.content.Context;
+import android.content.Intent;
+import android.os.Handler;
+import android.os.IBinder;
+import android.provider.Browser;
+import android.support.v4.app.NotificationCompat;
+import android.support.v4.app.NotificationManagerCompat;
+import android.widget.RemoteViews;
+
+import org.chromium.chrome.R;
+import org.chromium.chrome.browser.IntentHandler.TabOpenType;
+
+import java.util.Set;
+import java.util.concurrent.CopyOnWriteArraySet;
+
+/**
+ * A class for notifications that provide information and optional media controls for a given media.
+ * Internally implements a Service for transforming notification Intents into
+ * {@link NotificationMediaPlaybackControls.Listener} calls for all registered listeners.
+ */
+public class NotificationMediaPlaybackControls {
+    /**
+     * Interface for classes that need to be notified about play/pause events.
+     */
+    public static interface Listener {

[Ted C, 2015/06/26 00:43:53]

I would call this MediaPlaybackListener so it is easier to grok out of context
of this file.

[whywhat, 2015/06/26 19:29:32]

Done.

+        /**
+         * Called when the user wants to resume the playback.
+         */
+        void onPlay();
+
+        /**
+         * Called when the user wants to pause the playback.
+         */
+        void onPause();
+    }
+
+    /**
+     * Service used to transform intent requests triggered from the notification into
+     * {@code Listener} callbacks. Ideally this class should be private, but public is required to
+     * create as a service.
+     */
+    public static class ListenerService extends Service {
+        private static final String ACTION_PREFIX =
+                "NotificationMediaPlaybackControls.ListenerService.";
+
+        // Constants used by intent actions
+        public static final int ACTION_ID_PLAY = 0;

[Ted C, 2015/06/26 00:43:54]

I'm confused why there is this mapping?

Why not just

private static final String PLAY_ACTION =
        "NotificationMediaPlaybackControls.ListenerService.PLAY";
private static final String PAUSE_ACTION =
        "NotificationMediaPlaybackControls.ListenerService.PAUSE";

Why store the pending intents?  why not just generate them as needed?

This isn't going to be a very frequent occurrence, so that seems overkill.

[whywhat, 2015/06/26 19:29:31]

The notification may be recreated every second or so when/if we add a progress
bar to it.
Also, stripping the prefix from the intent's action is an easy way to dismiss
all actions we don't care about.
I agree it's an overkill for now. Updated the CL.

+        public static final int ACTION_ID_PAUSE = 1;
+
+        // Must be kept in sync with the ACTION_ID_XXX constants above
+        private static final String[] ACTION_VERBS = { "PLAY", "PAUSE" };
+
+        private PendingIntent[] mPendingIntents;
+
+        PendingIntent getPendingIntent(int id) {
+            return mPendingIntents[id];
+        }
+
+        @Override
+        public IBinder onBind(Intent intent) {
+            return null;
+        }
+
+        @Override
+        public void onCreate() {
+            super.onCreate();
+
+            // Create all the PendingIntents
+            int actionCount = ACTION_VERBS.length;
+            mPendingIntents = new PendingIntent[actionCount];
+            for (int i = 0; i < actionCount; ++i) {
+                Intent intent = new Intent(this, ListenerService.class);
+                intent.setAction(ACTION_PREFIX + ACTION_VERBS[i]);
+                mPendingIntents[i] = PendingIntent.getService(this, 0, intent,
+                        PendingIntent.FLAG_CANCEL_CURRENT);
+            }
+            onServiceStarted(this);
+        }
+
+        @Override
+        public void onDestroy() {
+            onServiceDestroyed();
+        }
+
+        @Override
+        public int onStartCommand(Intent intent, int flags, int startId) {
+            if (intent == null) {
+                stopSelf();
+                return START_NOT_STICKY;
+            }
+
+            String action = intent.getAction();
+            if (action != null && action.startsWith(ACTION_PREFIX)) {
+                Set<Listener> listeners = sInstance.getListeners();
+
+                // Strip the prefix for matching the verb
+                action = action.substring(ACTION_PREFIX.length());
+                if (ACTION_VERBS[ACTION_ID_PLAY].equals(action)) {
+                    for (Listener listener : listeners) listener.onPlay();
+                    getOrCreate(getApplicationContext()).onPlaybackStateChanged(false);

[Ted C, 2015/06/26 00:43:54]

why getOrCreate here but you use sInstance above.  is the above a potential NPE
case?

I think if sInstance is null, you should treat it in the intent == null case at
the top.

[whywhat, 2015/06/26 19:29:31]

Done.

+                } else if (ACTION_VERBS[ACTION_ID_PAUSE].equals(action)) {
+                    for (Listener listener : listeners) listener.onPause();
+                    getOrCreate(getApplicationContext()).onPlaybackStateChanged(true);
+                }
+            }
+
+            return START_STICKY;
+        }
+    }
+
+    private static NotificationMediaPlaybackControls sInstance = null;

[Ted C, 2015/06/26 00:43:54]

null is the default, so don't need it.

[whywhat, 2015/06/26 19:29:32]

Done.

+    private static final Object LOCK = new Object();

[Ted C, 2015/06/26 00:43:54]

move the static finals above the plain ol' static.

I would consider moving these above the class definitions above.

[whywhat, 2015/06/26 19:29:31]

Done.

+    private static final int MSG_UPDATE_NOTIFICATION = 100;

[Ted C, 2015/06/26 00:43:53]

I would call this MSG_ID_UPDATE_...

[whywhat, 2015/06/26 19:29:32]

Done.

+
+    /**
+     * Returns the singleton NotificationMediaPlaybackControls object.
+     *
+     * @param context The Context that the notification service needs to be created in.
+     * @return A {@code NotificationMediaPlaybackControls} object.
+     */
+    public static NotificationMediaPlaybackControls getOrCreate(Context context) {

[Ted C, 2015/06/26 00:43:54]

Seems like a few places in the Tab logic would benefit from just get w/o create
(i.e. if hiding and one doesn't exist).

[whywhat, 2015/06/26 19:29:31]

Done.

+        synchronized (LOCK) {
+            if (sInstance == null) {
+                sInstance = new NotificationMediaPlaybackControls(context);
+            }
+
+            return sInstance;
+        }
+    }
+
+    private static void onServiceDestroyed() {
+        sInstance.destroyNotification();
+        sInstance.mService = null;
+    }
+
+    private static void onServiceStarted(ListenerService service) {
+        sInstance.mService = service;

[Ted C, 2015/06/26 00:43:54]

assert mService == null?

+        sInstance.createNotification();
+    }
+
+    private final Context mContext;
+
+    // ListenerService running for the notification. Only non-null when showing.
+    private ListenerService mService;
+
+    private final String mPlayDescription;
+
+    private final String mPauseDescription;
+
+    private Notification mNotification;
+
+    private Handler mHandler;
+    private Set<Listener> mListeners;

[Ted C, 2015/06/26 00:43:54]

Should this be an observerList?

[whywhat, 2015/06/26 19:29:31]

Done.

+    private MediaInfo mMediaInfo;

[Ted C, 2015/06/26 00:43:54]

Can there ever only be one media info shown?  If so, we should have asserts in
place ensuring that.  Same for listeners?

[whywhat, 2015/06/26 19:29:32]

Currently, yes and yes (one media info, one listener).
However, the hide request can come from the previous tab after the current one
set its own.

+
+    private NotificationMediaPlaybackControls(Context context) {
+        this.mContext = context;

[Ted C, 2015/06/26 00:43:54]

"this." is unnecessary

[whywhat, 2015/06/26 19:29:31]

Done.

+        mHandler = new Handler(context.getMainLooper()) {
+            @Override
+            public void handleMessage(android.os.Message msg) {
+                if (msg.what == MSG_UPDATE_NOTIFICATION) {
+                    mHandler.removeMessages(MSG_UPDATE_NOTIFICATION); // Only one update is needed.
+                    updateNotificationInternal();
+                }
+            }
+        };
+
+        mPlayDescription = context.getResources().getString(R.string.accessibility_play);
+        mPauseDescription = context.getResources().getString(R.string.accessibility_pause);
+    }
+
+    public void hide(int tabId) {
+        if (getMediaInfo() == null || tabId != getMediaInfo().tabId) return;
+

[Ted C, 2015/06/26 00:43:54]

should you be setting mMediaInfo = null here?

[whywhat, 2015/06/26 19:29:31]

done.

+        mContext.stopService(new Intent(mContext, ListenerService.class));
+    }
+
+    /**
+     * @return true if the notification is currently visible to the user.
+     */
+    public boolean isShowing() {
+        return mService != null;
+    }
+
+    public void onPlaybackStateChanged(boolean isPaused) {

[Ted C, 2015/06/26 00:43:55]

there seems to be a fair amount of public methods that aren't accessed from
outside this file.  Please reduce the scope as needed and ensure that all public
methods have javadoc.

[whywhat, 2015/06/26 19:29:31]

Done.

+        MediaInfo mediaInfo = new MediaInfo(getMediaInfo());
+        mediaInfo.isPaused = isPaused;
+        setMediaInfo(mediaInfo);
+    }
+
+    public void show(MediaInfo mediaInfo) {
+        setMediaInfo(mediaInfo);
+        mContext.startService(new Intent(mContext, ListenerService.class));
+    }
+
+    private void updateNotification() {
+        // Defer the call to updateNotificationInternal() so it can be cancelled in
+        // destroyNotification(). This is done to avoid the OS bug b/8798662.
+        mHandler.sendEmptyMessage(MSG_UPDATE_NOTIFICATION);
+    }
+
+    Notification getNotification() {

[Ted C, 2015/06/26 00:43:54]

why getNotification instead of just referencing mNotification (since it seems to
be scoped to just this file), same for getService() and getContext()

+        return mNotification;
+    }
+
+    final ListenerService getService() {
+        return mService;
+    }
+
+    private void onMediaInfoChanged() {

[Ted C, 2015/06/26 00:43:54]

only called one place, inline it

+        if (isShowing()) updateNotification();
+    }
+
+    private RemoteViews createContentView() {
+        RemoteViews contentView =
+                new RemoteViews(getContext().getPackageName(), R.layout.playback_notification_bar);
+        return contentView;
+    }
+
+    private void createNotification() {
+        NotificationCompat.Builder notificationBuilder =
+                new NotificationCompat.Builder(getContext())
+                .setSmallIcon(R.drawable.audio_playing)
+                .setAutoCancel(false)
+                .setVisibility(NotificationCompat.VISIBILITY_PUBLIC)
+                .setOngoing(true)
+                .setContent(createContentView())
+                .setContentIntent(createContentIntent());
+        mNotification = notificationBuilder.build();
+        updateNotification();
+    }
+
+    private void destroyNotification() {

[Ted C, 2015/06/26 00:43:54]

For the MediaNotificationService, we found destroy to be pretty unreliable on
the service if swiped away.

So we had to call MediaNotificationService#clearMediaNotifications from
onDeferredStartup, so you might
need to do something like that as well.

+        // Cancel any pending updates - we're about to tear down the notification.
+        mHandler.removeMessages(MSG_UPDATE_NOTIFICATION);
+
+        NotificationManagerCompat manager = NotificationManagerCompat.from(getContext());
+
+        manager.cancel(R.id.media_playback_notification);
+        mNotification = null;
+    }
+
+    private final Context getContext() {
+        return mContext;
+    }
+
+    private String getStatus() {
+        Context context = getContext();
+        MediaInfo mediaInfo = getMediaInfo();
+        return context.getString(R.string.media_notification_link_text,
+                mediaInfo.origin != null ? mediaInfo.origin : "");
+    }
+
+    private String getTitle() {
+        Context context = getContext();
+        MediaInfo mediaInfo = getMediaInfo();
+        String mediaTitle = mediaInfo.title;
+        if (mediaInfo.isPaused) {
+            return context.getString(
+                    R.string.media_playback_notification_paused_for_media, mediaTitle);
+        }
+        return context.getString(
+                R.string.media_playback_notification_playing_for_media, mediaTitle);
+    }
+
+    private PendingIntent createContentIntent() {
+        int tabId = getMediaInfo().tabId;
+        Intent intent = new Intent(Intent.ACTION_MAIN);
+        intent.putExtra(Browser.EXTRA_APPLICATION_ID, getContext().getPackageName());
+        intent.putExtra(TabOpenType.BRING_TAB_TO_FRONT.name(), tabId);
+        intent.setPackage(mContext.getPackageName());
+        return PendingIntent.getActivity(getContext(), tabId, intent, 0);
+    }
+
+    private void updateNotificationInternal() {
+        RemoteViews contentView = createContentView();
+
+        contentView.setTextViewText(R.id.title, getTitle());
+        contentView.setTextViewText(R.id.status, getStatus());
+        contentView.setImageViewResource(R.id.icon, R.drawable.audio_playing);
+
+        MediaInfo mediaInfo = getMediaInfo();
+        if (mediaInfo == null) return;

[Ted C, 2015/06/26 00:43:54]

Should you be destroying the notification in this case?  Should this ever
happen?  if not, assert here.

[whywhat, 2015/06/26 19:29:32]

Done.

+
+        if (mediaInfo.isPaused) {
+            contentView.setImageViewResource(R.id.playpause, R.drawable.ic_vidcontrol_play);
+            contentView.setContentDescription(R.id.playpause, mPlayDescription);
+            contentView.setOnClickPendingIntent(R.id.playpause,
+                    getService().getPendingIntent(ListenerService.ACTION_ID_PLAY));
+        } else {
+            contentView.setImageViewResource(R.id.playpause, R.drawable.ic_vidcontrol_pause);
+            contentView.setContentDescription(R.id.playpause, mPauseDescription);
+            contentView.setOnClickPendingIntent(R.id.playpause,
+                    getService().getPendingIntent(ListenerService.ACTION_ID_PAUSE));
+        }
+
+        mNotification.contentView = contentView;
+
+        NotificationManagerCompat manager = NotificationManagerCompat.from(getContext());
+        manager.notify(R.id.media_playback_notification, mNotification);
+
+        getService().startForeground(R.id.media_playback_notification, mNotification);
+    }
+
+    /**
+     * @return the media information previously assigned with
+     * {@link #setMediaInfo(MediaInfo)}, or {@code null} if the {@link MediaInfo}
+     * has not yet been assigned.
+     */
+    public final MediaInfo getMediaInfo() {
+        return mMediaInfo;
+    }
+
+    /**
+     * Sets the media information to display on the MediaPlaybackControls.
+     * @param mediaInfo the media information to use.
+     */
+    public final void setMediaInfo(MediaInfo mediaInfo) {
+        if ((mediaInfo == null && mMediaInfo == null)
+                || (mMediaInfo != null && mMediaInfo.equals(mediaInfo))) {
+            return;
+        }
+
+        mMediaInfo = mediaInfo;
+        onMediaInfoChanged();
+    }
+
+    /**
+     * Registers a {@link Listener} with the MediaPlaybackControls.
+     * @param listener the Listener to be registered.
+     */
+    public void addListener(Listener listener) {
+        getListeners().add(listener);
+    }
+
+    /**
+     * Unregisters a {@link Listener} previously registered with {@link #addListener(Listener)}.
+     * @param listener the Listener to be removed.
+     */
+    public void removeListener(Listener listener) {
+        getListeners().remove(listener);
+    }
+
+    /**
+     * @return the current list of listeners.
+     */
+    private final Set<Listener> getListeners() {
+        if (mListeners == null) mListeners = new CopyOnWriteArraySet<Listener>();

[Ted C, 2015/06/26 00:43:54]

I would just initialize this in the constructor.  ensuring it is always
non-null.

[whywhat, 2015/06/26 19:29:31]

Done.

+        return mListeners;
+    }
+}