Bug 1254663 - Fall back to generated favicons if downloaded icon is too small r?sebastian draft
authorAndrzej Hunt <ahunt@mozilla.com>
Thu, 15 Sep 2016 12:15:59 -0700
changeset 414210 02dc468f30d658d8c5636057b85c74af274617a0
parent 413803 3ca1e2e9a7ad167b9d2e1df323004a52633ec429
child 414211 4bd8ef4d32d3274dfd895b906e854208b051d0ab
push id29618
push userahunt@mozilla.com
push dateThu, 15 Sep 2016 21:56:06 +0000
reviewerssebastian
bugs1254663
milestone51.0a1
Bug 1254663 - Fall back to generated favicons if downloaded icon is too small r?sebastian The only locations where we might want to use smaller icons are in: - Tab.java: used for: -- Doorhangers / Site ID popup: we only show a tiny icon regardless. -- TabStrip (on tablets): also only a small icon. -- One exception: the media notification probably requires a larger icon, this will require followup. - SearchEnginePreference: this is used for the search engine settings screen, where icons are also smaller. MozReview-Commit-ID: KUUVKMSmE3G
mobile/android/base/java/org/mozilla/gecko/Tab.java
mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java
mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java
mobile/android/base/java/org/mozilla/gecko/icons/IconTask.java
mobile/android/base/java/org/mozilla/gecko/preferences/SearchEnginePreference.java
--- a/mobile/android/base/java/org/mozilla/gecko/Tab.java
+++ b/mobile/android/base/java/org/mozilla/gecko/Tab.java
@@ -135,17 +135,17 @@ public class Tab {
         mTitle = title == null ? "" : title;
         mSiteIdentity = new SiteIdentity();
         mHistoryIndex = -1;
         mContentType = "";
         mZoomConstraints = new ZoomConstraints(false);
         mPluginViews = new ArrayList<View>();
         mState = shouldShowProgress(url) ? STATE_LOADING : STATE_SUCCESS;
         mLoadProgress = LOAD_PROGRESS_INIT;
-        mIconRequestBuilder = Icons.with(mAppContext).pageUrl(mUrl);
+        mIconRequestBuilder = Icons.with(mAppContext).pageUrl(mUrl).permitSmallIcons(true);
 
         updateBookmark();
     }
 
     private ContentResolver getContentResolver() {
         return mAppContext.getContentResolver();
     }
 
@@ -617,17 +617,18 @@ public class Tab {
                 // We can unconditionally clear the favicon and title here: we
                 // already filtered both cases in which this was a (pseudo-)
                 // spurious location change, so we're definitely loading a new
                 // page.
                 clearFavicon();
 
                 // Start to build a new request to load a favicon.
                 mIconRequestBuilder = Icons.with(mAppContext)
-                        .pageUrl(uri);
+                        .pageUrl(uri)
+                        .permitSmallIcons(true);
 
                 // Load local static Favicons immediately
                 if (AboutPages.isBuiltinIconPage(uri)) {
                     loadFavicon();
                 }
 
                 updateTitle(null);
             }
--- a/mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java
+++ b/mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java
@@ -24,30 +24,35 @@ public class IconRequest {
     /* package-private */ String pageUrl;
     /* package-private */ boolean privileged;
     /* package-private */ TreeSet<IconDescriptor> icons;
     /* package-private */ boolean skipNetwork;
     /* package-private */ boolean backgroundThread;
     /* package-private */ boolean skipDisk;
     /* package-private */ boolean skipMemory;
     /* package-private */ int targetSize;
+    /* package-private */ boolean ignoreSmallIcons;
     /* package-private */ boolean prepareOnly;
+
     private IconCallback callback;
 
     /* package-private */ IconRequest(Context context) {
         this.context = context.getApplicationContext();
         this.icons = new TreeSet<>(new IconDescriptorComparator());
 
         // Setting some sensible defaults.
         this.privileged = false;
         this.skipMemory = false;
         this.skipDisk = false;
         this.skipNetwork = false;
         this.targetSize = context.getResources().getDimensionPixelSize(R.dimen.favicon_bg);
         this.prepareOnly = false;
+        // By default we want larger favicons, as most of the UI uses large FaviconViews. This limits
+        // the number of callsites where we need to flip this parameter.
+        this.ignoreSmallIcons = true;
     }
 
     /**
      * Execute this request and try to load an icon. Once an icon has been loaded successfully the
      * callback will be executed.
      *
      * The returned Future can be used to cancel the job.
      */
--- a/mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java
+++ b/mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java
@@ -72,16 +72,25 @@ public class IconRequestBuilder {
      */
     @CheckResult
     public IconRequestBuilder skipDisk() {
         request.skipDisk = true;
         return this;
     }
 
     /**
+     * Allow returning tiny icons, that might be less than 1/2 target size.
+     */
+    @CheckResult
+    public IconRequestBuilder permitSmallIcons(boolean permitSmallIcons) {
+        request.ignoreSmallIcons = !permitSmallIcons;
+        return this;
+    }
+
+    /**
      * Skip the memory cache and do not return a previously loaded icon.
      */
     @CheckResult
     public IconRequestBuilder skipMemory() {
         request.skipMemory = true;
         return this;
     }
 
--- a/mobile/android/base/java/org/mozilla/gecko/icons/IconTask.java
+++ b/mobile/android/base/java/org/mozilla/gecko/icons/IconTask.java
@@ -118,23 +118,36 @@ import java.util.concurrent.Callable;
 
             logPreparer(request, preparer);
         }
     }
 
     private IconResponse loadIcon(IconRequest request) throws InterruptedException {
         while (request.hasIconDescriptors()) {
             for (IconLoader loader : loaders) {
+
                 ensureNotInterrupted();
 
                 IconResponse response = loader.load(request);
 
                 logLoader(request, loader, response);
 
                 if (response != null) {
+                    // Ignore icons that are less than 2/3 the requested size: we fallback
+                    // to generated icons in this case. We want to do this here since the smaller
+                    // icon may already be in memory, in which case we can skip directly to generated
+                    // icons.
+
+                    // TODO: we should still store this smaller icon in memory / process it for those
+                    // requests where it is useful?
+                    if (request.ignoreSmallIcons &&
+                            response.getBitmap().getWidth() < 2 * request.targetSize / 3) {
+                        break;
+                    }
+
                     return response;
                 }
             }
 
             request.moveToNextIcon();
         }
 
         return generator.load(request);
--- a/mobile/android/base/java/org/mozilla/gecko/preferences/SearchEnginePreference.java
+++ b/mobile/android/base/java/org/mozilla/gecko/preferences/SearchEnginePreference.java
@@ -158,16 +158,17 @@ public class SearchEnginePreference exte
 
         final String iconURI = geckoEngineJSON.getString("iconURI");
         // Keep a reference to the bitmap - we'll need it later in onBindView.
         try {
             Icons.with(getContext())
                     .pageUrl(mIdentifier)
                     .icon(IconDescriptor.createGenericIcon(iconURI))
                     .privileged(true)
+                    .permitSmallIcons(true)
                     .build()
                     .execute(new IconCallback() {
                         @Override
                         public void onIconResponse(IconResponse response) {
                             mIconBitmap = response.getBitmap();
 
                             if (mFaviconView != null) {
                                 mFaviconView.updateAndScaleImage(response);