Initial skeleton of custom layouts for Web Notifications.

chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java

Index: chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
index 965a3871c195be90ca9e3e9ca1d00160dd1f0d5a..957c01e49650cfd69d8e9a47f2ccaa34cb203a7c 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
@@ -18,13 +18,17 @@
 import android.text.Spannable;
 import android.text.SpannableStringBuilder;
 import android.text.TextUtils;
+import android.text.format.DateFormat;
 import android.text.style.StyleSpan;
 import android.util.Log;
+import android.widget.RemoteViews;
 
+import org.chromium.base.CommandLine;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.metrics.RecordUserAction;
 import org.chromium.chrome.R;
+import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.browser.preferences.Preferences;
 import org.chromium.chrome.browser.preferences.PreferencesLauncher;
 import org.chromium.chrome.browser.preferences.website.SingleCategoryPreferences;
@@ -34,6 +38,7 @@
 
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.util.Date;
 
 import javax.annotation.Nullable;
 
@@ -418,32 +423,39 @@ private void displayNotification(long persistentNotificationId, String origin, S
         PendingIntent pendingSettingsIntent = PendingIntent.getActivity(mAppContext,
                 PENDING_INTENT_REQUEST_CODE, settingsIntent, PendingIntent.FLAG_UPDATE_CURRENT);
 
-        NotificationCompat.Builder notificationBuilder = new NotificationCompat.Builder(mAppContext)
-                .setContentTitle(title)
-                .setContentText(body)
-                .setStyle(new NotificationCompat.BigTextStyle().bigText(body))
-                .setLargeIcon(ensureNormalizedIcon(icon, origin))
-                .setSmallIcon(R.drawable.ic_chrome)
-                .setContentIntent(makePendingIntent(
-                        NotificationConstants.ACTION_CLICK_NOTIFICATION,
-                        persistentNotificationId, origin, tag, -1 /* actionIndex */))
-                .setDeleteIntent(makePendingIntent(
-                        NotificationConstants.ACTION_CLOSE_NOTIFICATION,
-                        persistentNotificationId, origin, tag, -1 /* actionIndex */))
-                .setTicker(createTickerText(title, body))
-                .setSubText(origin);
-
-        for (int actionIndex = 0; actionIndex < actionTitles.length; actionIndex++) {
-            notificationBuilder.addAction(
-                    0 /* actionIcon */, actionTitles[actionIndex],
-                    makePendingIntent(NotificationConstants.ACTION_CLICK_NOTIFICATION,
-                                      persistentNotificationId, origin, tag, actionIndex));
+        NotificationCompat.Builder notificationBuilder =
+                new NotificationCompat.Builder(mAppContext)
+                        .setSmallIcon(R.drawable.ic_chrome);
+
+        if (useCustomLayouts()) {

[Peter Beverloo, 2015/10/12 14:08:30]

I'm a bit concerned that this isn't going to scale very well. Eventually
buildCustomLayout() would have to take pretty much all the options. Perhaps this
would be a valid place to apply the strategy pattern?

=========================
interface NotificationBuilder {
  NotificationBuilder setBody(String body);
  NotificationBuilder setContent(String content);
}

class NativeNotificationBuilder implements NotificationBuilder {
  NotificationBuilder setBody(String body) {
    mBuilder.setContentText(body);
    return this;
  }
}

class TemplateNotificationBuilder implements NotificationBuilder {
  NotificationBuilder setBody(String body) {
    mView.setTextViewText(R.id.body, body);
    return this;
  }
}
=========================

This would:

  - Clarify which features aren't supported by either implementation (e.g.
custom buttons)
  - Allow us to logically separate the Site Settings button/icon from the other
actions
  - Make the displayNotification() method significantly nicer to read.
  - Improve testability - instrumentation tests could make sure that the NB gets
the right information, unit tests could verify that the NB does the right thing
with it.

The only concern that I have here is that we might need quite some similar
mBuilder.setFoo() calls because of accessibility. More on that later.

[Michael van Ouwerkerk, 2015/10/12 17:10:59]

Seems like a good idea, I'll take a look.

+            notificationBuilder.setContent(buildCustomLayout(title, body, origin, icon));

[Peter Beverloo, 2015/10/12 14:08:30]

Do you have an understanding of how accessibility works for custom layouts? Or
how apps inspecting notifications (e.g. PushBullet or Hotpocket) deal with it?

I'd love to learn more about that, these are cases that we probably don't want
to break too badly.

[Michael van Ouwerkerk, 2015/10/12 17:10:59]

My understanding is that while this CL introduces a custom layout, it does not
introduce custom views, and there are good a11y hooks for the standard Android
views. So we can use contentDescription for image-only elements, and the
appropriate elements are focusable by default. We can tweak the focus order if
the default is somehow not quite right for the layout we made.

See this article for more details:
http://developer.android.com/guide/topics/ui/accessibility/apps.html

We'll have to test for a11y but that was already part of the plan.

Finally, these notification layouts are not that complex, not when compared to
e.g. a whole app. The layouts are not particularly dynamic, and not very deep or
huge.

+        } else {
+            notificationBuilder
+                    .setContentTitle(title)
+                    .setContentText(body)
+                    .setStyle(new NotificationCompat.BigTextStyle().bigText(body))
+                    .setLargeIcon(ensureNormalizedIcon(icon, origin))
+                    .setContentIntent(makePendingIntent(
+                            NotificationConstants.ACTION_CLICK_NOTIFICATION,
+                            persistentNotificationId, origin, tag, -1 /* actionIndex */))
+                    .setDeleteIntent(makePendingIntent(
+                            NotificationConstants.ACTION_CLOSE_NOTIFICATION,
+                            persistentNotificationId, origin, tag, -1 /* actionIndex */))
+                    .setTicker(createTickerText(title, body))
+                    .setSubText(origin);
+
+            for (int actionIndex = 0; actionIndex < actionTitles.length; actionIndex++) {
+                notificationBuilder.addAction(
+                        0 /* actionIcon */, actionTitles[actionIndex],
+                        makePendingIntent(NotificationConstants.ACTION_CLICK_NOTIFICATION,
+                                          persistentNotificationId, origin, tag, actionIndex));
+            }
+            // Site settings button is always the last action button.
+            notificationBuilder.addAction(R.drawable.settings_cog,
+                                          // TODO(johnme): Use shorter string to avoid truncation.
+                                          res.getString(R.string.page_info_site_settings_button),
+                                          pendingSettingsIntent);
         }
-        // Site settings button is always the last action button.
-        notificationBuilder.addAction(R.drawable.settings_cog,
-                                      // TODO(johnme): Use shorter string to avoid truncation.
-                                      res.getString(R.string.page_info_site_settings_button),
-                                      pendingSettingsIntent);
 
         notificationBuilder.setDefaults(makeDefaults(vibrationPattern.length, silent));
         if (vibrationPattern.length > 0) {
@@ -454,6 +466,17 @@ private void displayNotification(long persistentNotificationId, String origin, S
         mNotificationManager.notify(platformTag, PLATFORM_ID, notificationBuilder.build());
     }
 
+    private RemoteViews buildCustomLayout(String title, String body, String origin, Bitmap icon) {
+        // TODO(mvanouwerkerk): This is just a static skeleton with rough layout. Make it all real.
+        RemoteViews view = new RemoteViews(mAppContext.getPackageName(), R.layout.web_notification);
+        view.setImageViewBitmap(R.id.icon, ensureNormalizedIcon(icon, origin));
+        view.setTextViewText(R.id.title, title);
+        view.setTextViewText(R.id.time, DateFormat.getTimeFormat(mAppContext).format(new Date()));

[Peter Beverloo, 2015/10/12 14:08:30]

Hmm. One set of unit tests I can imagine for custom layouts is presentation of
the date and time. (Sorry, I realize that this is a nightmare.)

[Michael van Ouwerkerk, 2015/10/12 17:10:59]

Well, this relies heavily on android.text.format.DateFormat using trivial
arguments. It seems that anything testing the time part of the notification
deeply will risk testing DateFormat instead.

+        view.setTextViewText(R.id.body, body);
+        view.setTextViewText(R.id.origin, origin);
+        return view;
+    }
+
     /**
      * Creates the ticker text for a notification having |title| and |body|. The notification's
      * title will be printed in bold, followed by the text of the body.
@@ -510,6 +533,14 @@ public Bitmap ensureNormalizedIcon(Bitmap icon, String origin) {
         return icon;
     }
 
+    private static boolean useCustomLayouts() {
+        CommandLine commandLine = CommandLine.getInstance();
+        if (commandLine.hasSwitch(ChromeSwitches.DISABLE_NOTIFICATIONS_CUSTOM_LAYOUTS)) {
+            return false;
+        }
+        return commandLine.hasSwitch(ChromeSwitches.ENABLE_NOTIFICATIONS_CUSTOM_LAYOUTS);
+    }
+
     /**
      * Returns whether a notification has been clicked in the last 5 seconds.
      * Used for Startup.BringToForegroundReason UMA histogram.