ScreenCapture for Android phase1, part II

chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java

Index: chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java b/chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java
index 44e8838341b16c7dc730d63d857e565bf488c85d..c199dc1c7d0bd1c4bea182d7e3be35172d13bb03 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java
@@ -19,6 +19,7 @@ import org.chromium.base.ContextUtils;
 import org.chromium.base.Log;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.browser.tab.TabWebContentsDelegateAndroid;
 
 import java.net.MalformedURLException;
 import java.net.URL;
@@ -30,6 +31,10 @@ import java.util.Set;
  * Service that creates/destroys the WebRTC notification when media capture starts/stops.
  */
 public class MediaCaptureNotificationService extends Service {
+    private static final String ACTION_MEDIA_CAPTURE_UPDATE =
+            "org.chromium.chrome.browser.media.SCREEN_CAPTURE_UPDATE";
+    private static final String ACTION_SCREEN_CAPTURE_STOP =
+            "org.chromium.chrome.browser.media.SCREEN_CAPTURE_STOP";
 
     private static final String NOTIFICATION_NAMESPACE = "MediaCaptureNotificationService";
 
@@ -44,6 +49,7 @@ public class MediaCaptureNotificationService extends Service {
     private static final int MEDIATYPE_AUDIO_AND_VIDEO = 1;
     private static final int MEDIATYPE_VIDEO_ONLY = 2;
     private static final int MEDIATYPE_AUDIO_ONLY = 3;
+    private static final int MEDIATYPE_SCREEN_CAPTURE = 4;
 
     private NotificationManager mNotificationManager;
     private Context mContext;
@@ -83,10 +89,25 @@ public class MediaCaptureNotificationService extends Service {
             cancelPreviousWebRtcNotifications();
             stopSelf();
         } else {
-            updateNotification(
-                    intent.getIntExtra(NOTIFICATION_ID_EXTRA, Tab.INVALID_TAB_ID),
-                    intent.getIntExtra(NOTIFICATION_MEDIA_TYPE_EXTRA, MEDIATYPE_NO_MEDIA),
-                    intent.getStringExtra(NOTIFICATION_MEDIA_URL_EXTRA));
+            String action = intent.getAction();
+            int notificationId = intent.getIntExtra(NOTIFICATION_ID_EXTRA, Tab.INVALID_TAB_ID);
+            int mediaType = intent.getIntExtra(NOTIFICATION_MEDIA_TYPE_EXTRA, MEDIATYPE_NO_MEDIA);
+            String url = intent.getStringExtra(NOTIFICATION_MEDIA_URL_EXTRA);
+
+            switch (action) {
+                case ACTION_MEDIA_CAPTURE_UPDATE:

[Ted C, 2016/08/16 00:19:19]

since we only have two actions, I think 

if (ACTION_MEDIA_CAPTURE_UPDATE.equals(action)) {
   ...
} else if (ACTION_SCREEN_CAPTURE_STOP.equals(action)) {
   ...
}

is clearer and has less code.

[braveyao, 2016/08/16 23:05:49]

Done.

+                    updateNotification(notificationId, mediaType, url);
+                    break;
+
+                case ACTION_SCREEN_CAPTURE_STOP:
+                    // Notify native to stop screen capture when the STOP button in notification
+                    // is clicked.
+                    TabWebContentsDelegateAndroid.notifyStopped(notificationId);
+                    break;
+
+                default:
+                    break;
+            }
         }
         return super.onStartCommand(intent, flags, startId);
     }
@@ -122,7 +143,7 @@ public class MediaCaptureNotificationService extends Service {
         }
         destroyNotification(notificationId);
         if (mediaType != MEDIATYPE_NO_MEDIA) {
-            createNotification(notificationId, mediaType, url);
+            createNotification(notificationId, mediaType, url, true);

[Ted C, 2016/08/16 00:19:19]

this is only ever called once...and we always pass true.  let's get rid of the
new boolean and simply key it off of MEDIATYPE_SCREEN_CAPTURE

[braveyao, 2016/08/16 23:05:49]

Done.
Once there was another action added, which is removed according to UX
suggestion. I intended to still keep things generic here and there. Sorry about
that.

         }
         if (mNotifications.size() == 0) stopSelf();
     }
@@ -145,7 +166,8 @@ public class MediaCaptureNotificationService extends Service {
      * @param mediaType Media type of the notification.
      * @param url Url of the current webrtc call.
      */
-    private void createNotification(int notificationId, int mediaType, String url) {
+    private void createNotification(
+            int notificationId, int mediaType, String url, boolean headsup) {
         int notificationContentTextId = 0;
         int notificationIconId = 0;
         if (mediaType == MEDIATYPE_AUDIO_AND_VIDEO) {
@@ -157,6 +179,9 @@ public class MediaCaptureNotificationService extends Service {
         } else if (mediaType == MEDIATYPE_AUDIO_ONLY) {
             notificationContentTextId = R.string.audio_call_notification_text_2;
             notificationIconId = R.drawable.webrtc_audio;
+        } else if (mediaType == MEDIATYPE_SCREEN_CAPTURE) {
+            notificationContentTextId = R.string.screen_capture_notification_text;
+            notificationIconId = R.drawable.webrtc_video;
         }
 
         NotificationCompat.Builder builder = new NotificationCompat.Builder(mContext)
@@ -166,15 +191,28 @@ public class MediaCaptureNotificationService extends Service {
                 .setSmallIcon(notificationIconId)
                 .setLocalOnly(true);
 
-        StringBuilder contentText = new StringBuilder(
-                mContext.getResources().getString(notificationContentTextId)).append('.');
+        StringBuilder contentText =
+                new StringBuilder(mContext.getResources().getString(notificationContentTextId, url))

[Ted C, 2016/08/16 00:19:19]

While extra arguments are dropped in the underlying format call, I'm not a big
fan of passing the url as an argument that is only used by one of the 4 options.

Maybe we should just pull out two helper functions getNotificationContentText
and getNotificationIconId

Then we can make this a bit clearer.

[braveyao, 2016/08/16 23:05:49]

Done.

+                        .append('.');
         Intent tabIntent = Tab.createBringTabToFrontIntent(notificationId);
         if (tabIntent != null) {
             PendingIntent contentIntent = PendingIntent.getActivity(
                     mContext, notificationId, tabIntent, 0);
             builder.setContentIntent(contentIntent);
-            contentText.append(
-                    mContext.getResources().getString(R.string.media_notification_link_text, url));
+            if (mediaType == MEDIATYPE_SCREEN_CAPTURE) {
+                // Add a "Stop" button to the screen capture notification. If necessary, turn the
+                // notification into a high priority one.
+                if (headsup) {
+                    builder.setPriority(Notification.PRIORITY_HIGH);
+                    builder.setVibrate(new long[0]);

[Ted C, 2016/08/16 00:19:19]

we don't vibrate for any other notifications...seems odd to do it here.  And I
think we should pass an explicit duration of what we think is best instead,
maybe 50 or 100

[braveyao, 2016/08/16 23:05:49]

We don't vibrate for screen share too. 'setVibrate' is just necessary to show
the headsup notification. 

+                }
+                builder.addAction(R.drawable.ic_vidcontrol_stop, "Stop",

[Ted C, 2016/08/16 00:19:19]

"Stop" should be translated...and we might already have a Stop that you can use
(ACCESSIBILITY_STOP for example)

[braveyao, 2016/08/16 23:05:49]

Done.

+                        buildPendingIntent(ACTION_SCREEN_CAPTURE_STOP, notificationId, mediaType,

[Ted C, 2016/08/16 00:19:19]

again, let's not make this overly generic for now.  just do
buildStopCapturePendingIntent(int notificationId) and infer the rest

[braveyao, 2016/08/16 23:05:49]

Done.

+                                          url));

[Ted C, 2016/08/16 00:19:19]

clang formatting doesn't work perfectly in java...should be indented 8 from the
start of the previous line

[braveyao, 2016/08/16 23:05:49]

Done.

+            } else {
+                contentText.append(mContext.getResources().getString(
+                        R.string.media_notification_link_text, url));
+            }
         } else {
             contentText.append(" ").append(url);
         }
@@ -227,10 +265,13 @@ public class MediaCaptureNotificationService extends Service {
     /**
      * @param audio If audio is being captured.
      * @param video If video is being captured.
+     * @param screen If screen is being captured.
      * @return A constant identify what media is being captured.
      */
-    public static int getMediaType(boolean audio, boolean video) {
-        if (audio && video) {
+    public static int getMediaType(boolean audio, boolean video, boolean screen) {
+        if (screen) {
+            return MEDIATYPE_SCREEN_CAPTURE;
+        } else if (audio && video) {
             return MEDIATYPE_AUDIO_AND_VIDEO;
         } else if (audio) {
             return MEDIATYPE_AUDIO_ONLY;
@@ -264,10 +305,10 @@ public class MediaCaptureNotificationService extends Service {
      * @param fullUrl Url of the current webrtc call.
      */
     public static void updateMediaNotificationForTab(
-            Context context, int tabId, boolean audio, boolean video, String fullUrl) {
-        int mediaType = getMediaType(audio, video);
+            Context context, int tabId, int mediaType, String fullUrl) {

[Ted C, 2016/08/16 00:19:19]

update javadoc

[braveyao, 2016/08/16 23:05:49]

Done.

         if (!shouldStartService(context, mediaType, tabId)) return;
         Intent intent = new Intent(context, MediaCaptureNotificationService.class);
+        intent.setAction(ACTION_MEDIA_CAPTURE_UPDATE);
         intent.putExtra(NOTIFICATION_ID_EXTRA, tabId);
         String baseUrl = fullUrl;
         try {
@@ -293,4 +334,18 @@ public class MediaCaptureNotificationService extends Service {
 
         context.startService(new Intent(context, MediaCaptureNotificationService.class));
     }
+
+    /**
+     * Build PendingIntent for the actions of screen capture notification.
+     */
+    private PendingIntent buildPendingIntent(
+            String action, int notificationId, int mediaType, String url) {
+        Intent intent = new Intent(this, MediaCaptureNotificationService.class);
+        intent.setAction(action);
+        intent.putExtra(NOTIFICATION_ID_EXTRA, notificationId);
+        intent.putExtra(NOTIFICATION_MEDIA_TYPE_EXTRA, mediaType);
+        intent.putExtra(NOTIFICATION_MEDIA_URL_EXTRA, url);
+        return PendingIntent.getService(
+                mContext, notificationId, intent, PendingIntent.FLAG_UPDATE_CURRENT);
+    }
 }