Bug 931843 - Part 2: Rewrite OnFaviconLoadedListeners to hold fewer references. r=bnicholson, a=bajaj
authorRichard Newman <rnewman@mozilla.com>
Mon, 04 Nov 2013 11:48:58 -0800
changeset 166422 644c588e3e36cf653fc06a09fb20ccef148e1856
parent 166421 51b568709bb3aa287704fe8446331cbbafc15dd3
child 166423 5437dcd8200e6a7d1f55dc1bd6d7174d2e026da6
push id3066
push userakeybl@mozilla.com
push dateMon, 09 Dec 2013 19:58:46 +0000
treeherdermozilla-beta@a31a0dce83aa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbnicholson, bajaj
bugs931843
milestone27.0a2
Bug 931843 - Part 2: Rewrite OnFaviconLoadedListeners to hold fewer references. r=bnicholson, a=bajaj
mobile/android/base/BrowserApp.java
mobile/android/base/GeckoAppShell.java
mobile/android/base/Tab.java
mobile/android/base/Tabs.java
mobile/android/base/home/HomeFragment.java
mobile/android/base/home/TopSitesGridItemView.java
mobile/android/base/home/TopSitesPage.java
mobile/android/base/home/TwoLinePageRow.java
--- a/mobile/android/base/BrowserApp.java
+++ b/mobile/android/base/BrowserApp.java
@@ -725,31 +725,32 @@ abstract public class BrowserApp extends
                     Clipboard.setText(url);
                 }
             }
             return true;
         }
 
         if (itemId == R.id.add_to_launcher) {
             Tab tab = Tabs.getInstance().getSelectedTab();
-            if (tab != null) {
-                final String url = tab.getURL();
-                final String title = tab.getDisplayTitle();
-                if (url == null || title == null) {
-                    return true;
-                }
+            if (tab == null) {
+                return true;
+            }
 
-                Favicons.getFaviconForSize(url, tab.getFaviconURL(), Integer.MAX_VALUE, LoadFaviconTask.FLAG_PERSIST,
-                new OnFaviconLoadedListener() {
-                    @Override
-                    public void onFaviconLoaded(String pageUrl, String faviconURL, Bitmap favicon) {
-                        GeckoAppShell.createShortcut(title, url, url, favicon, "");
-                    }
-                });
+            final String url = tab.getURL();
+            final String title = tab.getDisplayTitle();
+            if (url == null || title == null) {
+                return true;
             }
+
+            final OnFaviconLoadedListener listener = new GeckoAppShell.CreateShortcutFaviconLoadedListener(url, title);
+            Favicons.getFaviconForSize(url,
+                                       tab.getFaviconURL(),
+                                       Integer.MAX_VALUE,
+                                       LoadFaviconTask.FLAG_PERSIST,
+                                       listener);
             return true;
         }
 
         return false;
     }
 
     @Override
     public void setAccessibilityEnabled(boolean enabled) {
@@ -1350,45 +1351,34 @@ abstract public class BrowserApp extends
     private boolean isHomePagerVisible() {
         return (mHomePager != null && mHomePager.isVisible());
     }
 
     private void openReadingList() {
         Tabs.getInstance().loadUrl(ABOUT_HOME, Tabs.LOADURL_READING_LIST);
     }
 
-    /* Favicon methods */
+    /* Favicon stuff. */
+    private static OnFaviconLoadedListener sFaviconLoadedListener = new OnFaviconLoadedListener() {
+        @Override
+        public void onFaviconLoaded(String pageUrl, String faviconURL, Bitmap favicon) {
+            // If we failed to load a favicon, we use the default favicon instead.
+            Tabs.getInstance()
+                .updateFaviconForURL(pageUrl,
+                                     (favicon == null) ? Favicons.sDefaultFavicon : favicon);
+        }
+    };
+
     private void loadFavicon(final Tab tab) {
         maybeCancelFaviconLoad(tab);
 
         final int tabFaviconSize = getResources().getDimensionPixelSize(R.dimen.browser_toolbar_favicon_size);
 
         int flags = (tab.isPrivate() || tab.getErrorType() != Tab.ErrorType.NONE) ? 0 : LoadFaviconTask.FLAG_PERSIST;
-        int id = Favicons.getFaviconForSize(tab.getURL(), tab.getFaviconURL(), tabFaviconSize, flags,
-            new OnFaviconLoadedListener() {
-                @Override
-                public void onFaviconLoaded(String pageUrl, String faviconURL, Bitmap favicon) {
-                    // If we failed to load a favicon, we use the default favicon instead.
-                    if (favicon == null) {
-                        favicon = Favicons.sDefaultFavicon;
-                    }
-
-                    // The tab might be pointing to another URL by the time the
-                    // favicon is finally loaded, in which case we simply ignore it.
-                    // See also: Bug 920331.
-                    if (!tab.getURL().equals(pageUrl)) {
-                        return;
-                    }
-
-                    tab.updateFavicon(favicon);
-                    tab.setFaviconLoadId(Favicons.NOT_LOADING);
-                    Tabs.getInstance().notifyListeners(tab, Tabs.TabEvents.FAVICON);
-                }
-            });
-
+        int id = Favicons.getFaviconForSize(tab.getURL(), tab.getFaviconURL(), tabFaviconSize, flags, sFaviconLoadedListener);
         tab.setFaviconLoadId(id);
     }
 
     private void maybeCancelFaviconLoad(Tab tab) {
         int faviconLoadId = tab.getFaviconLoadId();
 
         // Cancel pending favicon load task
         Favicons.cancelFaviconLoad(faviconLoadId);
--- a/mobile/android/base/GeckoAppShell.java
+++ b/mobile/android/base/GeckoAppShell.java
@@ -1,15 +1,16 @@
 /* -*- Mode: Java; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: nil; -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko;
 
+import org.mozilla.gecko.favicons.OnFaviconLoadedListener;
 import org.mozilla.gecko.gfx.BitmapUtils;
 import org.mozilla.gecko.gfx.GeckoLayerClient;
 import org.mozilla.gecko.gfx.GfxInfoThread;
 import org.mozilla.gecko.gfx.LayerView;
 import org.mozilla.gecko.gfx.PanZoomController;
 import org.mozilla.gecko.prompts.PromptService;
 import org.mozilla.gecko.mozglue.GeckoLoader;
 import org.mozilla.gecko.mozglue.GeneratableAndroidBridgeTarget;
@@ -251,16 +252,31 @@ public class GeckoAppShell
     public static native void scheduleResumeComposition(int width, int height);
 
     public static native float computeRenderIntegrity();
 
     public static native SurfaceBits getSurfaceBits(Surface surface);
 
     public static native void onFullScreenPluginHidden(View view);
 
+    public static class CreateShortcutFaviconLoadedListener implements OnFaviconLoadedListener {
+        private final String title;
+        private final String url;
+
+        public CreateShortcutFaviconLoadedListener(final String url, final String title) {
+            this.url = url;
+            this.title = title;
+        }
+
+        @Override
+        public void onFaviconLoaded(String pageUrl, String faviconURL, Bitmap favicon) {
+            GeckoAppShell.createShortcut(title, url, url, favicon, "");
+        }
+    }
+
     private static final class GeckoMediaScannerClient implements MediaScannerConnectionClient {
         private final String mFile;
         private final String mMimeType;
         private MediaScannerConnection mScanner;
 
         public static void startScan(Context context, String file, String mimeType) {
             new GeckoMediaScannerClient(context, file, mimeType);
         }
--- a/mobile/android/base/Tab.java
+++ b/mobile/android/base/Tab.java
@@ -354,18 +354,25 @@ public class Tab {
     public void setFaviconLoadId(int faviconLoadId) {
         mFaviconLoadId = faviconLoadId;
     }
 
     public int getFaviconLoadId() {
         return mFaviconLoadId;
     }
 
-    public void updateFavicon(Bitmap favicon) {
+    /**
+     * Returns true if the favicon changed.
+     */
+    public boolean updateFavicon(Bitmap favicon) {
+        if (mFavicon == favicon) {
+            return false;
+        }
         mFavicon = favicon;
+        return true;
     }
 
     public synchronized void updateFaviconURL(String faviconUrl, int size) {
         // If we already have an "any" sized icon, don't update the icon.
         if (mFaviconSize == -1)
             return;
 
         // Only update the favicon if it's bigger than the current favicon.
--- a/mobile/android/base/Tabs.java
+++ b/mobile/android/base/Tabs.java
@@ -1,29 +1,31 @@
 /* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko;
 
 import org.mozilla.gecko.db.BrowserDB;
+import org.mozilla.gecko.favicons.Favicons;
 import org.mozilla.gecko.home.HomePager;
 import org.mozilla.gecko.sync.setup.SyncAccounts;
 import org.mozilla.gecko.util.GeckoEventListener;
 import org.mozilla.gecko.util.ThreadUtils;
 
 import org.json.JSONObject;
 
 import android.accounts.Account;
 import android.accounts.AccountManager;
 import android.accounts.OnAccountsUpdateListener;
 import android.content.ContentResolver;
 import android.content.Context;
 import android.database.ContentObserver;
+import android.graphics.Bitmap;
 import android.graphics.Color;
 import android.net.Uri;
 import android.os.Handler;
 import android.text.TextUtils;
 import android.util.Log;
 
 import java.util.ArrayList;
 import java.util.Collections;
@@ -273,16 +275,19 @@ public class Tabs implements GeckoEventL
     }
 
     public boolean isSelectedTabId(int tabId) {
         final Tab selected = mSelectedTab;
         return selected != null && selected.getId() == tabId;
     }
 
     public synchronized Tab getTab(int id) {
+        if (id == -1)
+            return null;
+
         if (mTabs.size() == 0)
             return null;
 
         if (!mTabs.containsKey(id))
            return null;
 
         return mTabs.get(id);
     }
@@ -467,16 +472,34 @@ public class Tabs implements GeckoEventL
                 tab.setDesktopMode(message.getBoolean("desktopMode"));
                 notifyListeners(tab, TabEvents.DESKTOP_MODE_CHANGE);
             }
         } catch (Exception e) {
             Log.w(LOGTAG, "handleMessage threw for " + event, e);
         }
     }
 
+    /**
+     * Set the favicon for any tabs loaded with this page URL.
+     */
+    public void updateFaviconForURL(String pageURL, Bitmap favicon) {
+        // The tab might be pointing to another URL by the time the
+        // favicon is finally loaded, in which case we won't find the tab.
+        // See also: Bug 920331.
+        for (Tab tab : mOrder) {
+            String tabURL = tab.getURL();
+            if (pageURL.equals(tabURL)) {
+                tab.setFaviconLoadId(Favicons.NOT_LOADING);
+                if (tab.updateFavicon(favicon)) {
+                    notifyListeners(tab, TabEvents.FAVICON);
+                }
+            }
+        }
+    }
+
     public void refreshThumbnails() {
         final ThumbnailHelper helper = ThumbnailHelper.getInstance();
         ThreadUtils.postToBackgroundThread(new Runnable() {
             @Override
             public void run() {
                 for (final Tab tab : mOrder) {
                     helper.getAndProcessThumbnailFor(tab);
                 }
--- a/mobile/android/base/home/HomeFragment.java
+++ b/mobile/android/base/home/HomeFragment.java
@@ -2,17 +2,16 @@
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.home;
 
 import org.mozilla.gecko.EditBookmarkDialog;
 import org.mozilla.gecko.favicons.Favicons;
-import org.mozilla.gecko.favicons.OnFaviconLoadedListener;
 import org.mozilla.gecko.GeckoAppShell;
 import org.mozilla.gecko.GeckoEvent;
 import org.mozilla.gecko.GeckoProfile;
 import org.mozilla.gecko.R;
 import org.mozilla.gecko.ReaderModeUtils;
 import org.mozilla.gecko.Tabs;
 import org.mozilla.gecko.db.BrowserContract.Combined;
 import org.mozilla.gecko.db.BrowserDB;
@@ -129,23 +128,17 @@ abstract class HomeFragment extends Frag
 
         if (itemId == R.id.home_add_to_launcher) {
             if (info.url == null) {
                 Log.e(LOGTAG, "Can't add to home screen because URL is null");
                 return false;
             }
 
             // Fetch the largest cacheable icon size.
-            Favicons.getLargestFaviconForPage(info.url, new OnFaviconLoadedListener() {
-                @Override
-                public void onFaviconLoaded(String url, String faviconURL, Bitmap favicon) {
-                    GeckoAppShell.createShortcut(info.getDisplayTitle(), info.url, favicon, "");
-                }
-            });
-
+            Favicons.getLargestFaviconForPage(info.url, new GeckoAppShell.CreateShortcutFaviconLoadedListener(info.url, info.getDisplayTitle()));
             return true;
         }
 
         if (itemId == R.id.home_open_private_tab || itemId == R.id.home_open_new_tab) {
             if (info.url == null) {
                 Log.e(LOGTAG, "Can't open in new tab because URL is null");
                 return false;
             }
--- a/mobile/android/base/home/TopSitesGridItemView.java
+++ b/mobile/android/base/home/TopSitesGridItemView.java
@@ -28,17 +28,17 @@ public class TopSitesGridItemView extend
     private static final String LOGTAG = "GeckoTopSitesGridItemView";
 
     // Empty state, to denote there is no valid url.
     private static final int[] STATE_EMPTY = { android.R.attr.state_empty };
 
     private static final ScaleType SCALE_TYPE_FAVICON   = ScaleType.CENTER;
     private static final ScaleType SCALE_TYPE_RESOURCE  = ScaleType.CENTER;
     private static final ScaleType SCALE_TYPE_THUMBNAIL = ScaleType.CENTER_CROP;
-    
+
     // Child views.
     private final TextView mTitleView;
     private final ImageView mThumbnailView;
 
     // Data backing this view.
     private String mTitle;
     private String mUrl;
     private String mFaviconURL;
--- a/mobile/android/base/home/TopSitesPage.java
+++ b/mobile/android/base/home/TopSitesPage.java
@@ -619,17 +619,17 @@ public class TopSitesPage extends HomeFr
         }
 
         @Override
         public View newView(Context context, Cursor cursor, ViewGroup parent) {
             return new TopSitesGridItemView(context);
         }
     }
 
-    private class LoadIDAwareFaviconLoadedListener implements OnFaviconLoadedListener {
+    private static class LoadIDAwareFaviconLoadedListener implements OnFaviconLoadedListener {
         private volatile int loadId = Favicons.NOT_LOADING;
         private final TopSitesGridItemView view;
         public LoadIDAwareFaviconLoadedListener(TopSitesGridItemView view) {
             this.view = view;
         }
 
         public void setLoadId(int id) {
             this.loadId = id;
--- a/mobile/android/base/home/TwoLinePageRow.java
+++ b/mobile/android/base/home/TwoLinePageRow.java
@@ -21,36 +21,58 @@ import android.database.Cursor;
 import android.graphics.Bitmap;
 import android.text.TextUtils;
 import android.util.AttributeSet;
 import android.view.Gravity;
 import android.view.LayoutInflater;
 import android.widget.LinearLayout;
 import android.widget.TextView;
 
+import java.lang.ref.WeakReference;
+
 public class TwoLinePageRow extends LinearLayout
                             implements Tabs.OnTabsChangedListener {
     private static final int NO_ICON = 0;
 
     private final TextView mTitle;
     private final TextView mUrl;
     private final FaviconView mFavicon;
 
     private int mUrlIconId;
     private int mBookmarkIconId;
     private boolean mShowIcons;
     private int mLoadFaviconJobId = Favicons.NOT_LOADING;
 
-    // Listener for handling Favicon loads.
-    private final OnFaviconLoadedListener mFaviconListener = new OnFaviconLoadedListener() {
+    // Only holds a reference to the FaviconView itself, so if the row gets
+    // discarded while a task is outstanding, we'll leak less memory.
+    private static class UpdateViewFaviconLoadedListener implements OnFaviconLoadedListener {
+        private final WeakReference<FaviconView> view;
+        public UpdateViewFaviconLoadedListener(FaviconView view) {
+            this.view = new WeakReference<FaviconView>(view);
+        }
+
         @Override
         public void onFaviconLoaded(String url, String faviconURL, Bitmap favicon) {
-            setFaviconWithUrl(favicon, faviconURL);
+            FaviconView v = view.get();
+            if (v == null) {
+                // Guess we stuck around after the TwoLinePageRow went away.
+                return;
+            }
+
+            if (favicon == null) {
+                v.showDefaultFavicon();
+                return;
+            }
+
+            v.updateImage(favicon, url);
         }
-    };
+    }
+
+    // Listener for handling Favicon loads.
+    private final OnFaviconLoadedListener mFaviconListener;
 
     // The URL for the page corresponding to this view.
     private String mPageUrl;
 
     public TwoLinePageRow(Context context) {
         this(context, null);
     }
 
@@ -62,16 +84,17 @@ public class TwoLinePageRow extends Line
         mUrlIconId = NO_ICON;
         mBookmarkIconId = NO_ICON;
         mShowIcons = true;
 
         LayoutInflater.from(context).inflate(R.layout.two_line_page_row, this);
         mTitle = (TextView) findViewById(R.id.title);
         mUrl = (TextView) findViewById(R.id.url);
         mFavicon = (FaviconView) findViewById(R.id.favicon);
+        mFaviconListener = new UpdateViewFaviconLoadedListener(mFavicon);
     }
 
     @Override
     protected void onAttachedToWindow() {
         Tabs.registerOnTabsChangedListener(this);
     }
 
     @Override