From 74b03617b47f94743e03f288dc4d5c384ae22373 Mon Sep 17 00:00:00 2001 From: Dale Harvey Date: Tue, 16 Sep 2014 12:05:41 +0200 Subject: [PATCH] Bug 1059470 - Fallback to favicon.ico. r=kgrandon Conflicts: apps/system/js/app_chrome.js --- apps/search/js/providers/places.js | 10 ++-- .../sharedtest/test/unit/icons_helper_test.js | 11 +++- apps/system/js/app_chrome.js | 32 ++++++----- apps/system/js/browser_context_menu.js | 56 +++++++++---------- shared/js/icons_helper.js | 32 +++++++++-- shared/test/unit/mocks/mock_icons_helper.js | 4 ++ 6 files changed, 92 insertions(+), 53 deletions(-) diff --git a/apps/search/js/providers/places.js b/apps/search/js/providers/places.js index 90087c1362ff..1dcd6f59d533 100644 --- 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 index 2f7bf193a6d7..8795d53845a1 100644 --- 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() { }); -}); \ No newline at end of file +}); diff --git a/apps/system/js/app_chrome.js b/apps/system/js/app_chrome.js index 06518936a28c..ec2926881e74 100644 --- 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 index 74e3db689758..7f8138997200 100644 --- 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 index e223d999359c..3a10d98ef8d2 100644 --- 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,6 +59,7 @@ 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) { @@ -46,7 +67,6 @@ 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); \ No newline at end of file +})(window); diff --git a/shared/test/unit/mocks/mock_icons_helper.js b/shared/test/unit/mocks/mock_icons_helper.js index b14b53b830cf..b66d03a383c4 100644 --- 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) { },