420328 - Location bar doesn't use google 'I'm feeling lucky' if the first word typed is a keyword (breaking change)

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Darren]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3

---
I've marked this major because the "I'm feeling lucky" auto-search is IMHO, a major feature and the current behavior departs from previous versions of FF.
---

See steps to reproduce, which should be self-explanatory.

Reproducible: Always

Steps to Reproduce:
1. Pick two words (or more) and type them into the location bar, noting the page you come up with - should be a google search results page or a website;
2. Assign the first word to point to a page *other than* the one you end up at in step 1;
3. Type the two (or more) words into the location bar again.
Actual Results:  
You are directed to the bookmarked page.

Expected Results:  
In earlier FF versions you will still go to the google search result. This is expected because you haven't just typed the keyword, you have typed in something else that just happens to contain the keyword.

The expected results can be gotten by swapping the order of words.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008030100 Minefield/3.0b4pre

When I type amo tab in the locationbar it does a search on https://addons.mozilla.org/en-US/firefox/search?q=&status=4 for the word tab.
It does this as long as I can remember, because this is the function of the keyword amo. Do you expect a google "I'm feeling lucky search" for amo tab?

Comment 2

[Darren]

I wasn't explicit enough in my explanation, sorry.

The behaviour you describe is correct for keyword _searches_, but I'm talking about straight bookmarks that have a keyword assigned to them. In other words, the keyword will never take arguments (like with a keyword search) because they are meaningless.

In earlier versions of FF the location bar recognises this fact and takes a query of "keyword" and "keyword anotherkeyword" as completely separate things. The beta takes the latter term as just "keyword" and ignores the existence of the extra word.

Apologies for that explanation being so roundabout.

Comment 3

[Ria Klaassen (not reading all bugmail)]

Confirmed with latest trunk on Windows XP. 
Regression range is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1182936900&maxdate=1182974219
Bug 378553 seems to be the most likely cause. Bug 419855 seems to have been a similar bug but has a resolution. 

Comment 4

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

Attachment: patch

As far as I can tell, the existing |} else| branch is useless - it would only take effect for bookmark keywords that have postData, but that don't have a replaceable string in that postData or in the URI. I don't think it's ever been possible to add such a bookmark (since the "add keyword for this search" code always passes in a %s), and even if it somehow was, sending the postData when we didn't before shouldn't really hurt anything.

So I'm modifying that branch to catch this case - a keyword that takes no parameter, but that was given one anyways. We'll just return the original "URL" in that case (really a multi-word string).

The test depends on bug 420549, since removing the engine shortly after adding causes bad stuff to happen. If that doesn't land before this patch I'll comment out the engine removal code until it does (the profile gets deleted on each unit test run anyways, I just wanted to be sure that the added engines wouldn't affect other tests).

Comment 5

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

Attachment: fix postData test

Comment 6

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Comment on attachment 306859 [details] [diff] [review]
fix postData test

(In reply to comment #4)
> As far as I can tell, the existing |} else| branch is useless - it would only
> take effect for bookmark keywords that have postData, but that don't have a
> replaceable string in that postData or in the URI. I don't think it's ever been
> possible to add such a bookmark (since the "add keyword for this search" code
> always passes in a %s), and even if it somehow was, sending the postData when
> we didn't before shouldn't really hurt anything.
I vaguely remember adding it in to cover that case only because I hit it while doing testing by hand. I agree that we really shouldn't hit that in practice and that it's fine to repurpose it for this.

++ for the excellent test coverage :)

Comment 7

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

mozilla/browser/base/content/browser.js 	1.990 	mozilla/browser/base/content/test/Makefile.in 	1.8 	mozilla/browser/base/content/test/browser_getshortcutoruri.js 	1.1