feat(tippytop): Use default top icons if a site lacks a rich icon (#3890

) Fix Bug 1421903 - Allow default packaged top site rich icons to be used for non-root pages
mozilla · Dec 2, 2017 · ce92c43 · ce92c43
1 parent 6fcebac
commit ce92c43
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 26 deletions.
diff --git a/system-addon/lib/TippyTopProvider.jsm b/system-addon/lib/TippyTopProvider.jsm
@@ -10,18 +10,17 @@ const TIPPYTOP_JSON_PATH = "resource://activity-stream/data/content/tippytop/top
 const TIPPYTOP_URL_PREFIX = "resource://activity-stream/data/content/tippytop/images/";
 
 function getDomain(url) {
-  let domain = new URL(url).hostname;
+  let domain;
+  try {
+    domain = new URL(url).hostname;
+  } catch (ex) {}
   if (domain && domain.startsWith("www.")) {
     domain = domain.slice(4);
   }
   return domain;
 }
 this.getDomain = getDomain;
 
-function getPath(url) {
-  return new URL(url).pathname;
-}
-
 this.TippyTopProvider = class TippyTopProvider {
   constructor() {
     this._sitesByDomain = new Map();
@@ -43,15 +42,6 @@ this.TippyTopProvider = class TippyTopProvider {
     }
   }
   processSite(site) {
-    // Skip URLs with a path that isn't the root path /
-    let path;
-    try {
-      path = getPath(site.url);
-    } catch (e) {}
-    if (path !== "/") {
-      return site;
-    }
-
     const tippyTop = this._sitesByDomain.get(getDomain(site.url));
     if (tippyTop) {
       site.tippyTopIcon = TIPPYTOP_URL_PREFIX + tippyTop.image_url;

diff --git a/system-addon/lib/TopSitesFeed.jsm b/system-addon/lib/TopSitesFeed.jsm
@@ -165,17 +165,22 @@ this.TopSitesFeed = class TopSitesFeed {
    * Get an image for the link preferring tippy top, rich favicon, screenshots.
    */
   async _fetchIcon(link) {
-    // Check for tippy top icon and rich icon.
-    this._tippyTopProvider.processSite(link);
-    let hasTippyTop = !!link.tippyTopIcon;
-    let hasRichIcon = link.favicon && link.faviconSize >= MIN_FAVICON_SIZE;
+    // Nothing to do if we already have a rich icon from the page
+    if (link.favicon && link.faviconSize >= MIN_FAVICON_SIZE) {
+      return;
+    }
 
-    if (!hasTippyTop && !hasRichIcon) {
-      this._requestRichIcon(link.url);
+    // Nothing more to do if we can use a default tippy top icon
+    this._tippyTopProvider.processSite(link);
+    if (link.tippyTopIcon) {
+      return;
     }
 
-    // Request a screenshot if needed.
-    if (!hasTippyTop && !hasRichIcon && !link.screenshot) {
+    // Make a request for a better icon
+    this._requestRichIcon(link.url);
+
+    // Also request a screenshot if we don't have one yet
+    if (!link.screenshot) {
       const {url} = link;
       await Screenshots.maybeCacheScreenshot(link, url, "screenshot",
         screenshot => this.store.dispatch(ac.BroadcastToContent({

diff --git a/system-addon/test/unit/lib/TippyTopProvider.test.js b/system-addon/test/unit/lib/TippyTopProvider.test.js
@@ -39,10 +39,10 @@ describe("TippyTopProvider", () => {
     assert.equal(site.tippyTopIcon, "resource://activity-stream/data/content/tippytop/images/facebook-com.png");
     assert.equal(site.backgroundColor, "#3b5998");
   });
-  it("should not provide an icon for facebook.com/foobar", () => {
+  it("should provide an icon for facebook.com/foobar", () => {
     const site = instance.processSite({url: "https://facebook.com/foobar"});
-    assert.isUndefined(site.tippyTopIcon);
-    assert.isUndefined(site.backgroundColor);
+    assert.equal(site.tippyTopIcon, "resource://activity-stream/data/content/tippytop/images/facebook-com.png");
+    assert.equal(site.backgroundColor, "#3b5998");
   });
   it("should provide an icon for gmail.com", () => {
     const site = instance.processSite({url: "https://gmail.com"});
@@ -66,6 +66,6 @@ describe("TippyTopProvider", () => {
     fetchStub.rejects("whaaaa");
     instance = new TippyTopProvider();
     await instance.init();
-    instance.processSite("https://facebook.com");
+    instance.processSite({url: "https://facebook.com"});
   });
 });
diff --git a/system-addon/test/unit/lib/TopSitesFeed.test.js b/system-addon/test/unit/lib/TopSitesFeed.test.js
@@ -488,6 +488,20 @@ describe("Top Sites Feed", () => {
       assert.notProperty(link, "screenshot");
       assert.notCalled(fakeScreenshot.getScreenshotForURL);
     });
+    it("should use the link's rich icon even if there's a tippy top", () => {
+      feed._tippyTopProvider.processSite = site => {
+        site.tippyTopIcon = "icon.png";
+        site.backgroundColor = "#fff";
+        return site;
+      };
+      const link = {
+        url: "foo.com",
+        favicon: "data:foo",
+        faviconSize: 196
+      };
+      feed._fetchIcon(link);
+      assert.notProperty(link, "tippyTopIcon");
+    });
   });
   describe("#onAction", () => {
     it("should refresh on SYSTEM_TICK", async () => {