Bug 1423506 - Tippytop does not honor preference browser.chrome.site_icons. r?k88hudson draft
authorEd Lee <edilee@mozilla.com>
Fri, 08 Dec 2017 13:04:29 -0800
changeset 710196 3b5effb1e6f4b5096b7e03e909221ab3e7ffee4f
parent 710195 d9fad1593b8108e3742ecf2ef3c35bba0e83e0a0
child 710198 24ca8a87e64d9cc00984e664a1b9dc26a1a1b50f
push id92773
push userbmo:edilee@mozilla.com
push dateFri, 08 Dec 2017 21:05:38 +0000
reviewersk88hudson
bugs1423506
milestone59.0a1
Bug 1423506 - Tippytop does not honor preference browser.chrome.site_icons. r?k88hudson MozReview-Commit-ID: CKNY8tfniKq
browser/extensions/activity-stream/lib/FaviconFeed.jsm
browser/extensions/activity-stream/test/unit/lib/FaviconFeed.test.js
--- a/browser/extensions/activity-stream/lib/FaviconFeed.jsm
+++ b/browser/extensions/activity-stream/lib/FaviconFeed.jsm
@@ -5,38 +5,40 @@
 
 const {utils: Cu} = Components;
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 Cu.importGlobalProperties(["fetch", "URL"]);
 
 const {actionTypes: at} = Cu.import("resource://activity-stream/common/Actions.jsm", {});
 const {PersistentCache} = Cu.import("resource://activity-stream/lib/PersistentCache.jsm", {});
-const {Prefs} = Cu.import("resource://activity-stream/lib/ActivityStreamPrefs.jsm", {});
 const {getDomain} = Cu.import("resource://activity-stream/lib/TippyTopProvider.jsm", {});
 
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
   "resource://gre/modules/PlacesUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Services",
   "resource://gre/modules/Services.jsm");
 
 const FIVE_MINUTES = 5 * 60 * 1000;
 const ONE_DAY = 24 * 60 * 60 * 1000;
 const TIPPYTOP_UPDATE_TIME = ONE_DAY;
 const TIPPYTOP_RETRY_DELAY = FIVE_MINUTES;
 
 this.FaviconFeed = class FaviconFeed {
   constructor() {
     this.tippyTopNextUpdate = 0;
     this.cache = new PersistentCache("tippytop", true);
-    this.prefs = new Prefs();
     this._sitesByDomain = null;
     this.numRetries = 0;
   }
 
+  get endpoint() {
+    return this.store.getState().Prefs.values["tippyTop.service.endpoint"];
+  }
+
   async loadCachedData() {
     const data = await this.cache.get("sites");
     if (data && "_timestamp" in data) {
       this._sitesByDomain = data;
       this.tippyTopNextUpdate = data._timestamp + TIPPYTOP_UPDATE_TIME;
     }
   }
 
@@ -46,17 +48,17 @@ this.FaviconFeed = class FaviconFeed {
     }
   }
 
   async refresh() {
     let headers = new Headers();
     if (this._sitesByDomain && this._sitesByDomain._etag) {
       headers.set("If-None-Match", this._sitesByDomain._etag);
     }
-    let {data, etag, status} = await this.loadFromURL(this.prefs.get("tippyTop.service.endpoint"), headers);
+    let {data, etag, status} = await this.loadFromURL(this.endpoint, headers);
     let failedUpdate = false;
     if (status === 200) {
       this._sitesByDomain = this._sitesArrayToObjectByDomain(data);
       this._sitesByDomain._etag = etag;
     } else if (status !== 304) {
       failedUpdate = true;
     }
     let delay = TIPPYTOP_UPDATE_TIME;
@@ -108,16 +110,21 @@ this.FaviconFeed = class FaviconFeed {
         // If _sitesByDomain is still a Promise, no data was loaded from cache or fetch.
         this._sitesByDomain = {};
       }
       resolve(this._sitesByDomain);
     }));
   }
 
   async fetchIcon(url) {
+    // Avoid initializing and fetching icons if prefs are turned off
+    if (!this.shouldFetchIcons) {
+      return;
+    }
+
     const sitesByDomain = await this.getSitesByDomain();
     const domain = getDomain(url);
     if (domain in sitesByDomain) {
       let iconUri = Services.io.newURI(sitesByDomain[domain].image_url);
       // The #tippytop is to be able to identify them for telemetry.
       iconUri.ref = "tippytop";
       PlacesUtils.favicons.setAndFetchFaviconForPage(
         Services.io.newURI(url),
@@ -125,16 +132,23 @@ this.FaviconFeed = class FaviconFeed {
         false,
         PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE,
         null,
         Services.scriptSecurityManager.getSystemPrincipal()
       );
     }
   }
 
+  /**
+   * Determine if we should be fetching and saving icons.
+   */
+  get shouldFetchIcons() {
+    return this.endpoint && Services.prefs.getBoolPref("browser.chrome.site_icons");
+  }
+
   onAction(action) {
     switch (action.type) {
       case at.SYSTEM_TICK:
         if (this._sitesByDomain) {
           // No need to refresh if we haven't been initialized.
           this.maybeRefresh();
         }
         break;
--- a/browser/extensions/activity-stream/test/unit/lib/FaviconFeed.test.js
+++ b/browser/extensions/activity-stream/test/unit/lib/FaviconFeed.test.js
@@ -1,54 +1,54 @@
 "use strict";
-const injector = require("inject!lib/FaviconFeed.jsm");
-const {FakePrefs, GlobalOverrider} = require("test/unit/utils");
+const {FaviconFeed} = require("lib/FaviconFeed.jsm");
+const {GlobalOverrider} = require("test/unit/utils");
 const {actionTypes: at} = require("common/Actions.jsm");
 
+const FAKE_ENDPOINT = "https://foo.com/";
+
 describe("FaviconFeed", () => {
-  let FaviconFeed;
   let feed;
   let globals;
   let sandbox;
   let clock;
+  let siteIconsPref;
 
   beforeEach(() => {
-    FakePrefs.prototype.prefs["tippyTop.service.endpoint"] = "https://foo.com/";
     clock = sinon.useFakeTimers();
     globals = new GlobalOverrider();
     sandbox = globals.sandbox;
     globals.set("PlacesUtils", {
       favicons: {
         setAndFetchFaviconForPage: sandbox.spy(),
         FAVICON_LOAD_NON_PRIVATE: 1
       }
     });
+    siteIconsPref = true;
+    sandbox.stub(global.Services.prefs, "getBoolPref")
+      .withArgs("browser.chrome.site_icons").callsFake(() => siteIconsPref);
     let fetchStub = globals.sandbox.stub();
     globals.set("fetch", fetchStub);
     fetchStub.resolves({
       ok: true,
       status: 200,
       json: () => Promise.resolve([{
         "domains": ["facebook.com"],
         "image_url": "https://www.facebook.com/icon.png"
       }, {
         "domains": ["gmail.com", "mail.google.com"],
         "image_url": "https://iconserver.com/gmail.png"
       }])
     });
 
-    ({FaviconFeed} = injector({"lib/ActivityStreamPrefs.jsm": {Prefs: FakePrefs}}));
     feed = new FaviconFeed();
     feed.store = {
       dispatch: sinon.spy(),
       getState() { return this.state; },
-      state: {
-        Prefs: {},
-        TippyTop: {initialized: false, sitesByDomain: {}}
-      }
+      state: {Prefs: {values: {"tippyTop.service.endpoint": FAKE_ENDPOINT}}}
     };
   });
   afterEach(() => {
     clock.restore();
     globals.restore();
   });
 
   it("should create a TopStoriesFeed", () => {
@@ -117,17 +117,17 @@ describe("FaviconFeed", () => {
     });
   });
 
   describe("#refresh", () => {
     it("should loadFromURL with the right URL from prefs", async () => {
       feed.loadFromURL = sinon.spy(() => ({data: []}));
       await feed.refresh();
       assert.calledOnce(feed.loadFromURL);
-      assert.calledWith(feed.loadFromURL, FakePrefs.prototype.prefs["tippyTop.service.endpoint"]);
+      assert.calledWith(feed.loadFromURL, FAKE_ENDPOINT);
     });
     it("should set _sitesByDomain if new sites are returned from loadFromURL", async () => {
       const data = {
         data: [
           {"domains": ["mozilla.org"], "image_url": "https://mozilla.org/icon.png"},
           {"domains": ["facebook.com"], "image_url": "https://facebook.com/icon.png"}
         ],
         etag: "etag1234567890",
@@ -189,35 +189,62 @@ describe("FaviconFeed", () => {
       feed.loadFromURL = sinon.spy(url => ({data: [], status: 200}));
       await feed.refresh();
       assert.equal(0, feed.numRetries);
       assert.equal(24 * 60 * 60 * 1000, feed.tippyTopNextUpdate);
     });
   });
 
   describe("#fetchIcon", () => {
+    let domain;
+    let url;
+    beforeEach(() => {
+      domain = "mozilla.org";
+      url = `https://${domain}/`;
+      feed._sitesByDomain = {[domain]: {url, image_url: `${url}/icon.png`}};
+    });
+
     it("should setAndFetchFaviconForPage if the url is in the TippyTop data", async () => {
-      let url = "https://mozilla.org";
-      feed._sitesByDomain = {"mozilla.org": {url, image_url: `${url}/icon.png`}};
       await feed.fetchIcon(url);
+
       assert.calledOnce(global.PlacesUtils.favicons.setAndFetchFaviconForPage);
       assert.calledWith(global.PlacesUtils.favicons.setAndFetchFaviconForPage,
         {spec: url},
         {ref: "tippytop", spec: `${url}/icon.png`},
         false,
         global.PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE,
         null,
         undefined);
     });
+    it("should NOT setAndFetchFaviconForPage if site_icons pref is false", async () => {
+      siteIconsPref = false;
+
+      await feed.fetchIcon(url);
+
+      assert.notCalled(global.PlacesUtils.favicons.setAndFetchFaviconForPage);
+    });
+    it("should NOT setAndFetchFaviconForPage if the endpoint is empty", async () => {
+      feed.store.state.Prefs.values["tippyTop.service.endpoint"] = "";
+
+      await feed.fetchIcon(url);
+
+      assert.notCalled(global.PlacesUtils.favicons.setAndFetchFaviconForPage);
+    });
     it("should NOT setAndFetchFaviconForPage if the url is NOT in the TippyTop data", async () => {
-      let url = "https://mozilla.org";
-      feed.store.state.TippyTop.sitesByDomain["mozilla.org"] = {url, image_url: `${url}/icon.png`};
       await feed.fetchIcon("https://example.com");
+
       assert.notCalled(global.PlacesUtils.favicons.setAndFetchFaviconForPage);
     });
+    it("should cause sites to initialize with fetched sites if no sites", async () => {
+      delete feed._sitesByDomain;
+
+      await feed.fetchIcon(url);
+
+      assert.containsAllKeys(feed._sitesByDomain, ["facebook.com", "gmail.com", "mail.google.com"]);
+    });
   });
 
   describe("#onAction", () => {
     it("should maybeRefresh on SYSTEM_TICK if initialized", async () => {
       feed._sitesByDomain = {"mozilla.org": {}};
       feed.maybeRefresh = sinon.spy();
       feed.onAction({type: at.SYSTEM_TICK});
       assert.calledOnce(feed.maybeRefresh);