Bug 1401733: Make StreamOverridablePageIconLayout caches static. r=liuche
authorMichael Comella <michael.l.comella@gmail.com>
Wed, 20 Sep 2017 16:43:25 -0700
changeset 431637 7bb527a1748fad509712d02775c2637c2e3172aa
parent 431636 cdeac7bb864103f319b04f0a588a5194d3f02c71
child 431638 22f5e644534bfc2296e5d76785ad511436456269
push id7785
push userryanvm@gmail.com
push dateThu, 21 Sep 2017 13:39:55 +0000
treeherdermozilla-beta@06d4034a8a03 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersliuche
bugs1401733, 1401779
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
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,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.
      */