Bug 1400397: Do not try to reload failed highlight images. r=liuche draft
authorMichael Comella <michael.l.comella@gmail.com>
Fri, 15 Sep 2017 16:10:31 -0700
changeset 665806 ac6a26040dd0c1ccb33ebd0dc7e6a63b44d1a232
parent 665780 8cb2750225d2e0331b1cfe25e02c766dd631e565
child 731892 8cb51e674a02148337987e212aa770c516a0c5ad
push id80184
push usermichael.l.comella@gmail.com
push dateFri, 15 Sep 2017 23:43:40 +0000
reviewersliuche
bugs1400397
milestone57.0a1
Bug 1400397: Do not try to reload failed highlight images. r=liuche MozReview-Commit-ID: FnLcSfDrytS
mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamOverridablePageIconLayout.java
--- a/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
+++ b/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ -68,16 +68,17 @@ import android.widget.Button;
 import android.widget.ListView;
 import android.widget.ViewFlipper;
 
 import org.mozilla.gecko.AppConstants.Versions;
 import org.mozilla.gecko.DynamicToolbar.VisibilityTransition;
 import org.mozilla.gecko.Tabs.TabEvents;
 import org.mozilla.gecko.activitystream.ActivityStream;
 import org.mozilla.gecko.activitystream.ActivityStreamTelemetry;
+import org.mozilla.gecko.activitystream.homepanel.stream.StreamOverridablePageIconLayout;
 import org.mozilla.gecko.adjust.AdjustBrowserAppDelegate;
 import org.mozilla.gecko.animation.PropertyAnimator;
 import org.mozilla.gecko.annotation.RobocopTarget;
 import org.mozilla.gecko.bookmarks.BookmarkEditFragment;
 import org.mozilla.gecko.bookmarks.BookmarkUtils;
 import org.mozilla.gecko.bookmarks.EditBookmarkTask;
 import org.mozilla.gecko.cleanup.FileCleanupController;
 import org.mozilla.gecko.dawn.DawnHelper;
@@ -178,16 +179,17 @@ import java.lang.reflect.Method;
 import java.net.URLEncoder;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
+import java.util.Set;
 import java.util.regex.Pattern;
 
 import static org.mozilla.gecko.mma.MmaDelegate.NEW_TAB;
 
 public class BrowserApp extends GeckoApp
                         implements ActionModePresenter,
                                    AnchoredPopup.OnVisibilityChangeListener,
                                    BookmarkEditFragment.Callbacks,
@@ -344,25 +346,33 @@ public class BrowserApp extends GeckoApp
             mTelemetryCorePingDelegate,
             new OfflineTabStatusDelegate(),
             new AdjustBrowserAppDelegate(mTelemetryCorePingDelegate)
     ));
 
     @NonNull
     private SearchEngineManager mSearchEngineManager; // Contains reference to Context - DO NOT LEAK!
 
+    // Ideally, we would set this cache from the specific places it is used in Activity Stream. However, given that
+    // it's unlikely that StreamOverridablePageIconLayout will be used elsewhere and how messy it is to pass references
+    // from an object with the application lifecycle to the individual views using the cache in activity stream, we settle
+    // for storing it here and setting it on all new instances.
+    private final Set<String> mStreamIconLayoutFailedRequestCache = StreamOverridablePageIconLayout.newFailedRequestCache();
+
     private boolean mHasResumed;
 
     @Override
     public View onCreateView(final String name, final Context context, final AttributeSet attrs) {
         final View view;
         if (BrowserToolbar.class.getName().equals(name)) {
             view = BrowserToolbar.create(context, attrs);
         } else if (TabsPanel.TabsLayout.class.getName().equals(name)) {
             view = TabsPanel.createTabsLayout(context, attrs);
+        } else if (StreamOverridablePageIconLayout.class.getName().equals(name)) {
+            view = new StreamOverridablePageIconLayout(context, attrs, mStreamIconLayoutFailedRequestCache);
         } else {
             view = super.onCreateView(name, context, attrs);
         }
         return view;
     }
 
     @Override
     public void onTabChanged(Tab tab, TabEvents msg, String data) {
--- 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
@@ -19,16 +19,19 @@ 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.Collections;
+import java.util.HashSet;
+import java.util.Set;
 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.
  *
@@ -43,42 +46,79 @@ public class StreamOverridablePageIconLa
         FAVICON_IMAGE, NONFAVICON_IMAGE
     }
 
     private FaviconView faviconView;
     private ImageView imageView;
 
     private @Nullable Future<IconResponse> ongoingFaviconLoad;
 
-    public StreamOverridablePageIconLayout(final Context context, final AttributeSet attrs) {
+    /**
+     * A cache of URLs that Picasso has failed to load. Picasso will not cache which URLs it has failed to load so
+     * this is used to prevent Picasso from making additional requests to failed URLs, which is useful when the
+     * given URL does not contain an image.
+     *
+     * Picasso unfortunately does not implement this functionality: https://github.com/square/picasso/issues/475
+     *
+     * A single cache should be shared amongst all interchangeable views (e.g. in a RecyclerView) but could be
+     * shared across the app too.
+     *
+     * The consequences of not having highlight images and making requests each time the app is loaded are small,
+     * so we keep this cache in memory only.
+     */
+    private final @NonNull Set<String> nonFaviconFailedRequestURLs;
+
+    /**
+     * Create a new cache of failed requests non-favicon for use in
+     * {@link StreamOverridablePageIconLayout(Context, AttributeSet, Set)}.
+     */
+    public static Set<String> newFailedRequestCache() {
+        // To keep things simple and safe, we make this thread safe.
+        return Collections.synchronizedSet(new HashSet<String>());
+    }
+
+    /**
+     * @param nonFaviconFailedRequestCache a cache created by {@link #newFailedRequestCache()} - see that for details.
+     */
+    public StreamOverridablePageIconLayout(final Context context, final AttributeSet attrs,
+            @NonNull final Set<String> nonFaviconFailedRequestCache) {
         super(context, attrs);
+        if (nonFaviconFailedRequestCache == null) { throw new IllegalArgumentException("Expected non-null request cache"); }
+        this.nonFaviconFailedRequestURLs = nonFaviconFailedRequestCache;
+
         LayoutInflater.from(context).inflate(R.layout.activity_stream_overridable_page_icon_layout, this, true);
         initViews();
     }
 
     /**
      * Updates the icon for the view. If a non-null overrideImageURL is provided, this image will be shown.
      * Otherwise, a favicon will be retrieved for the given pageURL.
      */
     public void updateIcon(@NonNull final String pageURL, @Nullable final String overrideImageURL) {
         cancelPendingRequests();
 
         // We don't know how the large the non-favicon images could be (bug 1388415) so for now we're only going
         // to download them on wifi. Alternatively, we could get these from the Gecko cache (see below).
+        //
+        // If the Picasso request will always fail (e.g. url does not contain an image), it will make the request each
+        // time this method is called, which is each time the View is shown (or hidden and reshown): we prevent this by
+        // checking against a cache of failed request urls. If we let Picasso make the request each time, this is bad
+        // for the user's network and the replacement icon will pop-in after the timeout each time, which looks bad.
         if (NetworkUtils.isWifi(getContext()) &&
-                !TextUtils.isEmpty(overrideImageURL)) {
+                !TextUtils.isEmpty(overrideImageURL) &&
+                !nonFaviconFailedRequestURLs.contains(overrideImageURL)) {
             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, new OnErrorUsePageURLCallback(this, pageURL));
+                    .into(imageView, new OnErrorUsePageURLCallback(this, pageURL, overrideImageURL, nonFaviconFailedRequestURLs));
         } else {
             setFaviconImage(pageURL);
         }
     }
 
     private void setFaviconImage(@NonNull final String pageURL) {
         setUIMode(UIMode.FAVICON_IMAGE);
         ongoingFaviconLoad = Icons.with(getContext())
@@ -124,28 +164,39 @@ public class StreamOverridablePageIconLa
         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 final String requestURL;
+        private final Set<String> failedRequestURLs;
 
         private OnErrorUsePageURLCallback(final StreamOverridablePageIconLayout layoutWeakReference,
-                @NonNull final String pageURL) {
+                @NonNull final String pageURL,
+                @NonNull final String requestURL,
+                final Set<String> failedRequestURLs) {
             this.layoutWeakReference = new WeakReference<>(layoutWeakReference);
             this.pageURL = pageURL;
+            this.requestURL = requestURL;
+            this.failedRequestURLs = failedRequestURLs;
         }
 
         @Override
         public void onSuccess() { /* Picasso sets the image, nothing to do. */ }
 
         @Override
         public void onError() {
+            // We currently don't distinguish between URLs that do not contain an image and
+            // requests that failed for other reasons. However, these icons aren't vital
+            // so it should be fine.
+            failedRequestURLs.add(requestURL);
+
             // 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);
         }