Notification clicks on Android shouldn't pass an action index of -1

Bug: 789145
Change-Id: Iffd4099a6eb58cc532fb39bddda8581fd5a65a31
Reviewed-on: https://chromium-review.googlesource.com/793810
Commit-Queue: Peter Beverloo <peter@chromium.org>
Reviewed-by: Anita Woodruff <awdf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519747}

chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java

@@ -4,6 +4,8 @@
 
 package org.chromium.chrome.browser.notifications;
 
+import static org.junit.Assert.assertThat;
+
 import static org.chromium.base.test.util.ScalableTimeout.scaleTimeout;
 
 import android.annotation.TargetApi;
@@ -21,6 +23,7 @@ import android.support.test.InstrumentationRegistry;
 import android.support.test.filters.LargeTest;
 import android.support.test.filters.MediumTest;
 
+import org.hamcrest.Matchers;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Rule;
@@ -34,6 +37,7 @@ import org.chromium.base.test.util.DisabledTest;
 import org.chromium.base.test.util.Feature;
 import org.chromium.base.test.util.MinAndroidSdkLevel;
 import org.chromium.base.test.util.RetryOnFailure;
+import org.chromium.base.test.util.UserActionTester;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.browser.engagement.SiteEngagementService;
@@ -53,6 +57,8 @@ import org.chromium.content.browser.test.util.Criteria;
 import org.chromium.content.browser.test.util.CriteriaHelper;
 
 import java.net.URL;
+import java.util.ArrayList;
+import java.util.Iterator;
 import java.util.List;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
@@ -343,6 +349,8 @@ public class NotificationPlatformBridgeTest {
         mNotificationTestRule.setNotificationContentSettingForCurrentOrigin(ContentSetting.ALLOW);
         Context context = InstrumentationRegistry.getTargetContext();
 
+        UserActionTester actionTester = new UserActionTester();
+
         // +5 engagement from notification permission and +0.5 from navigating to the test page.
         Assert.assertEquals(5.5, getEngagementScoreBlocking(), 0);
         Notification notification = mNotificationTestRule.showAndGetNotification("MyNotification",
@@ -364,6 +372,11 @@ public class NotificationPlatformBridgeTest {
         // Expect +1 engagement from interacting with the notification.
         waitForTitle("reply: My Reply");
         Assert.assertEquals(6.5, getEngagementScoreBlocking(), 0);
+
+        // Replies are always delivered to an action button.
+        assertThat(actionTester.toString(), getNotificationActions(actionTester),
+                Matchers.contains("Notifications.Persistent.Shown",
+                        "Notifications.Persistent.ClickedActionButton"));
     }
 
     /**
@@ -692,6 +705,8 @@ public class NotificationPlatformBridgeTest {
         // +5 engagement from notification permission and +0.5 from navigating to the test page.
         Assert.assertEquals(5.5, getEngagementScoreBlocking(), 0);
 
+        UserActionTester actionTester = new UserActionTester();
+
         Notification notification =
                 mNotificationTestRule.showAndGetNotification("MyNotification", "{}");
 
@@ -712,6 +727,11 @@ public class NotificationPlatformBridgeTest {
                     RecordHistogram.getHistogramTotalCountForTesting(
                             "Notifications.Android.JobStartDelay"));
         }
+
+        // Clicking on a notification should record the right user metrics.
+        assertThat(actionTester.toString(), getNotificationActions(actionTester),
+                Matchers.contains(
+                        "Notifications.Persistent.Shown", "Notifications.Persistent.Clicked"));
     }
 
     /**
@@ -859,4 +879,18 @@ public class NotificationPlatformBridgeTest {
         // Expect +1 engagement from interacting with the notification.
         Assert.assertEquals(7.5, getEngagementScoreBlocking(), 0);
     }
+
+    /**
+     * Get Notification related actions, filter all other actions to avoid flakes.
+     */
+    private List<String> getNotificationActions(UserActionTester actionTester) {
+        List<String> actions = new ArrayList<>(actionTester.getActions());
+        Iterator<String> it = actions.iterator();
+        while (it.hasNext()) {
+            if (!it.next().startsWith("Notifications.")) {
+                it.remove();
+            }
+        }
+        return actions;
+    }
 }

chrome/browser/notifications/notification_platform_bridge_android.cc

@@ -44,6 +44,10 @@ using base::android::ScopedJavaLocalRef;
 
 namespace {
 
+// Value used to represent the absence of a button index following a user
+// interaction with a notification.
+constexpr int kNotificationInvalidButtonIndex = -1;
+
 // A Java counterpart will be generated for this enum.
 // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.notifications
 enum NotificationActionType {
@@ -167,7 +171,7 @@ void NotificationPlatformBridgeAndroid::OnNotificationClicked(
     jboolean incognito,
     const JavaParamRef<jstring>& java_tag,
     const JavaParamRef<jstring>& java_webapk_package,
-    jint action_index,
+    jint java_action_index,
     const JavaParamRef<jstring>& java_reply) {
   std::string notification_id =
       ConvertJavaStringToUTF8(env, java_notification_id);
@@ -185,6 +189,10 @@ void NotificationPlatformBridgeAndroid::OnNotificationClicked(
   regenerated_notification_infos_[notification_id] =
       RegeneratedNotificationInfo(origin, scope_url, tag, webapk_package);
 
+  base::Optional<int> action_index;
+  if (java_action_index != kNotificationInvalidButtonIndex)
+    action_index = java_action_index;
+
   ProfileManager* profile_manager = g_browser_process->profile_manager();
   DCHECK(profile_manager);
 
@@ -192,7 +200,7 @@ void NotificationPlatformBridgeAndroid::OnNotificationClicked(
       profile_id, incognito,
       base::Bind(&ProfileLoadedCallback, NotificationCommon::CLICK,
                  NotificationHandler::Type::WEB_PERSISTENT, origin,
-                 notification_id, action_index, std::move(reply),
+                 notification_id, std::move(action_index), std::move(reply),
                  base::nullopt /* by_user */));
 }