Introduce Animated Logo to Chrome on Android

chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java
index 94a7f3df7f0e7521d7ddb30dcf304a1a0a6a6cda..d64a4da58ebf5fc11bedf7409941db1899cd4bc1 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java
@@ -8,15 +8,18 @@ import android.animation.Animator;
 import android.animation.ObjectAnimator;
 import android.content.Context;
 import android.graphics.Bitmap;
+import android.graphics.Bitmap.Config;
 import android.graphics.BitmapFactory;
 import android.graphics.Canvas;
 import android.graphics.Matrix;
 import android.graphics.Paint;
+import android.graphics.drawable.Drawable;
 import android.text.TextUtils;
 import android.util.AttributeSet;
 import android.util.Property;
 import android.view.View;
 import android.view.View.OnClickListener;
+import android.widget.ImageView;
 
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ntp.LogoBridge.Logo;
@@ -24,6 +27,9 @@ import org.chromium.chrome.browser.ntp.NewTabPageView.NewTabPageManager;
 
 import java.lang.ref.WeakReference;
 
+import jp.tomorrowkey.android.gifplayer.BaseGifDrawable;
+import jp.tomorrowkey.android.gifplayer.BaseGifImage;
+
 /**
  * This view shows the default search provider's logo and fades in a new logo if one becomes
  * available.

[newt (away), 2015/09/18 20:46:02]

update javadoc

[Ian Wen, 2015/09/22 21:39:05]

Done.

@@ -36,14 +42,20 @@ public class LogoView extends View implements OnClickListener {
     // The default logo is shared across all NTPs.
     private static WeakReference<Bitmap> sDefaultLogo;
 
-    private Paint mPaint;
+    // mLogo and mNewLogo are remembered for cross fading animation.
     private Bitmap mLogo;

[newt (away), 2015/09/18 20:46:02]

Be sure to set mLogo and mNewLogo to null once the animated gif is shown to
avoid holding on to unneeded image bytes

[Ian Wen, 2015/09/22 21:39:05]

Done.

     private Bitmap mNewLogo;
+    private ObjectAnimator mAnimation;
+
+    private BaseGifDrawable mGifDrawable;
+    private Paint mPaint;
     private Matrix mLogoMatrix;
     private Matrix mNewLogoMatrix;
     private boolean mLogoIsDefault;
     private boolean mNewLogoIsDefault;
-    private ObjectAnimator mAnimation;
+
+    // The Logo object representing the latest logo this view is showing.
+    private Logo mCurrentLogo;
 
     /**
      * A measure from 0 to 1 of how much the new logo has faded in. 0 shows the old logo, 1 shows
@@ -114,6 +126,9 @@ public class LogoView extends View implements OnClickListener {
      * @param logo The new logo to fade in. May be null to reset to the default logo.
      */
     public void updateLogo(Logo logo) {
+        if (mCurrentLogo != null && mCurrentLogo.equals(logo)) return;

[newt (away), 2015/09/18 20:46:02]

Why is this needed? To prevent the non-animated logo from fading in over the
animated logo if logo revalidation happens after the animated doodle is already
playing?

If so, it would be nice to push this logic down towards the LogoBridge or the
LogoTracker. Also, this might indicate a bug if we're decoding the logo image
twice.

[Ian Wen, 2015/09/22 21:39:05]

Talked offline. This is trying to fix an imaginary issue that only happens with
my fake server. Reverted this line because double-fading-in is unlikely to
happen for real doodle server.

+        mCurrentLogo = logo;
+
         if (logo == null) {
             updateLogo(getDefaultLogo(), null, true);
         } else {
@@ -165,6 +180,23 @@ public class LogoView extends View implements OnClickListener {
     }
 
     /**
+     * Updates the GIF contained in this View and starts playing it.
+     */
+    void updateGif(BaseGifImage gifImage) {

[newt (away), 2015/09/18 20:46:02]

Make this public as with the method below. Let's try to keep this class's API
consistent, and it's currently a public API. (You could reasonably change the
class's visibility to package private, but technically, Android wants all View
subclasses to be public so they can be instantiated via reflection)

I'd probably move these two methods above the private updateLogo() method to
keep all the public methods together.

[Ian Wen, 2015/09/22 21:39:05]

Done.

+        mGifDrawable = new BaseGifDrawable(gifImage, Config.ARGB_8888);
+        // Set callback here to ensure #invalidateDrawable() is called.
+        mGifDrawable.setCallback(this);
+        mGifDrawable.start();
+    }
+
+    /**
+     * @return Whether the GIF animation is playing and the first frame is ready.
+     */
+    boolean isGifPlaying() {

[newt (away), 2015/09/18 20:46:02]

What does this return after the gif stops animating? I would guess that
mGifDrawable.isRunning() returns false at that point, so this would also return
false... but that would cause broken behavior: onDraw() wouldn't draw the
animated gif, and NewTabPageManager.openLogoLink() would do the wrong thing.

Instead, I think this method should be called "isAnimatedLogoShowing()", and
should always return true after the animated logo starts playing.

[Ian Wen, 2015/09/22 21:39:05]

BaseGifDrawable will always be running unless we call drawable#stop(). However I
still think your naming makes more sense. Done.

+        return mGifDrawable == null ? false : mGifDrawable.isRunning() && mGifDrawable.isValid();

[newt (away), 2015/09/18 20:46:02]

Simpler: "return mGifDrawable != null && mGifDrawable.isRunning() &&
mGifDrawable.isValid();"

[Ian Wen, 2015/09/22 21:39:06]

Done.

+    }
+
+    /**
      * @return Whether a new logo is currently fading in over the old logo.
      */
     private boolean isTransitioning() {
@@ -194,6 +226,11 @@ public class LogoView extends View implements OnClickListener {
         matrix.postTranslate(imageOffsetX, imageOffsetY);
     }
 
+    @Override
+    protected boolean verifyDrawable(Drawable who) {

[newt (away), 2015/09/18 20:46:01]

Move this down next to invalidateDrawable(). In general, keep related overrides
together.

[Ian Wen, 2015/09/22 21:39:05]

Done.

+        return (who == mGifDrawable) || super.verifyDrawable(who);
+    }
+
     /**
      * @return The default logo.
      */
@@ -206,22 +243,39 @@ public class LogoView extends View implements OnClickListener {
         return defaultLogo;
     }
 
+    /**
+     * @see ImageView#invalidateDrawable(Drawable)

[newt (away), 2015/09/18 20:46:02]

Remove this javadoc comment and instead add an implementation comment inside the
method. I'd briefly explain why we need to call invalidate() and refer to
ImageView#invalidateDrawable(Drawable) for more information.

[Ian Wen, 2015/09/22 21:39:05]

Done.

+     */
+    @Override
+    public void invalidateDrawable(Drawable drawable) {
+        if (drawable == mGifDrawable) invalidate();
+        else super.invalidateDrawable(drawable);
+    }
+
     @Override
     protected void onDraw(Canvas canvas) {
-        if (mLogo != null && mTransitionAmount < 0.5f) {
-            mPaint.setAlpha((int) (255 * 2 * (0.5f - mTransitionAmount)));
+        if (isGifPlaying()) {
+            // Gif drawable is present, draw animation instead.

[newt (away), 2015/09/18 20:46:02]

At this point we should set mLogo and mNewLogo to null and cancel mAnimation if
it happens to be running. In the normal case, mLogo will be the special logo,
and mNewLogo and mAnimation will be null, but this depends on code outside this
class, so I'd recommend handling the case where an animation is running rather
than assuming that it can't happen.

  if (mAnimation != null) mAnimation.cancel();
  if (mLogo != null) mLogo = null;

[Ian Wen, 2015/09/22 21:39:05]

Done.

             canvas.save();
             canvas.concat(mLogoMatrix);

[newt (away), 2015/09/18 20:46:01]

Rather than piggy backing on mLogoMatrix, I'd use a separate Matrix for the
animated logo. This would allow us to set mLogo to null and still get matrix
updates when the user rotates their device (see onSizeChanged() below). This
also makes the animated logo a bit more independent, so that if it happens to
have a different size than the custom logo (which shouldn't happen, but might),
the animated logo will still be displayed centered and sized correctly.

[Ian Wen, 2015/09/22 21:39:05]

Done.

-            canvas.drawBitmap(mLogo, 0, 0, mPaint);
+            mGifDrawable.draw(canvas);
             canvas.restore();
-        }
+        } else {
+            if (mLogo != null && mTransitionAmount < 0.5f) {
+                mPaint.setAlpha((int) (255 * 2 * (0.5f - mTransitionAmount)));
+                canvas.save();
+                canvas.concat(mLogoMatrix);
+                canvas.drawBitmap(mLogo, 0, 0, mPaint);
+                canvas.restore();
+            }
 
-        if (mNewLogo != null && mTransitionAmount > 0.5f) {
-            mPaint.setAlpha((int) (255 * 2 * (mTransitionAmount - 0.5f)));
-            canvas.save();
-            canvas.concat(mNewLogoMatrix);
-            canvas.drawBitmap(mNewLogo, 0, 0, mPaint);
-            canvas.restore();
+            if (mNewLogo != null && mTransitionAmount > 0.5f) {
+                mPaint.setAlpha((int) (255 * 2 * (mTransitionAmount - 0.5f)));
+                canvas.save();
+                canvas.concat(mNewLogoMatrix);
+                canvas.drawBitmap(mNewLogo, 0, 0, mPaint);
+                canvas.restore();
+            }
         }
     }