586821 - Add affiliate URL queries to location bar search

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Alex Limi (:limi) — Firefox UX Team]

With the changes away from using BBN in bug 565966, we should make sure we have the same affiliate codes and locale-specific settings for these searches since they are now equivalent to the search bar searches (sans autocomplete).

In other words, whatever setup takes care of adjusting the URL query strings for the search bar should also apply to these, since they are equivalent now.

(And as stated in bug 565966, this has been cleared with legal)

Comment 1

[Kev Needham [:kev]]

I can take this.

Comment 2

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

The value of the "client" parameter for the search bar is currently generated at run-time, and depends on whether or not Google is the default engine for the given build (which depends on the locale). I'm assuming we don't want to change the meaning of the parameter.

A simple way of fixing this without having to worry about parameter differences or keeping multiple URLs up to date would be to have keyword.url fall back to the search service for obtaining the search URL, if it isn't specified. It could even add an additional param if we do want to add a way to differentiate the source of the searches. I can whip up a patch to do that pretty quickly.

Comment 3

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

Attachment: WIP

Like this. Makes us use exactly the same URLs for keyword.uri and search bar searches. Can be toggled on by omitting keyword.url in region.properties (which would have to be done in all locales where this behavior is wanted). Locales (e.g. ru) can still override by specifying a value. This has no support for a differentiation parameter - that'd be a bit trickier but still doable if it's valuable.

(I picked netwerk/ as a destination for the interface somewhat at random - I'm not sure that move is even needed nowadays.)

Comment 4

[Frank Yan (:fryn)]

Comment on attachment 465564 [details] [diff] [review]
WIP

Despite being marked WIP, it looks landable to me, hence the request for review. A differentiation parameter does not seem entirely necessary. Of course, correct me if I am wrong.

Comment 5

[Alex Limi (:limi) — Firefox UX Team]

I think we can land this without the differentiation parameter, we can file a follow-up for that. Would be great to have this in beta4, so we can make sure it works as intended, and have a beta to adjust if it doesn't.

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 465564 [details] [diff] [review]
WIP

The keyword.URL change looks like it needs a key change.

This is a pretty massive hit on the l10n-drivers team, as by default, any change that a localizer does to this file needs to go through a reviewed patch. Going for a different policy requires up-front communication on what the change is, and what kinds of those are OK.

Comment 7

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

(In reply to comment #6)
> The keyword.URL change looks like it needs a key change.

I was thinking that we'd just mass change all the l10n files manually (apart from ru which seems to intentionally use a different URI). I know you've had reservations about that in the past, but I think we've also done it in the past, so I'm not sure what the right tradeoff is here.

(Also probably better to just remove the entity entirely, rather than setting it to the empty string.)

> This is a pretty massive hit on the l10n-drivers team, as by default, any
> change that a localizer does to this file needs to go through a reviewed
> patch. Going for a different policy requires up-front communication on what
> the change is, and what kinds of those are OK.

region.properties changes already require l10n-driver approval, right? I'm not sure how this would change anything, unless you're just talking about the need to make the one-time mass change.

Comment 8

[Axel Hecht [:Pike]]

Patching l10n-central doesn't fix all the locales that we have in the review queue and everything. That's just adding a false feeling of "job done".

Why do we actually need keyword.URL at all? Can't we just use the default search engine?

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 465564 [details] [diff] [review]
WIP

r+a=jst. Please file followup bugs to track the remainder of this, including Axel's comment.

Comment 10

[Frank Yan (:fryn)]

I built with this patch, and it doesn't seem to work: when entering a non-url, it doesn't use the search engine but rather prefixes it with "http://www." and appends ".com" to it if it's a alphabetic word or simply returns the "url is not valid" error page.

What else needs to be done?

Comment 11

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

(In reply to comment #8)
> Why do we actually need keyword.URL at all? Can't we just use the default
> search engine?

Removing support for the pref entirely would break people who want to customize this behavior (relatively common for both users and re-distributors). It would also break the current "ru" behavior, which intentionally uses a different URL for search bar vs. location bar searches.

(In reply to comment #10)
> I built with this patch, and it doesn't seem to work: when entering a non-url,
> it doesn't use the search engine but rather prefixes it with "http://www." and
> appends ".com" to it if it's a alphabetic word or simply returns the "url is
> not valid" error page.

The patch works for me. Are you sure keyword searches work as you would expect without this patch applied (DNS hijacking can affect testing)? Can you step through nsDefaultURIFixup::KeywordToURI to figure out what's going on?

Comment 12

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

Removing the pref would also break Gecko users that don't have a search service. As a followup, we can move to using a separate URL type in the search engine description files to allow specifying an alternate "keyword URL".

Comment 13

[Frank Yan (:fryn)]

Attachment: patch (simply updates WIP to tip of tree; r+, a+ carries over)

This patch simply changes the diff for the one line in region.properties so that it applies cleanly. r+, a+ for WIP carries over.

Comment 14

[Jacek Caban]

http://hg.mozilla.org/mozilla-central/rev/5761fb5b379f

Comment 15

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

This isn't really "FIXED" - we need a followup to make the relevant changes for l10n builds as well. It would have probably also been best to remove the string entirely, rather than just change it's value, but I didn't get a chance to test that. Frank, can you do that?

Comment 16

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

Looks like removing the string doesn't work (we hit the fallback code for non-localized values).

Comment 17

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

I filed bug 587352 about l10n followups.