863474 - Search service shouldn't be responsible for prompting for engine installation failures

Status: RESOLVED FIXED

(Firefox :: Search, task, P3)

Description

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

See discussion in bug 493051. The prompting should be up to the search service consumer (UI code), not built in to "addEngine" itself.

Comment 1

[Ben Bucksch (:BenB)]

I.e. use callbacks for success, errors (and status updates).

Comment 2

[Ben Bucksch (:BenB)]

Mike Kaply, can you make a patch, please?

Comment 3

[Mike Kaply [:mkaply]]

Gavin:

Should the prompts stay in, and we just don't display them if there is a search callback?

Should I add new error constants for the things we lump under unknown (bad download, invalid dataType), as well as the "user clicked no on the confirm" case?

Comment 4

[Ben Bucksch (:BenB)]

> Should I add new error constants for the things we lump under unknown
> (bad download, invalid dataType), as well as the "user clicked no on the confirm" case?

Yes, please do.

Comment 5

[Ben Bucksch (:BenB)]

> Should the prompts stay in, and we just don't display them if there is a search callback?

I would suggest yes, for backwards compat. At the top, I'd add something like:
 if (!errorCallback) {
   errorCallback = function searchErrorCallback(e) {
     errorPrompt(e);
   };
 }
and then remove all the UI code from the rest of the function.

Comment 6

[Mike Kaply [:mkaply]]

Attachment: First pass at a patch

This is a first pass at a patch.

It reworks the error handling into one function that takes the error code and if there is a callback, just returns. If not, it displays an error.

It has a case statement, so we an add detailed errors in the future. I did add the new error codes, but I haven't revved the IDL yet.

I've verified it does not regress bug 937870.

I haven't created any tests yet. In theory, this new patch will allow us to test better since we can handle the specific error cases.

Ben: creating a custom callback didn't really work in the context of this code which is why I did it how I did it.

Comment 7

[Mike Kaply [:mkaply]]

Also, I left the invalid type code behind, but I have no idea why it was put in this part of the code (the async part).

The invalid type check can be before we try to load the URL.

Also, since we're going to rev the ID anyway, we could probably use this opportunity to remove the SHERLOCK type because it's no longer supported.

Comment 8

[Mike Kaply [:mkaply]]

There's only one test change needed for this:

diff -r 250fe25bff9f toolkit/components/search/tests/xpcshell/test_addEngine_callback.js
--- a/toolkit/components/search/tests/xpcshell/test_addEngine_callback.js	Tue Dec 10 15:25:55 2013 -0500
+++ b/toolkit/components/search/tests/xpcshell/test_addEngine_callback.js	Wed Dec 11 11:56:52 2013 -0600
@@ -100,7 +100,7 @@
     },
     onError: function (errorCode) {
       do_check_true(!!errorCode);
-      do_check_eq(errorCode, Ci.nsISearchInstallCallback.ERROR_UNKNOWN_FAILURE);
+      do_check_eq(errorCode, Ci.nsISearchInstallCallback.ERROR_INVALID_ENGINE);
       run_next_test();
     }
   }

Comment 9

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

(In reply to Mike Kaply (:mkaply) from comment #6)
> It reworks the error handling into one function that takes the error code
> and if there is a callback, just returns. If not, it displays an error.

My goal for this bug is to remove all of the "display an error" code from nsSearchService, and put it in the relevant search service callers (which can use error callbacks to figure out when/what to prompt). This patch seems like a step in that direction, but doesn't go far enough - are you intending to go farther?

Comment 10

[Mike Kaply [:mkaply]]

That would break any add-ons that this API...

Comment 11

[Ben Bucksch (:BenB)]

Gavin, we're trying to preserve backwards compatibility of the API, to not break existing addons.

The caller can now provide the errorCallback, and in this case the nsSearchService will not prompt. (Before, it was prompting in some cases despite error callback, that doesn't make sense, that's what this patch is fixing.) This is where we want to be.

For addons which don't use the errorCallback yet, we provide a default errorCallback implementation, a shim to not break them. That code should be confined in one place, so doesn't disturb anything.

As for the current patch, it's not confined enough for my taste yet. onError() is mixing calling the caller's callback with the UI prompt in the same function. Personally, I think we should do what I suggested in comment 5. I didn't quite understand that's not possible. Mike, can you fix this or elaborate why not, please?

Gavin, would that be sufficient for you?

Comment 12

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

What add-ons are you worried about breaking? None of my proposed changes should affect addons, unless they're calling addEngine() with aConfirm=true. And I don't think that particular use is common enough to be worth worrying about.

Comment 13

[Ben Bucksch (:BenB)]

If that's true, I'd agree. I thought that all callers expected the service to show error dialogs, because that's how things used to work in the past years.

Comment 14

[Mike Kaply [:mkaply]]

Jorge:

Can you do a search of add-ons on AMO for consumers of addEngine?

Need to determine how many are using true as the fourth parameter.

Comment 15

[Mike Kaply [:mkaply]]

Attachment: Move UI into callers

Here's a new patch that does as Gavin requested.

It moves the error notifications into the callers of addEngine (All in various *Sidebar.js files).

Comment 16

[Jorge Villalobos [:jorgev] (he/him)]

I found 4 instances:

https://addons.mozilla.org/addon/the-official-blogtv-toolbar/
https://addons.mozilla.org/addon/taobaoshopping%E6%B7%98%E5%AE%9D%E7%BD%91%E5%AF%BC%E8%88%AA%E6%8C%89%E9%92%AE/
https://addons.mozilla.org/addon/joyoamazonshopping%E5%8D%93%E8%B6%8A%E4%BA%9A%E9%A9%AC%E9%80%8A%E5%AF%BC%E8%88%AA%E6%8C%89%E9%92%AE/
https://addons.mozilla.org/addon/taobaoshopping/

The first one is ancient. The others are related (Chinese shopping) with #2 and #3 being from the same developer.

Comment 17

[Mike Kaply [:mkaply]]

The only one that works on current Firefox is the second one, and the code it has for addEngine is never called.

Since changing this API doesn't break anything (you just don't get the error reported), I think Gavin is correct - we should just move the UI stuff out and document.

Comment 18

[Ben Bucksch (:BenB)]

Comment on attachment 8348213 [details] [diff] [review]
Move UI into callers

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

Mike, if we use this approach, can you make a patch that doesn't repeat the UI code 3 times, but shares it in a way that any other caller can use as well?

Most of the callback code is to generate the error strings for an error code. I think this - creating error messages, not just error codes -, is something that properly belongs into the search component, because it's difficult for callers to do (copy all strings or depend on XUL string bundles) and keep up with new errors.

So, basically, if we make (errorCode, errorCode), and move the string bundle code into onError(), most of the code duplication goes away, and it's also more future-proof when we add new error codes, and it's easier to other callers.

Comment 19

[Ben Bucksch (:BenB)]

Attachment: search-install-errorcallback-863474-3.diff

This implements what I suggested.

Comment 20

[Ben Bucksch (:BenB)]

Attachment: onError with code and msg, reduces code duplication

fix Typo
Note: Patch 3 and 4 are based on Mike's patch 2.

There is more room for removal of duplication, by moving validate _validateSearchEngine() from 3x sidebar.js into addEngine(). This would be straight-forward and logical, but we'd need to replace _installCallback with separate _installErrorCallback and _installSuccessCallback. I think that would be a good change, but it's a bigger changeset.

Comment 21

[Mike Kaply [:mkaply]]

Comment on attachment 8366015 [details] [diff] [review]
onError with code and msg, reduces code duplication

This looks good to me.

Although, the UUID will need to be revved for the new function signature.

Comment 22

[Mike Kaply [:mkaply]]

I thought the getNewPrompter versus Services.alert was weird, so I asked, Here's Neil's response:

<NeilAway> Ms2ger: Services.prompt uses xpcom :-P
<NeilAway> mkaply: ww is an embedding interface, Services.prompt is an implementation detail
<mkaply> NeilAway: is there a guideline when to use one versus the other?
<NeilAway> mkaply: use Services.prompt if you're calling it directly, ww.getNewPrompter if you need it indirectly e.g. for notification callbacks
<mkaply> NeilAway: this came up in some new search patch. Comparing http://mxr.mozilla.org/mozilla-central/source/browser/components/sidebar/nsSidebar.js#51 to http://mxr.mozilla.org/mozilla-central/source/browser/metro/components/Sidebar.js#65
<NeilAway> mkaply: nsSidebar.js was written way before Services.prompt existed
<mkaply> I bet these cases of ww.getNewPrompter were missed when we did the big "Move everything to Services" patch.

So either both should be "getNewPrompter" because they are now in notification callbacks, or both should be Services.alert.

Your call.

Comment 23

[Ben Bucksch (:BenB)]

Mike, this is a symptom of the forking: Clearly, the 3x Sidebar.js are all forks of the same code, and unfortunately, their implementations have since derived/diverted.
(This is why, when I copy code of more than 5 lines in our codebase, I always make // <copied from=/to=> markers in code comments and expect developers changing one copy to also change the other copy. And I try to avoid copies wherever possible.)

If one or the other is the right thing to do in both cases: fine, whatever.

Comment 24

[Mike Kaply [:mkaply]]

I agree, I'm simply saying as long as we're in this code, we should fix it.

Comment 25

[Ben Bucksch (:BenB)]

gavin: ping for review.

Comment 26

[Ben Bucksch (:BenB)]

gavin: ping for review.

Comment 27

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

Comment on attachment 8366015 [details] [diff] [review]
onError with code and msg, reduces code duplication

Apologies for the unreasonable review delay. I won't let it happen again.

>diff --git a/browser/components/sidebar/nsSidebar.js b/browser/components/sidebar/nsSidebar.js

>+function searchCallback(engineURL) {

engineURL is unused?

>diff --git a/netwerk/base/public/nsIBrowserSearchService.idl b/netwerk/base/public/nsIBrowserSearchService.idl

> interface nsISearchInstallCallback : nsISupports

>+  const unsigned long ERROR_NO_CONFIRMATION = 0x3;

_confirmAddEngine should also move to UI code at some point, so I'd rather not expose this error code now (can use ERROR_UNKNOWN_FAILURE in the interim).

>+  const unsigned long ERROR_INVALID_TYPE = 0x5;

These failures should be detected at the addEngine call site (and it should throw immediately) rather than being propagated to the error handler, so let's not add this.

>+  const unsigned long ERROR_INVALID_UPDATEURL = 0x6;

This can never be hit via the addEngine case, it's an implementation detail of the engine updating system. So we shouldn't add it either.

>+   * @param errorMsg
>+   *        An error message, in a form understandable by an end user.

I don't think the search service should be responsible for producing this - dealing with user-visible strings should be moved to the UI code as well (based on the errorCode).

Comment 28

[Ben Bucksch (:BenB)]

Comment on attachment 8366015 [details] [diff] [review]
onError with code and msg, reduces code duplication

> I don't think the search service should be responsible for producing this - dealing with
> user-visible strings should be moved to the UI code as well (based on the errorCode).

I think components should always be responsible for their own error messages. Reasons are:
* As a caller, I don't want to add strings that are for third party components
* There are presumably several callers, not just one caller in Firefox. They would all have
  to copy the error strings. But that's a thing you don't want to copy, esp. not to each caller.
* The component should be able to add error situations at any time. But it can't add
  error strings to the callers. So, splitting error codes and strings into different components
  breaks the component separation.

I've experienced this often when using modules, and it's always been a massive pain to use modules that do not expose *both* error code and error string. I need to error code, because I might want to catch 1 or 2 specific errors and handle those. I need the error strings, because I do not want to care about all the other errors and just show an error message to the user. "Not care" includes not copying the strings.

It goes back to the goal: As a caller, I want to have the flexibility to:
* Make a different UI - e.g. nsIPrompt, or Android toast, show it somewhere inline, or whatever.
  In all these cases, the error string would be the same.
  (If they are not the same, I still have the option to use custom strings.)
* Handle one specific error in my code.

Returning both error codes and strings allows the caller to do this, trivially and without effort.

This is kind of a deal-breaker. If the component doesn't return error strings to the caller, and all error situations, then we can't use the new API. The burden of carrying around third-party error strings is just too high.

Gavin, could you please reconsider this?

Comment 29

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

I really don't understand the problem you're describing.

The search service should be a low-level API. It isn't because of this bug. It needs to support installing engines, and informing the caller of failures, but it shouldn't impose any constraints on the caller about how those errors are exposed to the user. There's no benefit to consolidating the "map error code to error string" code in the search service because typically different consumers will want different strings/mappings (or won't need strings at all).

If you have a particular search service consumer in mind that wants to copy Firefox's strings/mappings exactly for some reason, we can discuss factoring it out somehow, but it's not a reason for those to live in the search service.

Comment 30

[Ben Bucksch (:BenB)]

> I really don't understand the problem you're describing. The search service should be
> a low-level API.

All that I stated above is true for low-level APIs. To explain the problem, I'll take another example that I painfully remember: For an ISP, I needed to write a modem dial module that was using a C library to dial. The C library would give me only error codes, dozens of them. Now, I had to find 1) documentation for each error 2) manually copy the error messages into a .properties file, 3) write code that maps the error code to the error message 4) place all that somewhere within my code. It felt like a total waste of time. And I felt like the list of error messages for that third-party module didn't belong into *my* code. And I worried that when the module would add an error code, or had undocumented error codes, all I would be able to say was "unknown error while dialing".

If you went through this once, you sigh. If you went through this 3 times, you yell.

Now, from the perspective of the module maintainer, it's not better: He doesn't have to maintain strings, but when he wants to split "modem can't be found" into "port doesn't exist" and "no modem connected to port", and he introduces 2 new error codes for it, he knows that my calling code will report "unknown error" for both errors, which is actually *worse* than "modem can't be found". Likewise, if the module wants to give an error like "port COM4: doesn't exist", the module has to rely on me to do the variable replacement for "COM4:" - and a different replacement for each error code.

In reality, most callers will not bother to create nice error messages and just say "error 45". This is why we see this so often, even with software from Adobe and similar big names. If you think that's their fault, see first paragraph.

> typically different consumers will want different strings/mappings

My experience is exactly opposite. Most consumers just want to display an error msg to the user. They don't care about its content, just that it usefully describes the error. They do very much about the concrete dialog it appears in.

As said, offering both error code and string does not reduce the possibilities for the caller. If you are right and the strings are not useful, the caller is free to ignore the error msgs. I think the vast majority of callers needs the exact same strings as the Firefox frontend, though, and can't copy them and their construction (given that we need placeholder replacements, too).

> If you have a particular search service consumer in mind that wants to copy
> Firefox's strings/mappings exactly for some reason

Yes, our extension was working around the fact that packed exts can't use searchplugins/, and was trying to install and uninstall them manually in the same way, using this API. This is why we contributed the patch. We need the exact same error strings as Firefox, we just want to control where the errors appear.

Comment 31

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

Using Firefox's strings from your extension won't really be much code, so I don't think we need to optimize for that case.

Comment 32

[Ben Bucksch (:BenB)]

Even if so, I'd need to do string replacement, too, and that's per-string.
What about Seamonkey and Thunderbird? Would they have access to the strings?

Comment 33

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

We're talking about fewer than six strings here, this really isn't a big deal. SeaMonkey and Thunderbird already duplicate many more strings than that from Firefox and it's not a big issue.

Comment 34

[Ben Bucksch (:BenB)]

All I can say is that my experience with using Mozilla and non-Mozilla APIs is that APIs that didn't offer both error code and string were a pain to use.
You're the maintainer, I can't argue with you, just supply a patch and rationale. What you do with it is your choice, in the end.

Yours faithfully,
Ben

Comment 35

[Mark Banner (:standard8)]

Attachment: Bug 863474 - Centralise OpenSearch install failure prompts into SearchUIUtils. r?daleharvey!

Depends on D75566, D89652

Comment 36

[Mark Banner (:standard8)]

Attachment: Bug 863474 - Simplify callback handling for loading OpenSearch engines. r?daleharvey!

Depends on D89879

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42d66b1f9b82
Centralise OpenSearch install failure prompts into SearchUIUtils. r=daleharvey,flod
https://hg.mozilla.org/integration/autoland/rev/5f00eaaa4e2d
Simplify callback handling for loading OpenSearch engines. r=daleharvey

Comment 38

[Razvan Maries]

Backed out for build bustages.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=Mdpl3Y4IR424H73QTIRmqg.0&resultStatus=pending%2Crunning%2Ctestfailed%2Cbusted%2Cexception&revision=5f00eaaa4e2d3a530c2471244e99055deb9d78f0

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315534729&repo=autoland&lineNumber=1446

Backout: https://hg.mozilla.org/integration/autoland/rev/cb2f9ccd7089480ea0cd68944da325a65c8503f8

Comment 39

[Mark Banner (:standard8)]

I'd missed dropping search.properties from the android configuration, have updated the patch.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89a92f877c43
Centralise OpenSearch install failure prompts into SearchUIUtils. r=daleharvey,flod,geckoview-reviewers,esawin
https://hg.mozilla.org/integration/autoland/rev/bde38689e5d3
Simplify callback handling for loading OpenSearch engines. r=daleharvey

Comment 41

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/89a92f877c43
https://hg.mozilla.org/mozilla-central/rev/bde38689e5d3