Bug 1400397: Use Icons if overridePageURL fails to load. r=liuche
authorMichael Comella <michael.l.comella@gmail.com>
Fri, 15 Sep 2017 14:31:29 -0700
changeset 666687 b4a4b9fa2cfc418e6c03294c266c36e0d4a075f6
parent 666686 6602fcf1da4280b8372796ffefff315962f494f1
child 666688 73f00193147b08865fa3d049aca5214df14c6276
push id80486
push userbmo:tlin@mozilla.com
push dateTue, 19 Sep 2017 03:52:30 +0000
reviewersliuche
bugs1400397
milestone57.0a1
Bug 1400397: Use Icons if overridePageURL fails to load. r=liuche Icons apparently doesn't fade images, however, so it looks bad. Also, we try to request the image each time we bind, so scrolling up and down will create additional pop-in, which also sucks. MozReview-Commit-ID: 246pokTMFl7
mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamOverridablePageIconLayout.java
--- a/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamOverridablePageIconLayout.java
+++ b/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamOverridablePageIconLayout.java
@@ -9,24 +9,26 @@ import android.net.Uri;
 import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
 import android.text.TextUtils;
 import android.util.AttributeSet;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.widget.FrameLayout;
 import android.widget.ImageView;
+import com.squareup.picasso.Callback;
 import com.squareup.picasso.Picasso;
 import org.mozilla.gecko.R;
 import org.mozilla.gecko.icons.IconCallback;
 import org.mozilla.gecko.icons.IconResponse;
 import org.mozilla.gecko.icons.Icons;
 import org.mozilla.gecko.util.NetworkUtils;
 import org.mozilla.gecko.widget.FaviconView;
 
+import java.lang.ref.WeakReference;
 import java.util.concurrent.Future;
 
 /**
  * A layout that represents page icons in Activity Stream, which can be overridden with a custom URL.
  *
  * Under the hood, it switches between multiple icon views because favicons (in FaviconView)
  * are handled differently from other types of page images.
  *
@@ -66,27 +68,30 @@ public class StreamOverridablePageIconLa
             setUIMode(UIMode.NONFAVICON_IMAGE);
 
             // TODO (bug 1322501): Optimization: since we've already navigated to these pages, there's a chance
             // Gecko has the image in its cache: we should try to get it first before making this network request.
             Picasso.with(getContext())
                     .load(Uri.parse(overrideImageURL))
                     .fit()
                     .centerCrop()
-                    .into(imageView);
+                    .into(imageView, new OnErrorUsePageURLCallback(this, pageURL));
         } else {
-            setUIMode(UIMode.FAVICON_IMAGE);
+            setFaviconImage(pageURL);
+        }
+    }
 
-            ongoingFaviconLoad = Icons.with(getContext())
-                    .pageUrl(pageURL)
-                    .skipNetwork()
-                    .forActivityStream()
-                    .build()
-                    .execute(this);
-        }
+    private void setFaviconImage(@NonNull final String pageURL) {
+        setUIMode(UIMode.FAVICON_IMAGE);
+        ongoingFaviconLoad = Icons.with(getContext())
+                .pageUrl(pageURL)
+                .skipNetwork()
+                .forActivityStream()
+                .build()
+                .execute(this);
     }
 
     @Override
     public void onIconResponse(final IconResponse response) {
         faviconView.updateImage(response);
     }
 
     private void setUIMode(final UIMode uiMode) {
@@ -115,9 +120,34 @@ public class StreamOverridablePageIconLa
         }
     }
 
     private void initViews() {
         faviconView = (FaviconView) findViewById(R.id.favicon_view);
         imageView = (ImageView) findViewById(R.id.image_view);
         setUIMode(UIMode.FAVICON_IMAGE); // set in code to ensure state is consistent.
     }
+
+    private static class OnErrorUsePageURLCallback implements Callback {
+        private final WeakReference<StreamOverridablePageIconLayout> layoutWeakReference;
+        private final String pageURL;
+
+        private OnErrorUsePageURLCallback(final StreamOverridablePageIconLayout layoutWeakReference,
+                @NonNull final String pageURL) {
+            this.layoutWeakReference = new WeakReference<>(layoutWeakReference);
+            this.pageURL = pageURL;
+        }
+
+        @Override
+        public void onSuccess() { /* Picasso sets the image, nothing to do. */ }
+
+        @Override
+        public void onError() {
+            // I'm slightly concerned that cancelPendingRequests could get called during
+            // this Picasso -> Icons request chain and we'll get bugs where favicons don't
+            // appear correctly. However, we're already in an unexpected error case so it's
+            // probably not worth worrying about.
+            final StreamOverridablePageIconLayout layout = layoutWeakReference.get();
+            if (layout == null) { return; }
+            layout.setFaviconImage(pageURL);
+        }
+    }
 }
\ No newline at end of file