WebUI: Split out getFavicon method out to 2 separate methods

cr.icon.getFavicon previously took in a URL and used regex to determine
if the URL is a favicon or a site/page URL, but each caller already knows
if the URL is an icon or a page URL.

This is to ultimately be able to use this method for the WebUI tab strip
given a favicon URL and bypass the regex check since favicons out in the
web can technically be gifs, jpgs, data URLs, etc.

This CL also fixes an issue where favicon .ico/.png URLs had params added
at the end and were therefore not detected by the regex.

Bug: 989131
Change-Id: I999ccf7d75e4e2401d54e861c735487c33ee1bdf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1761327
Commit-Queue: John Lee <johntlee@chromium.org>
Reviewed-by: Dan Beam <dbeam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688740}

chrome/browser/resources/bookmarks/item.js

@@ -248,7 +248,7 @@ Polymer({
   updateFavicon_: function(url) {
     this.$.icon.className = url ? 'website-icon' : 'folder-icon';
     this.$.icon.style.backgroundImage =
-        url ? cr.icon.getFavicon(url, false) : null;
+        url ? cr.icon.getFaviconForPageURL(url, false) : null;
   },
 
   /** @private */

chrome/browser/resources/discards/discards_tab.js

@@ -295,7 +295,8 @@ Polymer({
    * @private
    */
   getFavIconStyle_: function(item) {
-    return 'background-image:' + cr.icon.getFavicon(item.tabUrl, false);
+    return 'background-image:' +
+        cr.icon.getFaviconForPageURL(item.tabUrl, false);
   },
 
   /**

chrome/browser/resources/history/history_item.js

@@ -254,7 +254,7 @@ cr.define('history', function() {
      * @private
      */
     itemChanged_: function() {
-      this.$.icon.style.backgroundImage = cr.icon.getFavicon(
+      this.$.icon.style.backgroundImage = cr.icon.getFaviconForPageURL(
           this.item.url, this.item.isUrlInRemoteUserData,
           this.item.remoteIconUrlForUma);
       this.listen(this.$['time-accessed'], 'mouseover', 'addTimeTitle_');

chrome/browser/resources/history/synced_device_card.js

@@ -115,7 +115,7 @@ Polymer({
       for (let i = 0; i < this.tabs.length; i++) {
         // Entries on this UI are coming strictly from sync, so we can set
         // |isSyncedUrlForHistoryUi| to true on the getFavicon call below.
-        icons[i].style.backgroundImage = cr.icon.getFavicon(
+        icons[i].style.backgroundImage = cr.icon.getFaviconForPageURL(
             this.tabs[i].url, true, this.tabs[i].remoteIconUrlForUma);
       }
     });

chrome/browser/resources/settings/search_engines_page/omnibox_extension_entry.html

@@ -25,7 +25,7 @@
     </style>
     <div class="list-item" focus-row-container>
       <div class="name-column">
-        <site-favicon url="[[engine.iconURL]]"></site-favicon>
+        <site-favicon favicon-url="[[engine.iconURL]]"></site-favicon>
         <span>[[engine.displayName]]</span>
       </div>
       <div class="keyword-column">[[engine.keyword]]</div>

chrome/browser/resources/settings/search_engines_page/search_engine_entry.html

@@ -41,7 +41,7 @@
 
     <div class="list-item" focus-row-container>
       <div id="name-column">
-        <site-favicon url="[[engine.iconURL]]"></site-favicon>
+        <site-favicon favicon-url="[[engine.iconURL]]"></site-favicon>
         <div>[[engine.displayName]]</div>
       </div>
       <div id="keyword-column"><div>[[engine.keyword]]</div></div>

chrome/browser/resources/settings/site_favicon.html

@@ -5,7 +5,7 @@
 <dom-module id="site-favicon">
   <template>
     <style>
-      :host {
+      #favicon {
         background-repeat: no-repeat;
         background-size: contain;
         display: block;
@@ -13,6 +13,10 @@
         width: 16px;
       }
     </style>
+    <div
+        id="favicon"
+        style="background-image: [[getBackgroundImage_(faviconUrl, url)]]">
+    </div>
   </template>
   <script src="site_favicon.js"></script>
 </dom-module>

chrome/browser/resources/settings/site_favicon.js

@@ -11,18 +11,22 @@ Polymer({
   is: 'site-favicon',
 
   properties: {
-    url: {
-      type: String,
-      value: '',
-      observer: 'urlChanged_',
-    }
+    faviconUrl: String,
+    url: String,
   },
 
   /** @private */
-  urlChanged_: function() {
-    let url = this.removePatternWildcard_(this.url);
-    url = this.ensureUrlHasScheme_(url);
-    this.style.backgroundImage = cr.icon.getFavicon(url || '', false);
+  getBackgroundImage_: function() {
+    let backgroundImage = 'none';
+    if (this.faviconUrl) {
+      const url = this.ensureUrlHasScheme_(this.faviconUrl);
+      backgroundImage = cr.icon.getFavicon(url);
+    } else if (this.url) {
+      let url = this.removePatternWildcard_(this.url);
+      url = this.ensureUrlHasScheme_(url);
+      backgroundImage = cr.icon.getFaviconForPageURL(url || '', false);
+    }
+    return backgroundImage;
   },
 
   /**

chrome/test/data/webui/icon_test.html

@@ -3,8 +3,7 @@
 <body>
 <script>
 
-// Test the case where a non-favicon URL is passed.
-function testGetFavicon_NonFavicon() {
+function testGetFaviconForPageURL() {
   var url = 'http://foo.com';
   var expectedDesktop = '-webkit-image-set(' +
       'url("chrome://favicon2/?size=16&scale_factor=1x&page_url=' +
@@ -21,11 +20,10 @@ function testGetFavicon_NonFavicon() {
 
   var isDesktop = cr.isMac || cr.isChromeOS || cr.isWindows || cr.isLinux;
   var expected = isDesktop ? expectedDesktop : expectedOther;
-  assertEquals(expected, cr.icon.getFavicon(url));
+  assertEquals(expected, cr.icon.getFaviconForPageURL(url));
 }
 
-// Test the case where the favicon URL is passed.
-function testGetFavicon_IconUrl() {
+function testGetFavicon() {
   var url = 'http://foo.com/foo.ico';
   var expectedDesktop = '-webkit-image-set(' +
       'url("chrome://favicon2/?size=16&scale_factor=1x&icon_url=' +

chrome/test/data/webui/settings/site_favicon_test.js

@@ -12,7 +12,7 @@ suite('SiteFavicon', function() {
   });
 
   function assertIconEquals(expected) {
-    const background = siteFavicon.style.backgroundImage;
+    const background = siteFavicon.$.favicon.style.backgroundImage;
     assertEquals(background, expected);
   }
 

ui/webui/resources/js/icon.js

@@ -111,17 +111,29 @@ cr.define('cr.icon', function() {
                               getUrlForCss(path);
   }
 
+  function getBaseFaviconUrl() {
+    const faviconUrl = new URL('chrome://favicon2/');
+    faviconUrl.searchParams.set('size', '16');
+    faviconUrl.searchParams.set('scale_factor', 'SCALEFACTORx');
+    return faviconUrl;
+  }
+
   /**
-   * A regular expression for identifying favicon URLs.
-   * @const {!RegExp}
+   * Creates a CSS -webkit-image-set for a favicon.
+   *
+   * @param {string} url URL of the favicon
+   * @return {string} -webkit-image-set for the favicon
    */
-  const FAVICON_URL_REGEX = /\.(ico|png)$/i;
+  /* #export */ function getFavicon(url) {
+    const faviconUrl = getBaseFaviconUrl();
+    faviconUrl.searchParams.set('icon_url', url);
+    return getImageSet(faviconUrl.toString());
+  }
 
   /**
-   * Creates a CSS -webkit-image-set for a favicon request.
+   * Creates a CSS -webkit-image-set for a favicon request based on a page URL.
    *
-   * @param {string} url Either the URL of the original page or of the favicon
-   *     itself.
+   * @param {string} url URL of the original page
    * @param {boolean} isSyncedUrlForHistoryUi Should be set to true only if the
    *     caller is an UI aimed at displaying user history, and the requested url
    *     is known to be present in Chrome sync data.
@@ -130,25 +142,18 @@ cr.define('cr.icon', function() {
    *
    * @return {string} -webkit-image-set for the favicon.
    */
-  /* #export */ function getFavicon(
+  /* #export */ function getFaviconForPageURL(
       url, isSyncedUrlForHistoryUi, remoteIconUrlForUma = '') {
     // Note: URL param keys used below must match those in the description of
     // chrome://favicon2 format in components/favicon_base/favicon_url_parser.h.
-    const faviconUrl = new URL('chrome://favicon2/');
-    faviconUrl.searchParams.set('size', '16');
-    faviconUrl.searchParams.set('scale_factor', 'SCALEFACTORx');
-
-    if (FAVICON_URL_REGEX.test(url)) {
-      faviconUrl.searchParams.set('icon_url', url);
-    } else {
-      faviconUrl.searchParams.set('page_url', url);
-      // TODO(dbeam): use the presence of 'allow_google_server_fallback' to
-      // indicate true, otherwise false.
-      const fallback = isSyncedUrlForHistoryUi ? '1' : '0';
-      faviconUrl.searchParams.set('allow_google_server_fallback', fallback);
-      if (isSyncedUrlForHistoryUi) {
-        faviconUrl.searchParams.set('icon_url', remoteIconUrlForUma);
-      }
+    const faviconUrl = getBaseFaviconUrl();
+    faviconUrl.searchParams.set('page_url', url);
+    // TODO(dbeam): use the presence of 'allow_google_server_fallback' to
+    // indicate true, otherwise false.
+    const fallback = isSyncedUrlForHistoryUi ? '1' : '0';
+    faviconUrl.searchParams.set('allow_google_server_fallback', fallback);
+    if (isSyncedUrlForHistoryUi) {
+      faviconUrl.searchParams.set('icon_url', remoteIconUrlForUma);
     }
 
     return getImageSet(faviconUrl.toString());
@@ -157,6 +162,7 @@ cr.define('cr.icon', function() {
   // #cr_define_end
   return {
     getFavicon: getFavicon,
+    getFaviconForPageURL: getFaviconForPageURL,
     getFileIconUrl: getFileIconUrl,
     getImage: getImage,
     getUrlForCss: getUrlForCss,