341668 - Preload title into search engine confirmation dialog

Status: RESOLVED FIXED

(Firefox :: Search, enhancement, P3)

Description

[Pam Greene]

I'd like to load the full engine description, with a few seconds' timeout, before showing the dialog, so we always have a good title to display. That may involve another string or two (something like "Loading search-engine information..." for an interstitial dialog before the real one is available?).

(Split from bug 336452; see bug 336452 comment #9 and its replies for initial discussion.)

Comment 1

[Mike Connor [:mconnor]]

I don't think this is a blocker, especially since we're at the major string freeze today.  Would be nice to have, certainly, but we can still ship with just bug 336452 and still be ok.

Comment 2

[Pam Greene]

Would it be too odd to split the difference?  We could start fetching the description file right away; if it loads quickly (2-3 seconds?) then use its information in the dialog, otherwise fall back to what bug 336452 shows.

That would work well, and be simple, for what I expect is by far the majority of engines.  The downside is that it would give inconsistent results for flaky servers or slow connections: you'd get one title in the dialog when you had a quick link, a different one when you had a slow link.  On the other hand, any given user isn't likely to install the same engine many times.

Comment 3

[Mike Connor [:mconnor]]

I'm not sure about trying to do both, if its worth doing its worth doing "right"

That said, my main concern is that there are far more important parts of the user experience around search to fix still.

Comment 4

[Pam Greene]

(In reply to comment #3)
> I'm not sure about trying to do both, if its worth doing its worth doing
> "right"

Agreed, although I'm not sure what "right" is.

> That said, my main concern is that there are far more important parts of the
> user experience around search to fix still.

Bug numbers? :)

Comment 5

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

What are we doing now, waiting to get the title before displaying the dialog (as suggested in bug 336452 comment 12)? I like the idea of getting a proper title, and am wondering if it doesn't make sense to just wait on displaying the dialog until we actually have the full searchplugin and therefore know that we can complete the operation immediately when the user clicks. I think it might actually be more annoying for a user to click a "Install search plugin" button, see a confirmation dialog, and THEN have to wait for the install.

(btw, not getting blocking status doesn't mean that we wouldn't take a patch if you're bored! :)

Comment 6

[Pam Greene]

Okay, for the moment bug 336452 comment #12 will be my plan for this bug, should I get to it in time.

As for what we're doing right now, new search engines can be installed one of three ways:

If the add request engine is coming from our existing window.sidebar.addSearchEngine() function, the website has to provide a title in the function call.  That may not be the name the engine eventually has in the searchbox list, which comes from the OpenSearch or Sherlock description.

If it's coming from the new window.external.AddSearchProvider() function, we have no title at all until we load the description file.

And if the engine is autodetected from a <link rel...> tag, the author may or may not have provided a title, which again may not be the one the list eventually uses.

Right now we're using any title we have before loading the description file (from the old JS call or a title in the <link>); if we don't have a title, we're using "a new search engine".  In either case, we're also showing the hostname of the provider.  Bug 336452 has screenshots of examples.

Comment 7

[Pam Greene]

Attachment: Fixes issue

Confirmation before adding a search engine is moved from sidebar.js to nsSearchService.js and deferred until after the engine has loaded.  This also allows a more robust start-using-it-now implementation.  Autodetected engines from the searchbar menu are still added without additional confirmation.

Comment 8

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

Sorry, somehow my review comment got lost :( I'll write it up again.

Comment 9

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

Comment on attachment 228703 [details] [diff] [review]
Fixes issue

This is a good patch - I should have done it this way in the first place. I don't think we need to worry about the load taking a lot of time, since in general it should be pretty quick. This also has the effect of prompting about failures to load/parse the engine description file before prompting, which means that we won't prompt if we can't install the engine, and makes the loading errors consistent with the URL checks that we already do.

>+    // With tentative title: Add "Somebody's Search" to the list...
>+    // With no title:        Add a new search engine to the list...
>+    var engineName;
>+    if (this._name)
>+      engineName = stringBundle.formatStringFromName("addEngineQuotedEngineName",
>+                                                     [this._name], 1);
>+    else
>+      engineName = stringBundle.GetStringFromName("addEngineDefaultEngineName");

Since this function is only called after _initFromData, the engine is guaranteed to have a name, it would have already failed to load if it didn't. That means you can remove the "no name" case, and associated string (though see l10n comment below).

>+    const PROMPTSERVICE_CONTRACTID = "@mozilla.org/embedcomp/prompt-service;1";
>+    const nsIPromptService = Components.interfaces.nsIPromptService;
>+    ps =
>+      Components.classes[PROMPTSERVICE_CONTRACTID].getService(nsIPromptService);

Use the Cc/Ci notation here (should remove the need for the constants).

>+    var confirm = !ps.confirmEx(null,
>+                                titleMessage,
>+                                dialogMessage,
>+                                buttonFlags,
>+                                addButtonLabel,
>+                                "", "", // button 1 & 2 names not used
>+                                checkboxMessage,
>+                                checked);

Use null instead of "". Putting each argument on it's own line takes up a lot more vertical space than it needs to, but I guess if you find that more readable that's OK.

>+    // If requested, confirm the addition now that we have the title.
>+    var confirmation;
>+    if (aEngine._confirm) {
>+      confirmation = aEngine._confirmAddEngine();
>+      LOG("_onLoad: confirmation " + confirmation.confirmed + ", use now " + confirmation.useNow);
>+    }
>+    if (aEngine._confirm && !confirmation.confirmed)
>+      return;

Put the return in the first |if (aEngine._confirm)| block, to avoid checking aEngine._confirm twice?

>+    if (aEngine._confirm && confirmation.useNow)
>+      notifyAction(aEngine, SEARCH_ENGINE_LOADED_USE_NOW);
>+    else
>+      notifyAction(aEngine, SEARCH_ENGINE_LOADED);

I think I'd rather you use a property on the engine object (_useNow?) and have |observe| check that, instead of adding another observer topic. The search engine update patch will do something similar.

>Index: locales/en-US/chrome/browser/search.properties
>Index: locales/en-US/chrome/browser/sidebar/sidebar.properties

You should probably ask Axel if it would be better to just leave the strings in sidebar.properties for now, on the branch. Even if they don't change, moving them requires all localizations to update their builds, and _confirmAddEngine could just use sidebar.properties for the moment.

Comment 10

[Pam Greene]

(In reply to comment #9)

> I think I'd rather you use a property on the engine object (_useNow?) and have
> |observe| check that, instead of adding another observer topic.

I went back and forth on that one myself -- even had a patch using the other method for a while.  I finally ended up doing it this way because it seemed inefficient to keep a _useNow property on every engine object just so it could be used momentarily by the rare engine that was in the middle of being added to the list.  Are there other arguments in either direction that I'm missing?

(I've made the other changes you mentioned, and will upload a new patch when this question and the l10n one are resolved.)

Comment 11

[Pam Greene]

Axel -- 

As it is currently written, this patch would move 5 strings from sidebar.properties to search.properties and delete one unused string from sidebar.properties.  Would it be better to simply leave them all alone, use the 5 from sidebar.properties, and ignore the one that's no longer needed?

Comment 12

[Axel Hecht [:Pike]]

(In reply to comment #11)
> Axel -- 
> 
> As it is currently written, this patch would move 5 strings from
> sidebar.properties to search.properties and delete one unused string from
> sidebar.properties.  Would it be better to simply leave them all alone, use the
> 5 from sidebar.properties, and ignore the one that's no longer needed?
> 

I'd say leave the strings as is for the branch, and do the cleanup in a follow-up bug on the trunk. That's likely the smoothest path to test the patch on the trunk before landing it on the branch, without causing distress on the l10n front on the branch. Thanks for asking.

Comment 13

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

(In reply to comment #10)
> I went back and forth on that one myself -- even had a patch using the other
> method for a while.  I finally ended up doing it this way because it seemed
> inefficient to keep a _useNow property on every engine object just so it could
> be used momentarily by the rare engine that was in the middle of being added
> to the list.  Are there other arguments in either direction that I'm missing?

I'd say the cost of keeping a property on the JS object is pretty much negligible, and you don't need to keep it on all objects (just don't define it on the object's prototype). The observer topics are for the most part something meant to be used by code external to the search service - SEARCH_ENGINE_LOADED is the exception. I was trying to avoid adding another exception.

If you still want to use the new topic approach, then I think you should remove the SEARCH_ENGINE_LOADED #define and comment from nsIBrowserSearchService.idl, and then clearly differentiate _LOADED and _LOADED_USE_NOW from the other topics declared in nsSearchService.

Comment 14

[Pam Greene]

Attachment: Patch addressing Gavin's and Axel's comments

Okay, you've convinced me.  This patch uses engine._useNow alongside the similarly-purposed engine._confirm.  And the strings are left in sidebar.properties, to be cleaned up on the trunk in bug 344437.

Comment 15

[Pam Greene]

Attachment: Slightly cleaner patch

Comment 16

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

Comment on attachment 229016 [details] [diff] [review]
Slightly cleaner patch

>Index: components/search/nsSearchService.js

>       aEngine._initFromData();
>     } catch (ex) {
>       LOG("_onLoad: Failed to init engine!\n" + ex);
>       // Report an error to the user
>       onError();
>       return;
>     }
> 
>+    // If requested, confirm the addition now that we have the title.
>+    if (aEngine._confirm) {
>+      var confirmation = aEngine._confirmAddEngine();
>+      LOG("_onLoad: confirm is " + confirmation.confirmed +
>+          "; useNow is " + confirmation.useNow);
>+      if (!confirmation.confirmed)
>+        return;
>+      aEngine._useNow = confirmation.useNow;
>+    }

I just noticed that canceling the engine load after calling _initFromData is problematic - _initFromData gives the engine a file (it calls getSanitizedFile if this._file is null). This means that after pressing "cancel" in the dialog, either a blank file will be left in the profile, or if the icon load callback runs and causes a _serializeToFile, the engine will be written to disk and loaded on the next startup. I think the solution to this is to move the |if (!this._file)| check in _initFromData to _onLoad, and put it after the _confirmAddEngine call but before _serializeToFile().

>-          // Optionally start using this engine now.
>-          if (this._selectNewEngineURI &&
>-              this._selectNewEngineURI == engine.wrappedJSObject.uri) {
>+          if (engine.wrappedJSObject._useNow) {
>+            LOG("nsISearchEngine::observe: setting current");

Oops, the other comments in this function are wrong, this is nsIBrowserSearchService::observe, not nsISearchEngine::observe. Could you change them both to |SearchService.observe|?

Comment 17

[Pam Greene]

Attachment: Patch addressing Gavin's comments

Comment 18

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

Comment on attachment 229156 [details] [diff] [review]
Patch addressing Gavin's comments

>Index: components/search/nsSearchService.js

>+    // If we don't yet have a file (i.e. we instantiated an engine object from
>+    // a URL passed in), get one now.
>+    if (!aEngine._file)
>+      aEngine._file = getSanitizedFile(aEngine.name);

aEngine._file will always be null here, so you can remove the check. Since we're in _onLoad, we're guaranteed to be in the "instantiated from a passed in URL" case. It was necessary when it was in _initFromData because it had other callers.

>-   * Initialize this Engine object from the collected data.
>+   * Initialize this Engine object from the collected data.  Initialization of
>+   * this._file is deferred until we're ready to actually save it to the file
>+   * (in _onLoad), in case the user cancels adding the engine.

I don't think this comment is really relevant to this function. If you want to add it, I'd say put it before the assignment to ._file in _onLoad.

r=me with those changes.

Comment 19

[Pam Greene]

Attachment: Patch addressing Gavin's comments

Comment 20

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 229172 [details] [diff] [review]
Patch addressing Gavin's comments

r=[e-mail]

Comment 21

[Pam Greene]

Landed on trunk.  Bake until edges are light green, cool completely and serve.

Comment 22

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

Comment on attachment 229172 [details] [diff] [review]
Patch addressing Gavin's comments

Thanks Pam, looks tasty! a=drivers, please land away on the 1.8.1 branch

Comment 23

[Pam Greene]

Landed on branch