Notifications in sensitive contexts now display origin + small icon

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

Index: chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java
index c5b0fc072f4f25b359cd9df80dd291e1933047ca..d556984617ba83388e56806fc3a1252057ea2b49 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java
@@ -7,6 +7,8 @@ package org.chromium.chrome.browser.notifications;
 import android.annotation.TargetApi;
 import android.app.Notification;
 import android.app.PendingIntent;
+import android.content.Context;
+import android.content.res.Resources;
 import android.graphics.Bitmap;
 import android.graphics.Canvas;
 import android.graphics.Color;
@@ -17,6 +19,7 @@ import android.graphics.drawable.Icon;
 import android.os.Build;
 
 import org.chromium.base.VisibleForTesting;
+import org.chromium.chrome.browser.widget.RoundedIconGenerator;
 
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -55,12 +58,22 @@ public abstract class NotificationBuilderBase {
     @VisibleForTesting
     static final int MAX_CHARSEQUENCE_LENGTH = 5 * 1024;
 
+    @VisibleForTesting
+    static final int NOTIFICATION_ICON_BG_COLOR = 0xFF969696;
+
+    @VisibleForTesting
+    static final int NOTIFICATION_TEXT_SIZE_DP = 28;

[Peter Beverloo, 2016/09/12 17:52:02]

nit: consider adding docs for these constants (I realize that there weren't any
before, but the other two constants in this class do)

[awdf, 2016/09/14 12:30:28]

Done. Also renamed to NOTIFICATION_*ICON*_TEXT_SIZE_DP since that's all it's
used for.

+
     /**
      * The maximum number of author provided action buttons. The settings button is not part of this
      * count.
      */
     private static final int MAX_AUTHOR_PROVIDED_ACTION_BUTTONS = 2;
 
+    private final int mLargeIconWidthPx;
+    private final int mLargeIconHeightPx;
+    private final RoundedIconGenerator mIconGenerator;
+
     protected CharSequence mTitle;
     protected CharSequence mBody;
     protected CharSequence mOrigin;
@@ -78,6 +91,17 @@ public abstract class NotificationBuilderBase {
     protected long mTimestamp;
     protected boolean mRenotify;
 
+    public NotificationBuilderBase(Resources resources) {
+        mLargeIconWidthPx =
+                resources.getDimensionPixelSize(android.R.dimen.notification_large_icon_width);
+        mLargeIconHeightPx =
+                resources.getDimensionPixelSize(android.R.dimen.notification_large_icon_height);
+        float density = resources.getDisplayMetrics().density;
+        int cornerRadiusPx = Math.min(mLargeIconWidthPx, mLargeIconHeightPx) / 2;
+        mIconGenerator = new RoundedIconGenerator(mLargeIconWidthPx, mLargeIconHeightPx,
+                cornerRadiusPx, NOTIFICATION_ICON_BG_COLOR, NOTIFICATION_TEXT_SIZE_DP * density);
+    }
+
     /**
      * Combines all of the options that have been set and returns a new Notification object.
      */
@@ -102,7 +126,7 @@ public abstract class NotificationBuilderBase {
     /**
      * Sets the origin text of the notification.
      */
-    public NotificationBuilderBase setOrigin(@Nullable CharSequence origin) {
+    public NotificationBuilderBase setOrigin(@Nullable String origin) {

[Peter Beverloo, 2016/09/12 17:52:02]

Is this significant? It uses a CharSequence everywhere else.

[awdf, 2016/09/14 12:30:28]

oops, no it's not significant
- Done.

         mOrigin = limitLength(origin);
         return this;
     }
@@ -238,6 +262,62 @@ public abstract class NotificationBuilderBase {
         return this;
     }
 
+    protected Bitmap getNormalizedLargeIcon() {

[Peter Beverloo, 2016/09/12 17:52:02]

nit (here and on line 292): please document non-private methods

I don't think the style guide mandates this, but consistency within a file takes
precedent.

[awdf, 2016/09/14 12:30:29]

Done.

+        return ensureNormalizedIcon(mLargeIcon, mOrigin);
+    }
+
+    /**
+     * Ensures the availability of an icon for the notification.
+     *
+     * If |icon| is a valid, non-empty Bitmap, the bitmap will be scaled to be of an appropriate
+     * size for the current Android device. Otherwise, a default icon will be created based on the
+     * origin the notification is being displayed for.
+     *
+     * @param icon The developer-provided icon they intend to use for the notification.
+     * @param origin The origin the notification is being displayed for.
+     * @return An appropriately sized icon to use for the notification.
+     */
+    @VisibleForTesting
+    public Bitmap ensureNormalizedIcon(Bitmap icon, CharSequence origin) {
+        if (icon == null || icon.getWidth() == 0) {
+            return mIconGenerator.generateIconForUrl(origin, true);
+        }
+        if (icon.getWidth() > mLargeIconWidthPx || icon.getHeight() > mLargeIconHeightPx) {
+            return icon.createScaledBitmap(
+                    icon, mLargeIconWidthPx, mLargeIconHeightPx, false /* not filtered */);
+        }
+        return icon;
+    }
+
+    protected Notification createPublicNotification(Context context) {

[Peter Beverloo, 2016/09/12 17:52:02]

Does this need an @TargetApi annotation because you're using Icon?

[awdf, 2016/09/14 12:30:29]

I don't it's necessary because it seems the linter is smart enough to figure out
the call is within a version code if-check - at least, Android Studio doesn't
give me any inspection warnings and no warnings about it are printed when I
build.. (and they are if I remove the if-check). But for consistency with the
other @TargetApi annotations in this class I could add it if you prefer?

+        // Use Android's Notification.Builder because we want the default small icon behaviour.
+        Notification.Builder builder =
+                new Notification.Builder(context)
+                        .setContentText(context.getString(
+                                org.chromium.chrome.R.string.notification_hidden_text))
+                        .setSmallIcon(org.chromium.chrome.R.drawable.ic_chrome);
+
+        if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.M) {
+            // Set origin as title on L & M, because they look odd without one.
+            builder.setContentTitle(mOrigin);
+        } else {
+            // On N(+?) 'subtext' displays at the top of the notification and this looks better.
+            builder.setSubText(mOrigin);
+        }

[Peter Beverloo, 2016/09/12 17:52:02]

It might be clearer to turn this around, because we don't know what future
Android versions will do.

if (N) { use subtext }
else { use content title }

[awdf, 2016/09/14 12:30:29]

Hm not sure how to do that because currently we target M (sdk 23) so the
constant Build.VERSION_CODES.N is not available for us to match against! Do you
know a way around this? 

[Peter Beverloo, 2016/09/14 14:06:54]

Aaah, yes, you're right. Maybe add a TODO because it'd be a nice (super low
priority) cleanup once we change our target sdk?

+
+        // Use the badge if provided and SDK supports it, else use a generated icon.
+        if (mSmallIconBitmap != null && Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
+            // The Icon class was added in Android M.
+            Bitmap publicIcon = mSmallIconBitmap.copy(mSmallIconBitmap.getConfig(), true);
+            builder.setSmallIcon(Icon.createWithBitmap(publicIcon));
+        } else if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.M) {
+            // Only set the large icon for L & M because on N(+?) it would add an extra icon on
+            // the right hand side, which looks odd without a notification title.
+            builder.setLargeIcon(mIconGenerator.generateIconForUrl(mOrigin, true));
+        }
+        return builder.build();
+    }
+
     @Nullable
     private static CharSequence limitLength(@Nullable CharSequence input) {
         if (input == null) {