Closed Bug 441882 Opened 16 years ago Closed 13 years ago

keyword.URL should be synced with selected search engine

Categories

(SeaMonkey :: Search, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1b1

People

(Reporter: Manuel.Spam, Assigned: Manuel.Spam)

References

Details

Attachments

(3 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.13) Gecko/20080328 SeaMonkey/1.1.9
Build Identifier: 

keyword.enabled is on true by default in latest SeaMonkey builds, so users don't longer have to press the "cursor up" key first to be able to search directly from URL-Bar.

Reproducible: Always

Actual Results:  
Unfortunately this feature doesn't respect the user-selected search-engine.

Expected Results:  
User-selected search engine should be used
This is a first try to fix this one. If the user changes the search engine and "keyword.URL.sync" is true, then this service will create a valid keyword.URL.
Attachment #326762 - Flags: review?(neil)
This is the missing part to get the service enabled. It sets the pref to true and causes the build system to copy the file in place
Attachment #326765 - Flags: review?(neil)
Attachment #326762 - Attachment mime type: application/x-javascript → text/plain
Summary: keyword.URL should be synced with → keyword.URL should be synced with selected search engine
Assignee: general → Manuel.Spam
Version: unspecified → Trunk
Comment on attachment 326762 [details]
The service to link the two related prefs

>function KeywordSync() {
>    this.init();
Hmm, aren't you supposed to do this in the app-startup notification?

>        var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>                    .getService(Components.interfaces.nsIPrefBranch);
Nit: line up .getService with .classes

>        try {
>            if (!prefs.getBoolPref("keyword.URL.sync")) return;
>        } catch(e) { return; }
Shouldn't need try/catch here.
Should we observe keyword.URL.sync pref changes too?
Do we want UI for this pref?

>        // some old profiles still have the search engine URL in search.rdf
I don't think that's true any more, I think the .src file can contain an "actionButton" entry which specifies the search URL prefix directly, but (perhaps for compatibility?) it's mapped to the RDF assertion.

>        var kNC_Root = rdf.GetResource("NC:SearchEngineRoot");
Unused?

>        if (n) {
>            var s = n.QueryInterface(Components.interfaces.nsIRDFLiteral).Value;
Prefer if ((n instanceof Components.interfaces.nsIRDFLiteral) && n.Value)

>        var searchsvc = Components.classes["@mozilla.org/rdf/datasource;1?name=internetsearch"].getService(Components.interfaces.nsIInternetSearchService);
ds.QueryInterface(Components.interfaces.nsIInternetSearchService);
should also work.

>        var s = searchsvc.GetInternetSearchURL(curengine, "KeYwOrDsYnC", 0, 0, {value:0});
>        var parts = s.split(/[\?&]/);
>        var lparts = parts.length;
>
>        var url = parts[0] + "?";
>        var lastparam = false;
>        for (var index = 1; index < lparts; index++) {
>            var part = parts[index];
>            if (part.match(/KeYwOrDsYnC$/))
>                lastparam = part;
>            else
>                url += part + "&";
>        }
>        if (lastparam) {
>            url += lastparam.replace("KeYwOrDsYnC", "");
I think we should tweak the C++ to make this easier - I'm sure you'd prefer
var s = searchsvc.GetInternetSearchURL(curengine, "", 0, 0, {});

>    QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsISupports]),
IIRC generateQI already does nsISupports, did you mean nsIObserver?
(In reply to comment #3)
> Hmm, aren't you supposed to do this in the app-startup notification?

Yes. Changed that.

> Nit: line up .getService with .classes

Done

> Shouldn't need try/catch here.

I needed, as a pref-observer, defined in app-startup, is fired immediately, but *without* the profile-stored settings loaded! As I didn't have a default in my tests, this caused an exception. I moved the final init to "final-ui-startup" now. The required observer is initialized in a newly added "preinit" function. I also call SyncKeywordURL() on "final-ui-startup", now to be able to do an initial sync for new profiles.

> Should we observe keyword.URL.sync pref changes too?

IMHO not.

> Do we want UI for this pref?

No. There is no UI to edit the keyword.URL, so why should the user want to toggle this to false? The pref is just for power-users who want different settings for keyword.URL and search engine.

> I don't think that's true any more, I think the .src file can contain an
> "actionButton" entry which specifies the search URL prefix directly, but
> (perhaps for compatibility?) it's mapped to the RDF assertion.

I copied this code from navigator.js.

> >        var kNC_Root = rdf.GetResource("NC:SearchEngineRoot");
> Unused?

Yes --> deleted

> Prefer if ((n instanceof Components.interfaces.nsIRDFLiteral) && n.Value)

Sure that no "QueryInterface" is needed? Changed that, but I'm unsure if it really works.

> >        var searchsvc = Components.classes["@mozilla.org/rdf/datasource;1?name=internetsearch"].getService(Components.interfaces.nsIInternetSearchService);
> ds.QueryInterface(Components.interfaces.nsIInternetSearchService);
> should also work.

Doesn't work.

> I think we should tweak the C++ to make this easier - I'm sure you'd prefer
> var s = searchsvc.GetInternetSearchURL(curengine, "", 0, 0, {});

Sure? I think our own search service will be soon obsolete and I'm unsure if we get a fix that generates keyword.URL strings into the "toolkit search service".

... or is the new search service in the firefox codebase?

> IIRC generateQI already does nsISupports, did you mean nsIObserver?

Yes, changed.
Attached file Updated service
Attachment #326762 - Attachment is obsolete: true
Attachment #327239 - Flags: review?(neil)
Attachment #326762 - Flags: review?(neil)
If we drop the RDF part (is it still needed? If not, lets also delete it from navigator.js), then the whole service will get much smaller.

The loop, which resorts the search URL, may be replaced with this regular expression trick, which does resorting and matching all in once:

if (s.match(/^(.+[\?&])([^\?&]+)KeYwOrDsYnC(&?)(.*)$/)) {
    var r = RegExp;
    var url = r.$1 + r.$4 + r.$3 + r.$2;
    alert(url);
}

This way the whole thing gets even more smaller.

So the only questions, still open, is if we want to touch the C++ backend.
If we want to, another question is, how long we will keep with our own search backend until we port over to the new search backend already used in Firefox, where it IMHO may get difficult to get our fix in.
(In reply to comment #6)
> So the only questions, still open, is if we want to touch the C++ backend.
> If we want to, another question is, how long we will keep with our own search
> backend until we port over to the new search backend already used in Firefox,
> where it IMHO may get difficult to get our fix in.
If we switch backend then wouldn't we have to rewrite this service anyway?
Normally GetInternetSearchURL doesn't include the search parameter if you pass an empty string, although I don't think we have any callers that do that anyway. This patch makes it so that if you pass null then it will include a placeholder for the search parameter in the format that the keyword preference requires.
Attachment #328105 - Flags: review?(iann_bugzilla)
Attachment #328105 - Flags: review?(iann_bugzilla) → review+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(In reply to comment #6)
> The loop, which resorts the search URL, may be replaced with this regular
> expression trick, which does resorting and matching all in once:
> 
> if (s.match(/^(.+[\?&])([^\?&]+)KeYwOrDsYnC(&?)(.*)$/)) {
>     var r = RegExp;
>     var url = r.$1 + r.$4 + r.$3 + r.$2;
>     alert(url);
> }
> 
> This way the whole thing gets even more smaller.
I've checked in my C++ patch so you can now get a keyword URL by requesting a null (not "") search string from the internet search service.

I'd suggest leaving the RDF stuff in for now. Also, this works for me:
var rdf = Components.classes["@mozilla.org/rdf/rdf-service;1"]
                    .getService(Components.interfaces.nsIRDFService);
var ds = rdf.GetDataSourceBlocking("rdf:internetsearch");
...
ds.QueryInterface(Components.interfaces.nsIInternetSearchService);
Sorry for not responding for such a long time, but I'm unsure if we are on the right way and searched for an alternative.

The problem is, that the new search backend (which is already used by Firefox) is able to search using HTTP-POST and not just HTTP-GET. If we just place the search engine URL as keyword URL, then we would, again, limit this feature to HTTP-GET.

As a possible solution my idea was to create something like an "internal CGI". Means, that keyword.url would be set to something like "chrome://.../search.xul" which forwards to the real search engine. I also thought about a protocol handler to do this, but so far I was unable to find a real solution. Neither a Javascript/XUL in chrome context nor a protocol handler seems to be able to forward via HTTP-Post. The only idea would be a hack by creating a <form> based on the parameters and call "submit" on it.
Just for further reference:

>         this.pbi = Components.classes["@mozilla.org/preferences-service;1"]
>                              .getService(Components.interfaces.nsIPrefBranch)
>                              .QueryInterface(Components.interfaces.nsIPrefBranchInternal);

Shouldn't this be nsIPrefBranch2 instead of nsIPrefBranchInternal?

>         var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>                               .getService(Components.interfaces.nsIPrefBranch);
> 
>         if (!prefs.getBoolPref("keyword.URL.sync")) return;

How about just:
if (!this.pbi.getBoolPref("keyword.URL.sync")) return;

>         var searchsvc = Components.classes["@mozilla.org/rdf/datasource;1?name=internetsearch"].getService(Components.interfaces.nsIInternetSearchService);

nit?:
        var searchsvc = Components.classes["@mozilla.org/rdf/datasource;1?name=internetsearch"]
                                  .getService(Components.interfaces.nsIInternetSearchService);
Actually keyword.URL should not execute an ordinary search like it does in SeaMonkey 1 & 2, but redirect to a website like in Firefox via the "I'm Feeling Lucky"-search: http://www.google.com/search?ie=UTF-8&oe=UTF-8&sourceid=navclient&gfns=1&q=

Then a localization would not be that essential. Also we get the same results for a keyword independent of the localized build. So I vote for a change of keyword.URL to http://www.google.com/search?ie=UTF-8&oe=UTF-8&sourceid=navclient&gfns=1&q=
Keyword search should IMHO never go to a random website that happens to be the first on some random website, in my opinion that was a misdecision in Firefox already and we should not import it.

We should go to the default search engine though, ideally ignoring the old mycroft stuff and use the OpenSearch stuff as that's what we should switch to in 2.1 (and 2.0 will not see any more change there).
The target website is not that random, because it is the first result of the search-resultset which does not change so often.

I personally do not use the "Internet Keyword"-feature and I do not know much use it. However, it is currently broken as it is now. So changing the keyword.URL would be the quickest solution to see it in the final release of SeaMonkey 2.0.

As a long-term solution we can maybe agree on the following: "Internet Keyword"-feature is disabled by default and if you hit enter (and it is not an address) in the address-bar a search is executed. If the "Internet Keyword"-feature is enabled and you hit enter in the addressbar (and it is not an address) you are redirected to the first result of the search-resultset.
Just for a note, there won't be any changes here any more for SeaMonkey 2.0, all what we are discussing here can only be targeted for 2.1 now.
(In reply to comment #15)
> The target website is not that random, because it is the first result of the
> search-resultset which does not change so often.

It may change due to whoever pays for it => not a good idea.

> I personally do not use the "Internet Keyword"-feature

... and that's very visible in your line of arguments. ;-)

> However, it is currently broken as it is now.

It should sync with the default search engine, that's what this bug's about. 
Apart from that, it works perfectly well.

> As a long-term solution we can maybe agree on the following: "Internet
> Keyword"-feature is disabled by default and if you hit enter (and it is not an
> address) in the address-bar a search is executed. If the "Internet
> Keyword"-feature is enabled and you hit enter in the addressbar (and it is not
> an address) you are redirected to the first result of the search-resultset.

This has nothing to do with what "Internet Keyword" is about.
You want to implement a feature (which I, personally, doubt whether it's useful at all)...
> > > The target website is not that random, because it is the first result of the
> > > search-resultset which does not change so often.
> > 
> > It may change due to whoever pays for it => not a good idea.
 
Well, Netscape introduced the "Internet Keywords" and at this time Netscape operated
the server which responed to Internet Keywords searches so this wasn't maybe a topic
then. But as said Firefox is still using this feature with Google as searchprovider.

> It should sync with the default search engine, that's what this bug's about. 
> Apart from that, it works perfectly well.

The behaviour here with SeaMonkey 2 is that entering an Internet Keyword redirects
to a google.com-resultpage. Maybe upgrading from SeaMonkey 1 left some old prefs
still active, but unless you are not redirected automatically to the first page of
the resultset the feature it is broken.

> > As a long-term solution we can maybe agree on the following: "Internet
> > Keyword"-feature is disabled by default and if you hit enter (and it is not an
> > address) in the address-bar a search is executed. If the "Internet
> > Keyword"-feature is enabled and you hit enter in the addressbar (and it is not
> > an address) you are redirected to the first result of the search-resultset.
> 
> This has nothing to do with what "Internet Keyword" is about.
> You want to implement a feature (which I, personally, doubt whether it's useful
> at all)...

The "feature" would be that if Internet Keywords are disabled hitting enter would
initiate the (ordinary) search and not give an error-message instead of arrow-up
and enter. This would save one key-stroke.
Component: General → Search
QA Contact: general → search
Thankfully, this is much easier when we switch to OpenSearch, and my current bug 410613 patch includes the "code" (actually just setting keyword.URL to empty) to do this along with the technology switch.
fixed by bug 410613.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b1
Attachment #326765 - Flags: review?(neil)
Attachment #327239 - Flags: review?(neil)
You need to log in before you can comment on or make changes to this bug.