From 8238582d98313946ec5e3502123813cfd2bd6299 Mon Sep 17 00:00:00 2001
From: Mark Hammond <mhammond@skippinet.com.au>
Date: Mon, 22 Feb 2016 15:43:46 +1100
Subject: [PATCH] Bug 1246076 (part 1) - include the favicon in synced tabs
 records and optionally display it in the synced tabs UI. r=rnewman

---
 browser/app/profile/firefox.js              |  7 ++++++
 services/sync/modules/SyncedTabs.jsm        | 12 ++++++---
 services/sync/modules/engines/tabs.js       |  4 ++-
 services/sync/tests/unit/test_syncedtabs.js | 28 +++++++++++++++++++++
 4 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
index 37df1bd5e5a..9d79160b49d 100644
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -1318,6 +1318,7 @@ pref("services.sync.prefs.sync.security.OCSP.require", true);
 pref("services.sync.prefs.sync.security.default_personal_cert", true);
 pref("services.sync.prefs.sync.security.tls.version.min", true);
 pref("services.sync.prefs.sync.security.tls.version.max", true);
+pref("services.sync.prefs.sync.services.sync.syncedTabs.showRemoteIcons", true);
 pref("services.sync.prefs.sync.signon.rememberSignons", true);
 pref("services.sync.prefs.sync.spellchecker.dictionary", true);
 pref("services.sync.prefs.sync.xpinstall.whitelist.required", true);
@@ -1328,6 +1329,12 @@ pref("services.sync.syncedTabsUIRefresh", true);
 pref("services.sync.syncedTabsUIRefresh", false);
 #endif
 
+// A preference that controls whether we should show the icon for a remote tab.
+// This pref has no UI but exists because some people may be concerned that
+// fetching these icons to show remote tabs may leak information about that
+// user's tabs and bookmarks. Note this pref is also synced.
+pref("services.sync.syncedTabs.showRemoteIcons", true);
+
 // Developer edition preferences
 #ifdef MOZ_DEV_EDITION
 sticky_pref("lightweightThemes.selectedThemeID", "firefox-devedition@mozilla.org");
diff --git a/services/sync/modules/SyncedTabs.jsm b/services/sync/modules/SyncedTabs.jsm
index 82e3b3c945c..3eea56bc646 100644
--- a/services/sync/modules/SyncedTabs.jsm
+++ b/services/sync/modules/SyncedTabs.jsm
@@ -62,8 +62,11 @@ let SyncedTabsInternal = {
   },
 
   /* Make a "tab" record. Returns a promise */
-  _makeTab: Task.async(function* (client, tab, url) {
-    let icon = tab.icon;
+  _makeTab: Task.async(function* (client, tab, url, showRemoteIcons) {
+    let icon;
+    if (showRemoteIcons) {
+      icon = tab.icon;
+    }
     if (!icon) {
       try {
         icon = (yield PlacesUtils.promiseFaviconLinkUrl(url)).spec;
@@ -108,6 +111,9 @@ let SyncedTabsInternal = {
       return result;
     }
 
+    // A boolean that controls whether we should show the icon from the remote tab.
+    const showRemoteIcons = Preferences.get("services.sync.syncedTabs.showRemoteIcons", true);
+
     let engine = Weave.Service.engineManager.get("tabs");
 
     let seenURLs = new Set();
@@ -134,7 +140,7 @@ let SyncedTabsInternal = {
         if (!url || seenURLs.has(url)) {
           continue;
         }
-        let tabRepr = yield this._makeTab(client, tab, url);
+        let tabRepr = yield this._makeTab(client, tab, url, showRemoteIcons);
         if (filter && !this._tabMatchesFilter(tabRepr, filter)) {
           continue;
         }
diff --git a/services/sync/modules/engines/tabs.js b/services/sync/modules/engines/tabs.js
index 4447cfcd33b..d81b599f100 100644
--- a/services/sync/modules/engines/tabs.js
+++ b/services/sync/modules/engines/tabs.js
@@ -184,7 +184,9 @@ TabStore.prototype = {
         allTabs.push({
           title: current.title || "",
           urlHistory: urls,
-          icon: tabState.attributes && tabState.attributes.image || "",
+          icon: tabState.image ||
+                (tabState.attributes && tabState.attributes.image) ||
+                "",
           lastUsed: Math.floor((tabState.lastAccessed || 0) / 1000),
         });
       }
diff --git a/services/sync/tests/unit/test_syncedtabs.js b/services/sync/tests/unit/test_syncedtabs.js
index c7e74b92218..e10f71ac69b 100644
--- a/services/sync/tests/unit/test_syncedtabs.js
+++ b/services/sync/tests/unit/test_syncedtabs.js
@@ -7,6 +7,9 @@ Cu.import("resource://services-sync/main.js");
 Cu.import("resource://services-sync/SyncedTabs.jsm");
 Cu.import("resource://gre/modules/Log.jsm");
 
+const faviconService = Cc["@mozilla.org/browser/favicon-service;1"]
+                       .getService(Ci.nsIFaviconService);
+
 Log.repository.getLogger("Sync.RemoteTabs").addAppender(new Log.DumpAppender());
 
 // A mock "Tabs" engine which the SyncedTabs module will use instead of the real
@@ -79,6 +82,7 @@ add_task(function* test_clientWithTabs() {
       tabs: [
       {
         urlHistory: ["http://foo.com/"],
+        icon: "http://foo.com/favicon",
       }],
     },
     guid_mobile: {
@@ -92,10 +96,34 @@ add_task(function* test_clientWithTabs() {
   clients.sort((a, b) => { return a.name.localeCompare(b.name);});
   equal(clients[0].tabs.length, 1);
   equal(clients[0].tabs[0].url, "http://foo.com/");
+  equal(clients[0].tabs[0].icon, "http://foo.com/favicon");
   // second client has no tabs.
   equal(clients[1].tabs.length, 0);
 });
 
+add_task(function* test_clientWithTabsIconsDisabled() {
+  Services.prefs.setBoolPref("services.sync.syncedTabs.showRemoteIcons", false);
+  yield configureClients({
+    guid_desktop: {
+      clientName: "My Desktop",
+      tabs: [
+      {
+        urlHistory: ["http://foo.com/"],
+        icon: "http://foo.com/favicon",
+      }],
+    },
+  });
+
+  let clients = yield SyncedTabs.getTabClients();
+  equal(clients.length, 1);
+  clients.sort((a, b) => { return a.name.localeCompare(b.name);});
+  equal(clients[0].tabs.length, 1);
+  equal(clients[0].tabs[0].url, "http://foo.com/");
+  // expect the default favicon due to the pref being false.
+  equal(clients[0].tabs[0].icon, faviconService.defaultFavicon.spec);
+  Services.prefs.clearUserPref("services.sync.syncedTabs.showRemoteIcons");
+});
+
 add_task(function* test_filter() {
   // Nothing matches.
   yield configureClients({