Bug 1400397: Do not try to reload failed highlight images. r=liuche
MozReview-Commit-ID: FnLcSfDrytS
--- 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);
}