Skip to content

Commit

Permalink
Bug 1400397: Do not try to reload failed highlight images. r=liuche
Browse files Browse the repository at this point in the history
MozReview-Commit-ID: FnLcSfDrytS

--HG--
extra : rebase_source : ac6a26040dd0c1ccb33ebd0dc7e6a63b44d1a232
  • Loading branch information
mcomella committed Sep 15, 2017
1 parent 05ce5f7 commit 697050b
Showing 2 changed files with 65 additions and 4 deletions.
10 changes: 10 additions & 0 deletions mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
Original file line number Diff line number Diff line change
@@ -73,6 +73,7 @@
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;
@@ -183,6 +184,7 @@
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;
@@ -349,6 +351,12 @@ public float getInterpolation(float t) {
@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
@@ -358,6 +366,8 @@ public View onCreateView(final String name, final Context context, final Attribu
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);
}
Original file line number Diff line number Diff line change
@@ -24,6 +24,9 @@
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;

/**
@@ -48,8 +51,39 @@ private enum UIMode {

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();
}
@@ -63,8 +97,14 @@ public void updateIcon(@NonNull final String pageURL, @Nullable final String ove

// 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
@@ -73,7 +113,7 @@ public void updateIcon(@NonNull final String pageURL, @Nullable final String ove
.load(Uri.parse(overrideImageURL))
.fit()
.centerCrop()
.into(imageView, new OnErrorUsePageURLCallback(this, pageURL));
.into(imageView, new OnErrorUsePageURLCallback(this, pageURL, overrideImageURL, nonFaviconFailedRequestURLs));
} else {
setFaviconImage(pageURL);
}
@@ -129,18 +169,29 @@ private void initViews() {
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

0 comments on commit 697050b

Please sign in to comment.