Bug 1529380 - ContentSearch shouldn't do XHR for non-data URLs. r=r1cky

Differential Revision: https://phabricator.services.mozilla.com/D20613

--HG--
rename : browser/components/search/test/browser/testEngine.xml => browser/modules/test/browser/testEngine_chromeicon.xml
extra : moz-landing-system : lando

browser/base/content/contentSearchUI.js

@@ -81,8 +81,8 @@ ContentSearchUIController.prototype = {
       URL.revokeObjectURL(this._defaultEngine.icon);
     }
     let icon;
-    if (engine.iconBuffer) {
-      icon = this._getFaviconURIFromBuffer(engine.iconBuffer);
+    if (engine.iconData) {
+      icon = this._getFaviconURIFromIconData(engine.iconData);
     } else {
       icon = "chrome://mozapps/skin/places/defaultFavicon.svg";
     }
@@ -698,9 +698,15 @@ ContentSearchUIController.prototype = {
     return row;
   },
 
-  // Converts favicon array buffer into a data URI.
-  _getFaviconURIFromBuffer(buffer) {
-    let blob = new Blob([buffer]);
+  // If the favicon is an array buffer, convert it into a Blob URI.
+  // Otherwise just return the plain URI.
+  _getFaviconURIFromIconData(data) {
+    if (typeof(data) == "string") {
+      return data;
+    }
+
+    // If typeof(data) != "string", we assume it's an ArrayBuffer
+    let blob = new Blob([data]);
     return URL.createObjectURL(blob);
   },
 
@@ -875,8 +881,8 @@ ContentSearchUIController.prototype = {
       button.setAttribute("class", "contentSearchOneOffItem");
       let img = document.createElementNS(HTML_NS, "img");
       let uri;
-      if (engine.iconBuffer) {
-        uri = this._getFaviconURIFromBuffer(engine.iconBuffer);
+      if (engine.iconData) {
+        uri = this._getFaviconURIFromIconData(engine.iconData);
       } else {
         uri = this._getImageURIForCurrentResolution(
           "chrome://browser/skin/search-engine-placeholder.png");

browser/modules/ContentSearch.jsm

@@ -24,6 +24,8 @@ const OUTBOUND_MESSAGE = INBOUND_MESSAGE;
 const MAX_LOCAL_SUGGESTIONS = 3;
 const MAX_SUGGESTIONS = 6;
 
+const HANDOFF_TO_AWESOMEBAR_PREF = "browser.newtabpage.activity-stream.improvesearch.handoffToAwesomebar";
+
 /**
  * ContentSearch receives messages named INBOUND_MESSAGE and sends messages
  * named OUTBOUND_MESSAGE.  The data of each message is expected to look like
@@ -328,25 +330,21 @@ var ContentSearch = {
     return true;
   },
 
-  async currentStateObj(uriFlag = false) {
+  async currentStateObj() {
     let state = {
       engines: [],
       currentEngine: await this._currentEngineObj(),
     };
-    if (uriFlag) {
-      state.currentEngine.iconBuffer = Services.search.defaultEngine.getIconURLBySize(16, 16);
-    }
+
     let pref = Services.prefs.getCharPref("browser.search.hiddenOneOffs");
     let hiddenList = pref ? pref.split(",") : [];
     for (let engine of await Services.search.getVisibleEngines()) {
       let uri = engine.getIconURLBySize(16, 16);
-      let iconBuffer = uri;
-      if (!uriFlag) {
-        iconBuffer = await this._arrayBufferFromDataURI(uri);
-      }
+      let iconData = await this._maybeConvertURIToArrayBuffer(uri);
+
       state.engines.push({
         name: engine.name,
-        iconBuffer,
+        iconData,
         hidden: hiddenList.includes(engine.name),
         identifier: engine.identifier,
       });
@@ -517,15 +515,33 @@ var ContentSearch = {
     let obj = {
       name: engine.name,
       placeholder,
-      iconBuffer: await this._arrayBufferFromDataURI(favicon),
+      iconData: await this._maybeConvertURIToArrayBuffer(favicon),
     };
     return obj;
   },
 
-  _arrayBufferFromDataURI(uri) {
+  _maybeConvertURIToArrayBuffer(uri) {
     if (!uri) {
       return Promise.resolve(null);
     }
+
+    // The uri received here can be of two types
+    // 1 - resource://search-plugins/images/foo.ico
+    // 2 - data:image/x-icon;base64,VERY-LONG-STRING
+    //
+    // If the URI is not a data: URI, there's no point in converting
+    // it to an arraybuffer (which is used to optimize passing the data
+    // accross processes): we can just pass the original URI, which is cheaper.
+    //
+    // There's a catch, though: the previous UI (search one-off buttons)
+    // doesn't work with .ico files. So, if the pref for the new UI
+    // is not toggled, we skip this optimization and let all icons be
+    // sent as array buffers.
+    if (!uri.startsWith("data:") &&
+        Services.prefs.getBoolPref(HANDOFF_TO_AWESOMEBAR_PREF)) {
+      return Promise.resolve(uri);
+    }
+
     return new Promise(resolve => {
       let xhr = new XMLHttpRequest();
       xhr.open("GET", uri, true);

browser/modules/test/browser/browser.ini

@@ -13,6 +13,7 @@ support-files =
   contentSearchSuggestions.xml
   !/browser/components/search/test/browser/head.js
   !/browser/components/search/test/browser/testEngine.xml
+  testEngine_chromeicon.xml
 [browser_LiveBookmarkMigrator.js]
 [browser_PageActions.js]
 [browser_PermissionUI.js]

browser/modules/test/browser/browser_ContentSearch.js

@@ -5,6 +5,7 @@
 const TEST_MSG = "ContentSearchTest";
 const CONTENT_SEARCH_MSG = "ContentSearch";
 const TEST_CONTENT_SCRIPT_BASENAME = "contentSearch.js";
+const HANDOFF_TO_AWESOMEBAR_PREF = "browser.newtabpage.activity-stream.improvesearch.handoffToAwesomebar";
 
 /* import-globals-from ../../../components/search/test/browser/head.js */
 Services.scriptloader.loadSubScript(
@@ -13,11 +14,19 @@ Services.scriptloader.loadSubScript(
 
 var originalEngine;
 
+var arrayBufferIconTested = false;
+var plainURIIconTested = false;
+
 add_task(async function setup() {
   originalEngine = await Services.search.getDefault();
 
   await SpecialPowers.pushPrefEnv({
-    set: [["browser.newtab.preload", false]],
+    set: [
+      ["browser.newtab.preload", false],
+      // Hack: set this pref to make sure the expected
+      // optimization for non-data URIs kicks in.
+      [HANDOFF_TO_AWESOMEBAR_PREF, true],
+    ],
   });
 
   await promiseNewEngine("testEngine.xml", {
@@ -25,6 +34,10 @@ add_task(async function setup() {
     testPath: "chrome://mochitests/content/browser/browser/components/search/test/browser/",
   });
 
+  await promiseNewEngine("testEngine_chromeicon.xml", {
+    setAsCurrent: false,
+  });
+
   registerCleanupFunction(async () => {
     await Services.search.setDefault(originalEngine);
   });
@@ -40,6 +53,9 @@ add_task(async function GetState() {
     type: "State",
     data: await currentStateObj(),
   });
+
+  ok(arrayBufferIconTested, "ArrayBuffer path for the iconData was tested");
+  ok(plainURIIconTested, "Plain URI path for the iconData was tested");
 });
 
 add_task(async function SetDefaultEngine() {
@@ -157,9 +173,9 @@ add_task(async function badImage() {
   let expectedEngine =
     expectedCurrentState.engines.find(e => e.name == engine.name);
   ok(!!expectedEngine, "Sanity check: engine should be in expected state");
-  ok(expectedEngine.iconBuffer === null,
+  ok(expectedEngine.iconData === null,
      "Sanity check: icon array buffer of engine in expected state " +
-     "should be null: " + expectedEngine.iconBuffer);
+     "should be null: " + expectedEngine.iconData);
   checkMsg(finalCurrentStateMsg, {
     type: "CurrentState",
     data: expectedCurrentState,
@@ -358,7 +374,7 @@ var currentStateObj = async function() {
     let uri = engine.getIconURLBySize(16, 16);
     state.engines.push({
       name: engine.name,
-      iconBuffer: await arrayBufferFromDataURI(uri),
+      iconData: await iconDataFromURI(uri),
       hidden: false,
       identifier: engine.identifier,
     });
@@ -373,14 +389,21 @@ var defaultEngineObj = async function() {
   return {
     name: engine.name,
     placeholder: bundle.formatStringFromName("searchWithEngine", [engine.name], 1),
-    iconBuffer: await arrayBufferFromDataURI(uriFavicon),
+    iconData: await iconDataFromURI(uriFavicon),
   };
 };
 
-function arrayBufferFromDataURI(uri) {
+function iconDataFromURI(uri) {
   if (!uri) {
     return Promise.resolve(null);
   }
+
+  if (!uri.startsWith("data:") &&
+      Services.prefs.getBoolPref(HANDOFF_TO_AWESOMEBAR_PREF)) {
+    plainURIIconTested = true;
+    return Promise.resolve(uri);
+  }
+
   return new Promise(resolve => {
     let xhr = new XMLHttpRequest();
     xhr.open("GET", uri, true);
@@ -389,6 +412,7 @@ function arrayBufferFromDataURI(uri) {
       resolve(null);
     };
     xhr.onload = () => {
+      arrayBufferIconTested = true;
       resolve(xhr.response);
     };
     try {

browser/modules/test/browser/testEngine_chromeicon.xml

@@ -0,0 +1,12 @@
+<OpenSearchDescription xmlns="http://a9.com/-/spec/opensearch/1.1/"
+                       xmlns:moz="http://www.mozilla.org/2006/browser/search/">
+  <ShortName>FooChromeIcon</ShortName>
+  <Description>Foo Chrome Icon Search</Description>
+  <InputEncoding>utf-8</InputEncoding>
+  <Image width="16" height="16">chrome://browser/skin/info.svg</Image>
+  <Url type="text/html" method="GET" template="http://mochi.test:8888/browser/browser/components/search/test/browser/?search">
+    <Param name="test" value="{searchTerms}"/>
+  </Url>
+  <moz:SearchForm>http://mochi.test:8888/browser/browser/components/search/test/browser/</moz:SearchForm>
+  <moz:Alias>foochromeiconalias</moz:Alias>
+</OpenSearchDescription>