Bug 1059470 - Fallback to favicon.ico. r=kgrandon

Conflicts: apps/system/js/app_chrome.js
mozilla-b2g · Sep 19, 2014 · 74b0361 · 74b0361
1 parent 0c0fd39
commit 74b0361
Show file tree

Hide file tree

Showing 6 changed files with 92 additions and 53 deletions.
diff --git a/apps/search/js/providers/places.js b/apps/search/js/providers/places.js
@@ -37,13 +37,15 @@
   var iconUrls = {};
 
   function getIcon(place) {
-    var icon = IconsHelper.getBestIcon(place.icons);
-    if (icon) {
-      saveIcon(place.url, icon);
-    }
+
     if (place.url in icons && icons[place.url]) {
       return icons[place.url];
     }
+
+    IconsHelper.getIcon(place.url, null, place).then(icon => {
+      saveIcon(place.url, icon);
+    });
+
     return false;
   }
 

diff --git a/apps/sharedtest/test/unit/icons_helper_test.js b/apps/sharedtest/test/unit/icons_helper_test.js
@@ -28,6 +28,15 @@ suite('Icons Helper', function() {
     Object.defineProperty(window, 'devicePixelRatio', dprProperty);
   });
 
+  suite('Default fallback to favicon.ico', function() {
+    test('> ensure we fallback to favicon.ico', function(done) {
+      IconsHelper.getIcon('http://example.com').then(icon => {
+        assert.equal(icon, 'http://example.com/favicon.ico');
+        done();
+      });
+    });
+  });
+
   suite('No size information', function() {
     test('> Single element with no size info', function() {
       var icons = {
@@ -182,4 +191,4 @@ suite('Icons Helper', function() {
 
   });
 
-});
+});
diff --git a/apps/system/js/app_chrome.js b/apps/system/js/app_chrome.js
@@ -653,23 +653,25 @@
     var url = this._currentURL;
 
     LazyLoader.load('shared/js/icons_helper.js', (function() {
-      var activity = new MozActivity({
-        name: 'save-bookmark',
-        data: {
-          type: 'url',
-          url: url,
-          name: name,
-          icon: IconsHelper.getBestIcon(favicons),
-          useAsyncPanZoom: dataset.useAsyncPanZoom,
-          iconable: false
+      IconsHelper.getIcon(url, null, {icons: favicons}).then(icon => {
+        var activity = new MozActivity({
+          name: 'save-bookmark',
+          data: {
+            type: 'url',
+            url: url,
+            name: name,
+            icon: icon,
+            useAsyncPanZoom: dataset.useAsyncPanZoom,
+            iconable: false
+          }
+        });
+
+        if (this.addToHomeButton) {
+          activity.onsuccess = function onsuccess() {
+            this.addToHomeButton.dataset.disabled = true;
+          }.bind(this);
         }
       });
-
-      if (this.addToHomeButton) {
-        activity.onsuccess = function onsuccess() {
-          this.addToHomeButton.dataset.disabled = true;
-        }.bind(this);
-      }
     }).bind(this));
   };
 

diff --git a/apps/system/js/browser_context_menu.js b/apps/system/js/browser_context_menu.js
@@ -309,43 +309,43 @@
     return new Promise((resolve) => {
       var favicons = this.app.favicons;
       var config = this.app.config;
-      LazyLoader.load('shared/js/icons_helper.js', (function() {
+      LazyLoader.load('shared/js/icons_helper.js', (() => {
+        IconsHelper.getIcon(config.url, null, {icons: favicons}).then(icon => {
 
-        var icon = IconsHelper.getBestIcon(favicons);
-        var menuData = [];
+          var menuData = [];
 
-        menuData.push({
-          id: 'new-window',
-          label: _('new-window'),
-          callback: this.newWindow.bind(this, manifest)
-        });
+          menuData.push({
+            id: 'new-window',
+            label: _('new-window'),
+            callback: this.newWindow.bind(this, manifest)
+          });
 
-        menuData.push({
-          id: 'show-windows',
-          label: _('show-windows'),
-          callback: this.showWindows.bind(this)
-        });
+          menuData.push({
+            id: 'show-windows',
+            label: _('show-windows'),
+            callback: this.showWindows.bind(this)
+          });
+
+          BookmarksDatabase.get(config.url).then((result) => {
+            if (!result) {
+              menuData.push({
+                id: 'add-to-homescreen',
+                label: _('add-to-home-screen'),
+                callback: this.bookmarkUrl.bind(this, config.url, name, icon)
+              });
+            }
 
-        BookmarksDatabase.get(config.url).then((result) => {
-          if (!result) {
             menuData.push({
-              id: 'add-to-homescreen',
-              label: _('add-to-home-screen'),
-              callback: this.bookmarkUrl.bind(this, config.url, name, icon)
+              id: 'share',
+              label: _('share'),
+              callback: this.shareUrl.bind(this, config.url)
             });
-          }
 
-          menuData.push({
-            id: 'share',
-            label: _('share'),
-            callback: this.shareUrl.bind(this, config.url)
+            this.showMenu(menuData);
+            resolve();
           });
-
-          this.showMenu(menuData);
-          resolve();
         });
-
-      }).bind(this));
+      }));
     });
   };
 

diff --git a/shared/js/icons_helper.js b/shared/js/icons_helper.js
@@ -7,7 +7,27 @@
  */
 (function IconsHelper(exports) {
 
-  //
+  function getIcon(uri, iconSize, placeObj) {
+    var icon;
+
+    if (placeObj && placeObj.icons) {
+      icon = getBestIcon(placeObj.icons, iconSize);
+    }
+
+    // If we dont pick up a valid icon, use favicon.ico at the origin
+    if (!icon) {
+      var a = document.createElement('a');
+      a.href = uri;
+      icon = a.origin + '/favicon.ico';
+    }
+
+    // Future proofing as eventually this helper will retrieve and
+    // cache the icons, and will need an async API
+    return new Promise(resolve => {
+      resolve(icon);
+    });
+  }
+
   // See bug 1041482, we will need to support better
   // icons for different part of the system application.
   // A web page have different ways to defining icons
@@ -22,8 +42,8 @@
   // {
   //   '[uri 1]': {
   //     sizes: ['16x16 32x32 48x48', '60x60']
-  //   }, 
-  //   '[uri 2]': { 
+  //   },
+  //   '[uri 2]': {
   //     sizes: ['16x16']
   //   }
   // }
@@ -39,14 +59,14 @@
     var sizes = Object.keys(options).sort(function(a, b) {
       return a - b;
     });
+
     // Handle the case of no size info in the whole list
     // just return the first icon.
     if (sizes.length === 0) {
       var iconStrings = Object.keys(icons);
       return iconStrings.length > 0 ? iconStrings[0] : null;
     }
 
-
     var preferredSize = getPreferredSize(sizes, iconSize);
 
     return options[preferredSize];
@@ -109,9 +129,11 @@
   }
 
   exports.IconsHelper = {
+    getIcon: getIcon,
+
     getBestIcon: getBestIcon,
     // Make public for unit test purposes
     getSizes: getSizes
   };
 
-})(window);
+})(window);
diff --git a/shared/test/unit/mocks/mock_icons_helper.js b/shared/test/unit/mocks/mock_icons_helper.js
@@ -3,6 +3,10 @@
 /* exported MockIconsHelper */
 
 var MockIconsHelper = {
+  getIcon: function() {
+    return new Promise(resolve => { resolve(); });
+  },
+
   getBestIcon: function(size) {
 
   },