Bug 1401733: Make StreamOverridablePageIconLayout caches static. r=liuche
Unfortunately, I can't reproduce so I'm unable to verify this fix works for
sure. However, I verified the behavior of the StreamOverridablePageIconLayout
remained the same after these changes.
As per my bug comments, using the new version of onCreateView may fix this
problem and is "more correct" than this fix but it's speculative and it's too
dangerous to make speculative fixes this close to the 57 merge. Instead, we do
this surefire fix and lose a little correctness.
The onCreateView fix moved to
bug 1401779.
MozReview-Commit-ID: DSq9jEgz6gL
--- a/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
+++ b/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ -68,17 +68,16 @@ 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;
@@ -346,33 +345,25 @@ 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
@@ -53,42 +53,29 @@ public class StreamOverridablePageIconLa
/**
* 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)}.
+ *
+ * HACK: this cache is static because it's messy to create a single instance of this cache and pass it to all
+ * relevant instances at construction time. The downside of being static is that 1) the lifecycle of the cache
+ * no longer related to the Activity and 2) *all* instances share the same cache. The original implementation
+ * fixed these problems by overriding Activity.onCreateView and passing in a single instance to the cache there,
+ * but it crashed on Android O.
*/
- public static Set<String> newFailedRequestCache() {
- // To keep things simple and safe, we make this thread safe.
- return Collections.synchronizedSet(new HashSet<String>());
- }
+ private final static Set<String> nonFaviconFailedRequestURLs = 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) {
+ public StreamOverridablePageIconLayout(final Context context, final AttributeSet attrs) {
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.
*/