Introduce Animated Logo to Chrome on Android

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

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
index 2edbf6eaf404192a0a77899c15165133e9f358f4..8b6bfe967a6313911322882420ed1703d92acebc 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
@@ -10,6 +10,7 @@ import android.content.Context;
 import android.graphics.Bitmap;
 import android.graphics.Canvas;
 import android.graphics.Rect;
+import android.text.TextUtils;
 import android.view.ContextMenu;
 import android.view.LayoutInflater;
 import android.view.Menu;
@@ -55,6 +56,8 @@ import org.chromium.ui.base.PageTransition;
 
 import java.util.concurrent.TimeUnit;
 
+import jp.tomorrowkey.android.gifplayer.BaseGifImage;
+
 /**
  * Provides functionality when the user interacts with the NTP.
  */
@@ -87,6 +90,7 @@ public class NewTabPage
     private boolean mSearchProviderHasLogo;
     private final boolean mOptOutPromoShown;
     private String mOnLogoClickUrl;
+    private String mGifUrl;

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

Call this "mAnimatedLogoUrl". Yes, it's a gif, but that's not its most
descriptive aspect (it's sort of like naming a variable "integer". Yes, it's an
integer, *but* what's it used for?)

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

Done.

     private FakeboxDelegate mFakeboxDelegate;
 
     // The timestamp at which the constructor was called.
@@ -353,7 +357,17 @@ public class NewTabPage
         public void openLogoLink() {
             if (mIsDestroyed) return;
             if (mOnLogoClickUrl == null) return;

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

This check should be moved inside the if clause below. Otherwise, we'll never be
able to show animated logos if mOnLogoClickUrl is null.

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

Done.

-            mTab.loadUrl(new LoadUrlParams(mOnLogoClickUrl, PageTransition.LINK));
+
+            if (TextUtils.isEmpty(mGifUrl) || mNewTabPageView.isGifPlaying()) {

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

I'd rename this method to onLogoClicked() since it has multiple behaviors now.
I'd also add a parameter "isAnimatedLogoShowing" instead of querying
mNewTabPageView.isGifPlaying().

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

Done.

+                mTab.loadUrl(new LoadUrlParams(mOnLogoClickUrl, PageTransition.LINK));
+            } else {
+                mLogoBridge.getGif(new LogoBridge.GifListener() {
+                    @Override
+                    public void onGifDownloaded(byte[] bytes) {

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

add "if (mIsDestroyed) return;" or "assert !mIsDestroyed" is you're feeling
overly confident that this can't be called after onDestroy().

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

Done.

+                        mNewTabPageView.updateLogoGif(new BaseGifImage(bytes));
+                    }
+                }, mGifUrl);
+            }
         }
 
         @Override
@@ -364,6 +378,7 @@ public class NewTabPage
                 public void onLogoAvailable(Logo logo, boolean fromCache) {
                     if (mIsDestroyed) return;
                     mOnLogoClickUrl = logo != null ? logo.onClickUrl : null;
+                    mGifUrl = logo != null ? logo.gifUrl : null;
                     logoObserver.onLogoAvailable(logo, fromCache);
                 }
             };