493051 - nsIBrowserSearchService addEngine changes browser.search.selectedEngine

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[david hardtke]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.8) Gecko/2009032712 Ubuntu/8.04 (hardy) Firefox/3.0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.8) Gecko/2009032712 Ubuntu/8.04 (hardy) Firefox/3.0.8

When I install a search engine using:

	var searchService = Components.classes["@mozilla.org/browser/search-service;1"].getService().QueryInterface(Components.interfaces.nsIBrowserSearchService);
	if (searchService.getEngineByName(name) == null) {
		searchService.addEngine(openSearchUrl, Components.interfaces.nsISearchEngine.DATA_XML, null, false);
	}

It changes the parameter browser.search.selectedEngine to be the newly installed search engine.  Note that the user is not being prompted (last field is false).  

Reproducible: Always

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is expected. See the appropriate documentation of the addEngine method:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsIBrowserSearchService.idl#168

It says: "if no confirmation dialog is shown, the new engine will be used right away automatically."

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Oh wait. I was too fast. Now with a second look I see what you mean. There is an inconsistency in the documentation:

168   /**
169    * Adds a new search engine from the file at the supplied URI, optionally
170    * asking the user for confirmation first.  If a confirmation dialog is
171    * shown, it will offer the option to begin using the newly added engine
172    * right away; if no confirmation dialog is shown, the new engine will be
173    * used right away automatically.

187    * @param confirm
188    *        A boolean value indicating whether the user should be asked for
189    *        confirmation before this engine is added to the list.  If this
190    *        value is false, the engine will be added to the list upon successful
191    *        load, but it will not be selected as the current engine.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ryan, what is the correct behavior here?

Comment 4

[Mike Kaply [:mkaply]]

Personally I think we need a way to have both behaviors.

Right now we have a limitation that there is no way to add an engine using a URL to an XML file and have it NOT be the default.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Sure there is, you just have to get selectedEngine, call addEngine, and then re-set selectedEngine. That's not ideal, certainly, but it's far from "no way".

I think we should just change the behavior to match the documentation, and people who want it to be selected after the installation can set selectedEngine.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Means we should update the @param section?

Comment 7

[Mike Kaply [:mkaply]]

Gacin:(In reply to comment #5)
> Sure there is, you just have to get selectedEngine, call addEngine, and then
> re-set selectedEngine. That's not ideal, certainly, but it's far from "no way".

I'll check again, but that didn't work when I tried. I think it's because addEngine returns before the engine is selected because it doesn't want to block on the network request for the XML.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #6)
> Means we should update the @param section?

Yes.

(In reply to comment #7)
> I'll check again, but that didn't work when I tried. I think it's because
> addEngine returns before the engine is selected because it doesn't want to
> block on the network request for the XML.

Ah, yeah, that could be. In that case you might need to use the engine-added notification.

Comment 9

[Mike Kaply [:mkaply]]

Do you really believe that making it the default engine without telling the user is the right decision?

Clearly the discrepancy in the comments mean that two people thought two different things.

Which is the "correct" behavior?

(And yes I know someone might be depending on the current behavior, but honestly I don't care. If someone wants their search engine to be the default, make them add an extra line to set selectedEngine, don't do it for them)

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #9)
> Do you really believe that making it the default engine without telling the
> user is the right decision?

Just a note, it will be the selected not the default engine.

Comment 11

[Mike Kaply [:mkaply]]

(In reply to comment #10)
is the right decision?
> 
> Just a note, it will be the selected not the default engine.

In practice, selected engine is default engine. "default engine" is only used when there is no chrome search at all.

To the user, they will thing someone took over search.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No. I wrote a Mozmill test for exactly this part in the last days and the selected engine is not the default engine. For en-US builds the default engine is Google. While I had selected Yahoo I got Google when querying defaultEngine.

Comment 13

[Mike Kaply [:mkaply]]

Sorry, that's not my point. My point is that to a user, default engine is irrelevant. Default engine is only used when search chrome is missing.

So to a typical user, if the selected engine is changed, they think someone changed the "default engine"

What we call it internally is irrelevant.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #9)
> Do you really believe that making it the default engine without telling the
> user is the right decision?

Are you asking me? I thought I made it clear in comment 5 that I agree that we should change the behavior to match the documentation. Write the patch and I'll review it.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

> (In reply to comment #6)
> > Means we should update the @param section?
> 
> Yes.

Oh, I guess this is what caused confusion. I misread that question, so ignore that answer. The documentation is fine.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Gavin, now I'm confused. :) Please see comment 2. The documenation describes it in two different ways. The param section differs from the function description.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

The parameter should control whether we prompt. If we don't prompt, we shouldn't set it as the selected engine. The code and documentation should both be fixed to match that behavior, if needed.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Hello all,

I'm taking this bug, as I've figured out how to obtain the desired behavior.  Just one question about what we expect some behaviors to be.

Once addEngine behaves correctly, the "Add [search engine name here]" option in the drop down search menu that appears on a page which has a rel="search" link element (like Bugzilla) does not set that engine to be the default anymore.  Should that set the newly added engine as the default?

My inclination would be to say no, but I'd appreciate some guidance from Gavin or somebody else who is a UI guru.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

I think we probably want to preserve the current behavior in the standard add-engine case.

That raises the question of how we should do that, I guess. You can change search.xml to have it set currentEngine in its engine-added observer, but that will potentially affect engines added elsewhere as well (e.g. from someone else calling addEngine). The search binding could set a flag to detect the cases where it's calling addEngine itself, but that's error prone and won't fix this problem for other open windows.

Maybe we need to add an additional addEngine parameter after all :/

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah I don't think there are any good answers here that don't involve messing with the interfaces.  What we need is to be able to choose from {no, ask, yes} for setting the engine as the default.  We also need similar flexibility in adding the engine to mimic the current behavior.  The cases are:

Add Engine? | Default Engine? | Comments
------------+-----------------+-----------------------------------------
    Yes     |       Yes       |  Current addEngine behavior, confirm = false
------------+-----------------+-----------------------------------------
    Yes     |       Ask       |  Not possible yet
------------+-----------------+-----------------------------------------
    Yes     |       No        |  Not possible yet
------------+-----------------+-----------------------------------------
    Ask     |       Ask       |  Current addEngine behavior, confirm = true
------------+-----------------+-----------------------------------------
    Ask     |       No        |  Not possible yet

IMO it makes no sense to ask to add the engine and then always make it the default, or to not add the engine (given that we're calling addEngine).

What I think we need to do is convert confirm into engineFlags with the following values:

ENGINE_FLAG_ADD_YES
ENGINE_FLAG_ADD_ASK
ENGINE_FLAG_DEFAULT_YES
ENGINE_FLAG_DEFAULT_ASK
ENGINE_FLAG_DEFAULT_NO

where ENGINE_FLAG_ADD_ASK + ENGINE_FLAG_DEFAULT_YES is not allowed, ENGINE_FLAG_ADD_YES + ENGINE_FLAG_DEFAULT_ASK is the default addEngine behavior, and the search box uses ENGINE_FLAG_ADD_YES + ENGINE_FLAG_DEFAULT_YES

That's what I came up with.  If nobody comes up with anything better I'll go ahead and make a patch for this setup and we can play around with it.

Comment 21

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 22

[Mike Kaply [:mkaply]]

I just ran into this again. I want to add an Engine but I don't want to change the user's default (I want to be nice).

This is not easily done with the code as it exists.

Comment 23

[Mike Kaply [:mkaply]]

OK, for Firefox 4, we should make this change for the API. This is such a bad bug for addon developers.

Kyle, were you still interested in making a patch?

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't have time to work on this ATM, sorry.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Switches the default behavior, and adds a callback to addEngine to allow people to set the current engine if they need to.

Comment 26

[Ben Bucksch (:BenB)]

Compare bug 598623, which has wider scope.

Comment 27

[Ben Bucksch (:BenB)]

Actually, the patch here fixes all problems listed in bug 598623. Thanks!
I'll mark it a DUP and broaden the summary here.

I'll also mark this as blocking2.0?, because we (web.de/GMX, several million users) *need* this for FF4.

(In FF3.6, we just copies the OSD files to searchplugins/ function, and FF would use them immediately, but that regressed in FF4, it doesn't load them until restart. So, we need a solution for FF4, and this patch here is by far the cleanest solution.)

Comment 28

[Ben Bucksch (:BenB)]

Comment on attachment 466870 [details] [diff] [review]
patch

Gavin, thanks a lot for this patch! It's great, solves all the problems with the addEngine() API (see above) and is almost perfect.

I tried it and it works like a charm.

To help getting it in, here's a code review:

>     /**
>      * Handle an error during the load of an engine by prompting the user to
>      * notify him that the load failed.
>      */
>-    function onError(aErrorString, aTitleString) {
>-      if (aEngine._engineToUpdate) {
>-        // We're in an update, so just fail quietly
>-        LOG("updating " + aEngine._engineToUpdate.name + " failed");
>+    function onError(aPrompt, aErrorString, aTitleString) {
>+      // Notify the callback of the failure, if needed
>+      if (aEngine._installCallback)
>+        aEngine._installCallback.notify(null, false);
>+
>+      if (aEngine._engineToUpdate || !aPrompt) {
>+        if (aEngine._engineToUpdate) {
>+          // We're in an update, so just fail quietly
>+          LOG("updating " + aEngine._engineToUpdate.name + " failed");
>+        }
>         return;
>       }
>+

Please change aPrompt to aDoDisplayError and document it. I thought it's a reference to an nsIPromptService object.

I'd write the code like so:

+      if (!aDoDisplayError) {
+        return;
+      }
+      if (aEngine._engineToUpdate) {
+        // We're in an update, so just fail quietly
+        LOG("updating " + aEngine._engineToUpdate.name + " failed");
+        return;
+      }

I think that's a lot more readable.

>-      onError();
>+      onError(true);

We generally don't want to show a prompt here, if we have a callback. A caller may well want to handle the error himself, and a user dialog is likely to very much get in the way of that. If we notified the caller, he can (and must) handle the error.

We'd also need to pass the error text in this case, so we need to add 2 params to the callback. Alternatively, we could have 2 callbacks, one for success (with engine as param) and one for error (with error strings are params).

>-        onError();
>+        onError(true);
>         LOG("_onLoad: Bogus engine _dataType: \"" + this._dataType + "\"");

Please pass this as error text to the error handler. (Better english than nothing.)
Proper errors are important. laga did run into exactly that (he erronously passed 3 = OSD instead of 2 = XML), and the error message was misleading, saying the URL was not correct, sending him to debug in the wrong direction.

>-        onError();
>+        onError(true);
>         LOG("_onLoad: Bogus engine _dataType: \"" + this._dataType + "\"");

ditto

>       LOG("_onLoad: Failed to init engine!\n" + ex);
>       // Report an error to the user
>-      onError();
>+      onError(true);

ditto

>-        if (aEngine._confirm)
>-          onError("error_duplicate_engine_msg", "error_invalid_engine_title");
>+        onError(aEngine._confirm, "error_duplicate_engine_msg", "error_invalid_engine_title");

Here we do it.

>             LOG("_onLoad: updateURL missing in updated engine for " +
>                 aEngine.name + " aborted");
>+            onError(false);

ditto

>           LOG("_onLoad: updateURLs do not match! Update of " + aEngine.name + " aborted");
>+          onError(false);

ditto

>     } catch (ex) {
>       FAIL("addEngine: Error adding engine:\n" + ex, Cr.NS_ERROR_FAILURE);
...
>+        aCallBack.notify(null, false);

ditto


The rest of the patch looks OK.

Comment 30

[Ben Bucksch (:BenB)]

This is serious because of a FF4 regression, bug 598697.

Also, the following use case is almost impossible with the current API:
only upon user confirmation, add several search engines, and make one of them the default (depending on user confirmation).
See bug 598623 for the API problems that make this almost impossible. All of these are fixed by this patch.

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

I don't think this blocks, based on the regression bug which has a workaround (to use <em:unpack>) but if the patch gets reviewed I'd be pretty happy to accept it.

Feel free to renominate if you feel that I've missed something. We're not saying that it's ideal, we're saying that everyone can achieve their goals without it.

Comment 32

[Ben Bucksch (:BenB)]

beltzner, the second paragraph in comment 30 is not practically possible with the current API. We don't currently run into that, but if somebody's policy changes, we will, and then we'll have a problem with an unchangeable FF4.

Comment 33

[Mike Kaply [:mkaply]]

Is there any reason this patch is not going forward?

Comment 34

[Ben Bucksch (:BenB)]

Mike, I made a review of it - in the interest of getting it in. Could you adopt the patch and fix the review comments?

Comment 35

[:Gavin Sharp [email: [e-mail]]]

(In reply to Michael Kaply (mkaply) from comment #33)
> Is there any reason this patch is not going forward?

No assignee, no review request.

Comment 36

[Mike Kaply [:mkaply]]

> Please pass this as error text to the error handler. (Better english than nothing.)
Proper errors are important. laga did run into exactly that (he erroneously passed 3 = OSD instead of 2 = XML), and the error message was misleading, saying the URL was not correct, sending him to debug in the wrong direction.

This will make for a much more complicated, and I'm not sure of the proper way to do it.

As it stands, the search code only has three error messages:

error_loading_engine_msg2
error_duplicate_engine_msg
error_invalid_engine_ms

error_loading_engine_msg2 is used as the generic message for most failures.

There are five additional error conditions that would need to be handled if we pass a message along.

The errors are

1. Got zero bytes when we downloaded
2. Bad engine datatype
3. Initializing the engine failed (bad data)
4. User chose not to confirm the engine
5. updateURL missing (doesn't need to be reported since it doesn't happen from addEngine)
6. new updateURL doesn't match the old updateURL  (doesn't need to be reported since it doesn't happen from addEngine)
7. Generic error adding engine (try/catch around new Engine)

And we can't just pass the English along because it's not meant to be used in a standard way. We would have to create five new standard error conditions and then either provide messages for each of those conditions, or provide some way for the error code to ignore those conditions.

It seems wrong to just keep making up text representations for these messages, since the only reason they exist is to query from the properties files.

I'm open to any suggestions.

Comment 37

[:Gavin Sharp [email: [e-mail]]]

It doesn't make sense to modify search service error reporting here, in an unrelated bug. If you guys want to fix that, please do it in a new bug.

Comment 38

[Mike Kaply [:mkaply]]

Attachment: Correct a couple review comments, mostly the same

gavin, obviously this is your patch originally so I'm not sure you should review it?

The only change I made was to the formatting of onError to make it more readable.

Not sure of the protocol here.

Comment 39

[Ben Bucksch (:BenB)]

> Not sure of the protocol here.

mkaply, if you and gavin work on a patch together, you are each other's reviewers.

Comment 40

[Mike Kaply [:mkaply]]

Gavin:

Any chance you'll have time to review this? Is there someone else I should ask?

Comment 41

[Mike Kaply [:mkaply]]

Gavin - any chance for review here?

Comment 42

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

This is untested, but it fixes a few problems (it being possible to never drop the reference to the passed-in callback, dealing with callback exceptions) and cleans up a few things (onError handling, some comments) from the previous patch.

Mike, can you test this?

I will file a followup to get nsSearchService out of the UI business (it really should leave the prompting and such to the caller).

Comment 43

[Mike Kaply [:mkaply]]

Comment on attachment 738834 [details] [diff] [review]
updated patch

Patch works great for me. A few comments.

> Cu.reportError("Error invoking addEngine install callback: " + ex);

Cu is not defined in the file, so we need to either define it at the top or use Components.utils

> aCallback.notify(e, success);

Should be aCallBack.

The bigger question is if a callback has been specified, should we show an error at all? Is the intent of the callback only to let the caller know what happened? Or to allow them to do something if there is an error?

Comment 44

[:Gavin Sharp [email: [e-mail]]]

(In reply to Michael Kaply (mkaply) from comment #43)
> The bigger question is if a callback has been specified, should we show an
> error at all? Is the intent of the callback only to let the caller know what
> happened? Or to allow them to do something if there is an error?

That's related to the "Get out of the UI business" comment I was going to file a followup on. There should be no prompting in nsSearchService itself, and consumers who want prompts should be able to implement them using the callback.

Comment 45

[:Gavin Sharp [email: [e-mail]]]

I filed bug 863474.

Comment 46

[:Gavin Sharp [email: [e-mail]]]

(In reply to Michael Kaply (mkaply) from comment #43)
> Cu is not defined in the file, so we need to either define it at the top or
> use Components.utils

> > aCallback.notify(e, success);
> 
> Should be aCallBack.

Thanks, fixed these locally. This really also needs a test - any chance you have time to write one, Mike? http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/ has some examples to copy.

Comment 47

[Mike Kaply [:mkaply]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #46)
> This really also needs a test - any chance you
> have time to write one, Mike?
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/
> tests/xpcshell/ has some examples to copy.

Sure. I'll take a look. How do I actually run the tests?

Comment 48

[Matthew N. [:MattN]]

from the source dir:
./mach xpcshell-test toolkit/components/search/tests/xpcshell/test_foo.js

Comment 49

[Ben Bucksch (:BenB)]

> > aCallback.notify(e, success);
> 
> Should be aCallBack.

Actually, "aCallback" is correct, because "callback" is a noun, a specific term in programming. "Call back" would be verb. But doesn't matter either way.

Comment 50

[Ben Bucksch (:BenB)]

review:

IDL API:

I personally would prefer 2 callbacks: one for success, one for error. Normally, you do very different things in these 2 cases. More importantly, the error case needs a reason, you need to pass an error code and error message back to the caller. Bug 863747 wants to move the actual UI prompts out of the component, and that's made a lot easier with a separate error callback that has the error code and msg.

search.xml:
+          this.currentEngine = target.engine;
vs.
+              searchService.currentEngine = engine;

Why is one case setting currentEngine on the widget, and the other directly on the searchService? If this is correct as-is, could you please at least add a comment explaining why this difference, preferably in form of a JavaDoc documentation of the "command" event handler?

-   * right away; if no confirmation dialog is shown, the new engine will be
-   * used right away automatically.
+   * right away.

Please make this API change clear, by stating that it's *no longer* automatically selected as it used to. We also need to document this as in "Firefox 23 for developers", because this is likely to break many addons.

     } catch (ex) {
       LOG("_onLoad: Failed to init engine!\n" + ex);
       // Report an error to the user
-      onError();
+      onError({});

Please pass the exception text as error message. It's important not to eat error reasons, as a generic "setting search engine didn't work" message only makes people tear their hairs. If you're concerned about confusing end users, you can make a generic message first, and then append the technical detail as second paragraph.

> Should be aCallBack.

Oh, I see now. You used 2 different spellings for the *same* variable. :-)

---

If you wish, I can make any of these changes myself, under the condition that you're then reviewing them.

Comment 51

[Mike Kaply [:mkaply]]

Attachment: Attempt at a test

Here's my attempt at a test.

It shows TEST-PASS in the console when I run it, but in the output it says:

 0:02.60 INFO | Result summary:
 0:02.60 INFO | Passed: 0
 0:02.60 INFO | Failed: 1

I don't understand why.

All I'm doing is making sure that addEngine no longer sets the default engine.

Comment 52

[:Gavin Sharp [email: [e-mail]]]

The unneeded removeObserver(search_observer) is probably causing the failure?

Comment 53

[:Gavin Sharp [email: [e-mail]]]

(In reply to Ben Bucksch (:BenB) from comment #50)
> I personally would prefer 2 callbacks: one for success, one for error.
> Normally, you do very different things in these 2 cases. More importantly,
> the error case needs a reason, you need to pass an error code and error
> message back to the caller. Bug 863747 wants to move the actual UI prompts
> out of the component, and that's made a lot easier with a separate error
> callback that has the error code and msg.

True, but since this is an IDL-defined interface two callbacks is kind of a pain. We could alternatively pass a callback object with two methods, similar to nsIContentPrefCallback2 or mozIStorageStatementCallback, I guess.

> search.xml:
> +          this.currentEngine = target.engine;
> vs.
> +              searchService.currentEngine = engine;
> 
> Why is one case setting currentEngine on the widget, and the other directly
> on the searchService?

Good catch - no good reason, it should use this.searchEngine. I just didn't change that from the version of this patch that pre-dated bug 738818.

> Please make this API change clear, by stating that it's *no longer*
> automatically selected as it used to. We also need to document this as in
> "Firefox 23 for developers", because this is likely to break many addons.

I guess there's no harm in mentioning "this doesn't affect selectedEngine/defaultEngine", but I don't think the IDL is the best place to indicate *changes* in the API (as opposed to describing its current state).

Comment 54

[Matthew N. [:MattN]]

Comment on attachment 738834 [details] [diff] [review]
updated patch

Review of attachment 738834 [details] [diff] [review]:
-----------------------------------------------------------------

f=me for now as I want to take a closer look at nsSearchService.js

::: toolkit/components/search/nsSearchService.js
@@ +1145,5 @@
>    // 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: false,
> +  // A function to invoked when this engine object's addition completes (or
> +  // fails). Only used for during installation via addEngine.

s/during//

@@ +3460,5 @@
>      try {
>        var uri = makeURI(aEngineURL);
>        var engine = new Engine(uri, aDataType, false);
> +      if (aCallBack) {
> +        engine._installCallback = function (e, success) {

s/e/eng/ IIUC. (I initially thought "e" was for the error message).

Why is this engine method taking an engine as its first argument?  It could just only pass the engine along to the callback if |success| is truthy.

Comment 55

[Ben Bucksch (:BenB)]

> We could alternatively pass a callback object with two methods, similar
> to nsIContentPrefCallback2 or mozIStorageStatementCallback, I guess.

Personally, I find *that* solution a pain, because I can't pass a JS function anymore (XPIDL "function" feature).

> True, but since this is an IDL-defined interface two callbacks is kind of a pain.

I understand, but it's not really difficult, just 2 interfaces instead of 1.

If I was you, I would introduce a generic interface somewhere in a more central place, and use the same 2 interfaces everywhere, because we need it in every async API. I.e.

/**
 * Called when an async operation completes.
 * Only one of onError and onSuccess/onResult are called, never both.
 */
interface tkIErrorCallback : nsISupports
{
  void onError(nsIException e);
};
interface tkISuccessCallback : nsISupports
{
  void onSuccess();
};
interface tkIResultCallback : nsISupports
{
  void onResult(nsISupports result);
};

I'd be happy to supply a patch.

> > Please pass ... error message.

Either way, it's important to have root cause details for all errors passed on to caller.

> Good catch - no good reason, it should use this.searchEngine

Ah, nice, glad I could help.

Comment 56

[:Gavin Sharp [email: [e-mail]]]

(In reply to Ben Bucksch (:BenB) from comment #55)
> > We could alternatively pass a callback object with two methods, similar
> > to nsIContentPrefCallback2 or mozIStorageStatementCallback, I guess.
> 
> Personally, I find *that* solution a pain, because I can't pass a JS
> function anymore (XPIDL "function" feature).

You can pass an object with two function properties, which is basically equivalent to just passing two functions.

Comment 57

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch with tests for the back-end changes

This addresses all comments so far and has decent test coverage. The UI bits (i.e. the code in search.xml) aren't well tested, but I tested those manually.

I refactored things a little bit to make fixing bug 863474 easier.

Comment 58

[Matthew N. [:MattN]]

Maybe I'm missing something so could you explain this for me:
(Quoting Matthew N. [:MattN] from comment #54)
> Why is this engine method taking an engine as its first argument?

Comment 59

[Ben Bucksch (:BenB)]

> void onError(in unsigned long errorCode);

We really really need the error message (string), see comment 50.

> onError()

There are still many places where onError() is called. It should *always* be called with reason (as string). I don't want to ever get "unknown error", that's a nightmare to debug.

> // A function to invoked

NIT: "to invoke" or "to be invoked"

Comment 60

[:Gavin Sharp [email: [e-mail]]]

(In reply to Ben Bucksch (:BenB) from comment #59)
> > void onError(in unsigned long errorCode);
> 
> We really really need the error message (string), see comment 50.

I don't understand why. What's the use case? In any case, we should have this debate in a followup bug. This patch fixes this bug as summarized, and we can always add more features to the error callback after the fact.

Comment 61

[:Gavin Sharp [email: [e-mail]]]

(In reply to Matthew N. [:MattN] from comment #58)
> Maybe I'm missing something so could you explain this for me:
> (Quoting Matthew N. [:MattN] from comment #54)
> > Why is this engine method taking an engine as its first argument?

Not a great reason, I guess. I think I originally did this because the callback was invoked from within _addEngineToStore, which gets an already-wrapped object (that has been passed through xpcom via the observer service). So it's wrapped differently than the plain JS object that comes from new Engine(). But I don't even know that our wrappers still work that way (they probably don't), and in any case it doesn't matter because the object should be wrapped appropriately automatically when passing them back to the callback.

Comment 62

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Tweaks made.

Comment 63

[Ben Bucksch (:BenB)]

> > We really really need the error message (string), see comment 50.

> ... in a followup bug.

Fair enough.

Comment 64

[Mike Kaply [:mkaply]]

Any chance this will make the FF 23 train?

Comment 65

[Mike Kaply [:mkaply]]

Is there anything that can be done to move this along?

I'd really like to see this at least in Firefox 24, so we don't have to support the old way for another 7 releases.

Comment 66

[:Gavin Sharp [email: [e-mail]]]

Note to self: need to fix http://mxr.mozilla.org/mozilla-central/search?string=493051 too.

Comment 67

[:Gavin Sharp [email: [e-mail]]]

Docs o update are at:
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIBrowserSearchService#addEngine%28%29

Comment 68

[:Gavin Sharp [email: [e-mail]]]

This is now on top of bug 860560 in my queue.

Comment 69

[:Gavin Sharp [email: [e-mail]]]

Attachment: additional test changes

Comment 70

[:Gavin Sharp [email: [e-mail]]]

Attachment: final patch

Comment 71

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/423d565f5637

Comment 72

[:Gavin Sharp [email: [e-mail]]]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f6c5f89800 for failing debug xpcshell tests. I think the code that prompts is causing trouble (i.e. a fix for bug 863474 would be useful). I think we can probably just skip that test in the mean time.

Comment 73

[:Gavin Sharp [email: [e-mail]]]

Attachment: test fix

Comment 74

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 763802 [details] [diff] [review]
test fix

sigh, wrong bug

Comment 75

[:Gavin Sharp [email: [e-mail]]]

Attachment: test fix

Comment 76

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9b946da20cb

Comment 77

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #75)
> Created attachment 764130 [details] [diff] [review]
> test fix

I ended up just replacing the promptservice/nsiprompt for the test so that the prompts were no-ops.

Comment 78

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9b946da20cb

Comment 79

[Mike Kaply [:mkaply]]

I apologize for bringing this one back, I'm now encountering the results of this patch and one thing is confusing me.

The IDL says:

240 * A nsISearchInstallCallback that will be notified when the
241 * addition is complete, or if the addition fails. It will not be
242 * called if addEngine throws an exception.

Is there a case where the addition fails, but it doesn't throw an exception? Looking at the code, it the callback never happens in the error case because we always return in those cases after showing the error.

Comment 80

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mike Kaply (:mkaply) from comment #79)
> I apologize for bringing this one back, I'm now encountering the results of
> this patch and one thing is confusing me.
> 
> The IDL says:
> 
> 240 * A nsISearchInstallCallback that will be notified when the
> 241 * addition is complete, or if the addition fails. It will not be
> 242 * called if addEngine throws an exception.
> 
> Is there a case where the addition fails, but it doesn't throw an exception?

The intent of the comment is to clarify that failure of this method manifests itself in one of two ways (but not both):
- the method throws (for failures before we need to do async stuff, e.g. you specify an invalid name)
- the nsISearchInstallCallback onError handler is invoked (for failures after async stuff, like an invalid engine file)

Is this posing a problem for you?

Comment 81

[Mike Kaply [:mkaply]]

> Is this posing a problem for you?

Not now that I understand it better. Makes perfect sense. The main change for me was that addEngine used to fail silently if the engine already existed whereas now an error dialog appears. So I need to check if the engine exists before adding. Not a big deal.

I just want to make sure that for Firefox 24 we document what is different for add-on developers.

Comment 82

[Mike Kaply [:mkaply]]

> So I need to check if the engine exists before adding. Not a big deal.

Actually, this is a big deal, and I'm putting a note here in case other people run into it.

The problem is that because addEngine is async, getEngineByName will return false even if the engine is in the process of being added. So addEngine will fail and put up a dialog.

So:

addEngine(SOMEXML);
if (!getEngineByName(XMLNAME)) { <---- This returns false because the engine doesn't exist yet
  addEngine(SOMEXML); <---- This now errors when it didn't before.
}

I know this example looks strange, but in practice it can happen. I've seen two add-ons do it so far.

Also, the errorCallback doesn't seem to get called in this case either. We really need to do the followup bug to allow the error message to be controlled by the caller.

Comment 83

[Ben Bucksch (:BenB)]

> So addEngine will fail and put up a dialog.
> Also, the errorCallback doesn't seem to get called in this case either.
> We really need to do the followup bug to allow the error message to be controlled by the caller.

Yes, if we have an errorCallback, and the code still puts up a prompt in any case whatsoever, that's a bug that causes massive problems for callers.

I've been stressing that in comment 28, comment 50 and 59.

Comment 84

[:Gavin Sharp [email: [e-mail]]]

A patch for bug 863474 would be most welcome!

Comment 85

[Ben Bucksch (:BenB)]

Thanks