Bug 493051: avoid having addEngine select the engine by default, by adding an optional callback to let callers observe the successful addition of the engine, r=MattN

--HG--
extra : transplant_source : M%40%CB%E5%8F%D9RMc%BA%23%20%C7%EC%9A%84H%DB%1B%81

browser/components/search/content/search.xml

@@ -488,17 +488,22 @@
     <handlers>
       <handler event="command"><![CDATA[
         const target = event.originalTarget;
-        if (target.classList.contains("addengine-item")) {
+        if (target.engine) {
+          this.currentEngine = target.engine;
+        } else if (target.classList.contains("addengine-item")) {
           var searchService =
             Components.classes["@mozilla.org/browser/search-service;1"]
                       .getService(Components.interfaces.nsIBrowserSearchService);
           // We only detect OpenSearch files
           var type = Components.interfaces.nsISearchEngine.DATA_XML;
+          // Select the installed engine if the installation succeeds
+          var installCallback = {
+            onSuccess: engine => this.currentEngine = engine
+          }
           searchService.addEngine(target.getAttribute("uri"), type,
-                                  target.getAttribute("src"), false);
+                                  target.getAttribute("src"), false,
+                                  installCallback);
         }
-        else if (target.engine)
-          this.currentEngine = target.engine;
         else
           return;
 

browser/components/search/test/browser_426329.js

@@ -30,8 +30,7 @@ function test() {
       case "engine-added":
         var engine = ss.getEngineByName("Bug 426329");
         ok(engine, "Engine was added.");
-        //XXX Bug 493051
-        //ss.currentEngine = engine;
+        ss.currentEngine = engine;
         break;
       case "engine-current":
         ok(ss.currentEngine.name == "Bug 426329", "currentEngine set");

browser/components/search/test/browser_contextmenu.js

@@ -16,8 +16,7 @@ function test() {
       case "engine-added":
         var engine = ss.getEngineByName(ENGINE_NAME);
         ok(engine, "Engine was added.");
-        //XXX Bug 493051
-        //ss.currentEngine = engine;
+        ss.currentEngine = engine;
         break;
       case "engine-current":
         is(ss.currentEngine.name, ENGINE_NAME, "currentEngine set");

browser/components/search/test/browser_private_search_perwindowpb.js

@@ -42,20 +42,18 @@ function test() {
   }
 
   function addEngine(aCallback) {
-    function observer(aSub, aTopic, aData) {
-      switch (aData) {
-        case "engine-current":
-          ok(Services.search.currentEngine.name == "Bug 426329",
-             "currentEngine set");
-          aCallback();
-          break;
+    let installCallback = {
+      onSuccess: function (engine) {
+        Services.search.currentEngine = engine;
+        aCallback();
+      },
+      onError: function (errorCode) {
+        ok(false, "failed to install engine: " + errorCode);
       }
-    }
-
-    Services.obs.addObserver(observer, "browser-search-engine-modified", false);
-    Services.search.addEngine(
-      engineURL + "426329.xml", Ci.nsISearchEngine.DATA_XML,
-      "data:image/x-icon,%00", false);
+    };
+    Services.search.addEngine(engineURL + "426329.xml",
+                              Ci.nsISearchEngine.DATA_XML,
+                              "data:image/x-icon,%00", false, installCallback);
   }
 
   function testOnWindow(aIsPrivate, aCallback) {

netwerk/base/public/nsIBrowserSearchService.idl

@@ -140,6 +140,30 @@ interface nsISearchEngine : nsISupports
 
 };
 
+[scriptable, uuid(9fc39136-f08b-46d3-b232-96f4b7b0e235)]
+interface nsISearchInstallCallback : nsISupports
+{
+  const unsigned long ERROR_UNKNOWN_FAILURE = 0x1;
+  const unsigned long ERROR_DUPLICATE_ENGINE = 0x2;
+
+  /**
+   * Called to indicate that the engine addition process succeeded.
+   *
+   * @param engine
+   *        The nsISearchEngine object that was added (will not be null).
+   */
+  void onSuccess(in nsISearchEngine engine);
+
+  /**
+   * Called to indicate that the engine addition process failed.
+   *
+   * @param errorCode
+   *        One of the ERROR_* values described above indicating the cause of
+   *        the failure.
+   */
+  void onError(in unsigned long errorCode);
+};
+
 /**
  * Callback for asynchronous initialization of nsIBrowserSearchService
  */
@@ -186,8 +210,7 @@ interface nsIBrowserSearchService : nsISupports
    * Adds a new search engine from the file at the supplied URI, optionally
    * asking the user for confirmation first.  If a confirmation dialog is
    * shown, it will offer the option to begin using the newly added engine
-   * right away; if no confirmation dialog is shown, the new engine will be
-   * used right away automatically.
+   * right away.
    *
    * @param engineURL
    *        The URL to the search engine's description file.
@@ -207,11 +230,16 @@ interface nsIBrowserSearchService : nsISupports
    *        value is false, the engine will be added to the list upon successful
    *        load, but it will not be selected as the current engine.
    *
+   * @param callback
+   *        A nsISearchInstallCallback that will be notified when the
+   *        addition is complete, or if the addition fails. It will not be
+   *        called if addEngine throws an exception.
+   *
    * @throws NS_ERROR_FAILURE if the type is invalid, or if the description
    *         file cannot be successfully loaded.
    */
   void addEngine(in AString engineURL, in long dataType, in AString iconURL,
-                 in boolean confirm);
+                 in boolean confirm, [optional] in nsISearchInstallCallback callback);
 
   /**
    * Adds a new search engine, without asking the user for confirmation and

toolkit/components/search/nsSearchService.js

@@ -5,6 +5,7 @@
 const Ci = Components.interfaces;
 const Cc = Components.classes;
 const Cr = Components.results;
+const Cu = Components.utils;
 
 Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
 Components.utils.import("resource://gre/modules/Services.jsm");
@@ -1150,7 +1151,10 @@ Engine.prototype = {
   _confirm: false,
   // Whether to set this as the current engine as soon as it is loaded.  This
   // is only used when the engine is first added to the list.
-  _useNow: true,
+  _useNow: false,
+  // A function to be invoked when this engine object's addition completes (or
+  // fails). Only used for installation via addEngine.
+  _installCallback: null,
   // Where the engine was loaded from. Can be one of: SEARCH_APP_DIR,
   // SEARCH_PROFILE_DIR, SEARCH_IN_EXTENSION.
   __installLocation: null,
@@ -1304,10 +1308,19 @@ Engine.prototype = {
    */
   _onLoad: function SRCH_ENG_onLoad(aBytes, aEngine) {
     /**
-     * Handle an error during the load of an engine by prompting the user to
-     * notify him that the load failed.
+     * Handle an error during the load of an engine by notifying the engine's
+     * error callback, if any.
      */
-    function onError(aErrorString, aTitleString) {
+    function onError(errorCode = Ci.nsISearchInstallCallback.ERROR_UNKNOWN_FAILURE) {
+      // Notify the callback of the failure
+      if (aEngine._installCallback) {
+        aEngine._installCallback(errorCode);
+      }
+    }
+
+    function promptError(strings = {}, error = undefined) {
+      onError(error);
+
       if (aEngine._engineToUpdate) {
         // We're in an update, so just fail quietly
         LOG("updating " + aEngine._engineToUpdate.name + " failed");
@@ -1317,8 +1330,8 @@ Engine.prototype = {
       var brandName = brandBundle.GetStringFromName("brandShortName");
 
       var searchBundle = Services.strings.createBundle(SEARCH_BUNDLE);
-      var msgStringName = aErrorString || "error_loading_engine_msg2";
-      var titleStringName = aTitleString || "error_loading_engine_title";
+      var msgStringName = strings.error || "error_loading_engine_msg2";
+      var titleStringName = strings.title || "error_loading_engine_title";
       var title = searchBundle.GetStringFromName(titleStringName);
       var text = searchBundle.formatStringFromName(msgStringName,
                                                    [brandName, aEngine._location],
@@ -1328,7 +1341,7 @@ Engine.prototype = {
     }
 
     if (!aBytes) {
-      onError();
+      promptError();
       return;
     }
 
@@ -1351,7 +1364,7 @@ Engine.prototype = {
         aEngine._data = aBytes;
         break;
       default:
-        onError();
+        promptError();
         LOG("_onLoad: Bogus engine _dataType: \"" + this._dataType + "\"");
         return;
     }
@@ -1362,7 +1375,7 @@ Engine.prototype = {
     } catch (ex) {
       LOG("_onLoad: Failed to init engine!\n" + ex);
       // Report an error to the user
-      onError();
+      promptError();
       return;
     }
 
@@ -1371,9 +1384,9 @@ Engine.prototype = {
     // otherwise we fail silently.
     if (!engineToUpdate) {
       if (Services.search.getEngineByName(aEngine.name)) {
-        if (aEngine._confirm)
-          onError("error_duplicate_engine_msg", "error_invalid_engine_title");
-
+        promptError({ error: "error_duplicate_engine_msg",
+                      title: "error_invalid_engine_title"
+                    }, Ci.nsISearchInstallCallback.ERROR_DUPLICATE_ENGINE);
         LOG("_onLoad: duplicate engine found, bailing");
         return;
       }
@@ -1386,8 +1399,10 @@ Engine.prototype = {
       var confirmation = aEngine._confirmAddEngine();
       LOG("_onLoad: confirm is " + confirmation.confirmed +
           "; useNow is " + confirmation.useNow);
-      if (!confirmation.confirmed)
+      if (!confirmation.confirmed) {
+        onError();
         return;
+      }
       aEngine._useNow = confirmation.useNow;
     }
 
@@ -1414,6 +1429,7 @@ Engine.prototype = {
           if (!newSelfURL || !newSelfURL._hasRelation("self")) {
             LOG("_onLoad: updateURL missing in updated engine for " +
                 aEngine.name + " aborted");
+            onError();
             return;
           }
           newUpdateURL = newSelfURL.template;
@@ -1421,6 +1437,7 @@ Engine.prototype = {
 
         if (oldUpdateURL != newUpdateURL) {
           LOG("_onLoad: updateURLs do not match! Update of " + aEngine.name + " aborted");
+          onError();
           return;
         }
       }
@@ -1428,10 +1445,6 @@ Engine.prototype = {
       // Set the new engine's icon, if it doesn't yet have one.
       if (!aEngine._iconURI && engineToUpdate._iconURI)
         aEngine._iconURI = engineToUpdate._iconURI;
-
-      // Clear the "use now" flag since we don't want to be changing the
-      // current engine for an update.
-      aEngine._useNow = false;
     }
 
     // Write the engine to file. For readOnly engines, they'll be stored in the
@@ -1442,6 +1455,11 @@ Engine.prototype = {
     // Notify the search service of the successful load. It will deal with
     // updates by checking aEngine._engineToUpdate.
     notifyAction(aEngine, SEARCH_ENGINE_LOADED);
+
+    // Notify the callback if needed
+    if (aEngine._installCallback) {
+      aEngine._installCallback();
+    }
   },
 
   /**
@@ -3341,14 +3359,31 @@ SearchService.prototype = {
   },
 
   addEngine: function SRCH_SVC_addEngine(aEngineURL, aDataType, aIconURL,
-                                         aConfirm) {
+                                         aConfirm, aCallback) {
     LOG("addEngine: Adding \"" + aEngineURL + "\".");
     this._ensureInitialized();
     try {
       var uri = makeURI(aEngineURL);
       var engine = new Engine(uri, aDataType, false);
+      if (aCallback) {
+        engine._installCallback = function (errorCode) {
+          try {
+            if (errorCode == null)
+              aCallback.onSuccess(engine);
+            else
+              aCallback.onError(errorCode);
+          } catch (ex) {
+            Cu.reportError("Error invoking addEngine install callback: " + ex);
+          }
+          // Clear the reference to the callback now that it's been invoked.
+          engine._installCallback = null;
+        };
+      }
       engine._initFromURI();
     } catch (ex) {
+      // Drop the reference to the callback, if set
+      if (engine)
+        engine._installCallback = null;
       FAIL("addEngine: Error adding engine:\n" + ex, Cr.NS_ERROR_FAILURE);
     }
     engine._setIcon(aIconURL, false);

toolkit/components/search/tests/xpcshell/head_search.js

@@ -97,7 +97,7 @@ function afterCache(callback)
   Services.obs.addObserver(obs, "browser-search-service", false);
 }
 
-function  parseJsonFromStream(aInputStream) {
+function parseJsonFromStream(aInputStream) {
   const json = Cc["@mozilla.org/dom/json;1"].createInstance(Components.interfaces.nsIJSON);
   const data = json.decodeFromStream(aInputStream, aInputStream.available());
   return data;

toolkit/components/search/tests/xpcshell/test_addEngine_callback.js

@@ -0,0 +1,89 @@
+/* Any copyright is dedicated to the Public Domain.
+ *    http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/*
+ * Tests covering nsIBrowserSearchService::addEngine's optional callback.
+ */
+
+"use strict";
+
+const Ci = Components.interfaces;
+
+Components.utils.import("resource://testing-common/httpd.js");
+
+// First test inits the search service
+add_test(function init_search_service() {
+  Services.search.init(function (status) {
+    if (!Components.isSuccessCode(status))
+      do_throw("Failed to initialize search service");
+
+    run_next_test();
+  });
+});
+
+// Simple test of the search callback
+add_test(function simple_callback_test() {
+  let searchCallback = {
+    onSuccess: function (engine) {
+      do_check_true(!!engine);
+      do_check_neq(engine.name, Services.search.defaultEngine.name);
+      run_next_test();
+    },
+    onError: function (errorCode) {
+      do_throw("search callback returned error: " + errorCode);
+    }
+  }
+  Services.search.addEngine("http://localhost:4444/data/engine.xml",
+                            Ci.nsISearchEngine.DATA_XML,
+                            null, false, searchCallback);
+});
+
+// Test of the search callback on duplicate engine failures
+add_test(function duplicate_failure_test() {
+  let searchCallback = {
+    onSuccess: function (engine) {
+      do_throw("this addition should not have succeeded");
+    },
+    onError: function (errorCode) {
+      do_check_true(!!errorCode);
+      do_check_eq(errorCode, Ci.nsISearchInstallCallback.ERROR_DUPLICATE_ENGINE);
+      run_next_test();
+    }
+  }
+  // Re-add the same engine added in the previous test
+  Services.search.addEngine("http://localhost:4444/data/engine.xml",
+                            Ci.nsISearchEngine.DATA_XML,
+                            null, false, searchCallback);
+});
+
+// Test of the search callback on failure to load the engine failures
+add_test(function load_failure_test() {
+  let searchCallback = {
+    onSuccess: function (engine) {
+      do_throw("this addition should not have succeeded");
+    },
+    onError: function (errorCode) {
+      do_check_true(!!errorCode);
+      do_check_eq(errorCode, Ci.nsISearchInstallCallback.ERROR_UNKNOWN_FAILURE);
+      run_next_test();
+    }
+  }
+  // Try adding an engine that doesn't exist
+  Services.search.addEngine("http://invalid/data/engine.xml",
+                            Ci.nsISearchEngine.DATA_XML,
+                            null, false, searchCallback);
+});
+
+function run_test() {
+  updateAppInfo();
+
+  let httpServer = new HttpServer();
+  httpServer.start(4444);
+  httpServer.registerDirectory("/", do_get_cwd());
+
+  do_register_cleanup(function cleanup() {
+    httpServer.stop(function() {});
+  });
+
+  run_next_test();
+}

toolkit/components/search/tests/xpcshell/test_notifications.js

@@ -40,8 +40,7 @@ function search_observer(subject, topic, data) {
       let retrievedEngine = Services.search.getEngineByName("Test search engine");
       do_check_eq(engine, retrievedEngine);
       Services.search.defaultEngine = engine;
-      // XXX bug 493051
-      // Services.search.currentEngine = engine;
+      Services.search.currentEngine = engine;
       do_execute_soon(function () {
         Services.search.removeEngine(engine);
       });

toolkit/components/search/tests/xpcshell/xpcshell.ini

@@ -19,4 +19,4 @@ skip-if = debug && os == "linux"
 [test_engineselect.js]
 [test_prefSync.js]
 [test_notifications.js]
-
+[test_addEngine_callback.js]