453120 - With no parameter, parameter-accepting keyword item in awesomebar has the keyword itself substituted for %s

Status: NEW

(Toolkit :: Places, defect, P3)

Description

[7842.9]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080831162801 Minefield/3.1b1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080831162801 Minefield/3.1b1pre

With no parameter, parameter-accepting keyword item in awesomebar has the keyword itself substituted for %s. However, just pressing enter loads the url with %s replaced with nothing. Is this intended behavior?

Reproducible: Always

Steps to Reproduce:
1. Add bookmark http://en.wikipedia.org/wiki/%s with keyword of w.
2. Type w in awesomebar, and select the keyword item in the dropdown and press enter. You end up at http://en.wikipedia.org/wiki/W
3. Type w in awesomebar again and just press enter. You end up at http://en.wikipedia.org/wiki/Main_Page

Comment 1

[7842.9]

Also, typing something after the keyword (like w LHC) makes this work normally, i.e. pressing enter or selecting the keyword item both do the normal keyword search.

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch

Comment 4

[Marco Bonardo [:mak]]

I think the idea is correct, there's no reason to search keyword for keyword.

But looks like there's another bug there around (or that code was written like that to workaround something), without your patch as soon as i type "keyword a" i see "Search keyword: a" suggestion. with your patch i have to write "keyword abstract" and then back space to see any suggestion. sounds like in this case we are searching till the popup contains any suggestion, so we are searching in current results (repeating keyword search). Adding some debug output i see that AutoCompleteKeywordSearch() is not even called, most likely because we try to search in current results.
Current behavior makes so that first chunk always returns a result, and we requery for keywords on every change.

CC-ing Mardak that knows this code fairly good.

Comment 5

[Marco Bonardo [:mak]]

PS: it is quite interesting that our unit tests still pass, so would be better to look into them.

we stop here:

    // Got nothing before? We won't get anything new, so stop now
    if (!mAutoCompleteEnabled ||
        (mAutoCompleteFinishedSearch && prevMatchCount == 0)) {

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 379581 [details] [diff] [review]
patch

removing request till comment 4 is solved or clarified.

Edward could you please comment about that?

Comment 7

[Ed Lee :Mardak]

Comment on attachment 379581 [details] [diff] [review]
patch

There's a good reason why there aren't any tests for replacing %s with the keyword -- it's not a defined functionality for what keywords should do. I'm not even sure what functionality we want right now anyway.

If you have a "key %s" search, don't show it if you just type "key", don't show anything? How about if you type "key "? "key K" we would show ?..=K for sure.

How about a plain "key" keyword? "key" -> url, "key "? "key K" ?

>diff --git a/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp b/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp
>--- a/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp
>+++ b/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp
>@@ -905,29 +905,31 @@ nsNavHistory::ProcessTokensForSpecialSea
> nsNavHistory::AutoCompleteKeywordSearch()
> {
>   mozStorageStatementScoper scope(mDBKeywordQuery);
> 
>-  // Get the keyword parameters to replace the %s in the keyword search
>-  nsCAutoString params;
>-  PRInt32 paramPos = mOrigSearchString.FindChar(' ') + 1;
If we were to make this change, might as well just do "if (paramPos == 0) return;" instead of shifting everything around.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> If you have a "key %s" search, don't show it if you just type "key", don't show
> anything?

Right, because any concrete suggestion will be misleading, as per this bug. Even if %s is replaced with an empty string, the result will be useless in most cases.

> How about if you type "key "?

Same thing. I think mOrigSearchString was already normalized or something like that, so I didn't have to special-case trailing spaces...

Comment 9

[Ed Lee :Mardak]

Right, so the code addition of if (pos == 0) return; should work.

Comment 10

[Dão Gottwald [:dao]]

I found that less straightforward, so I reorganized it. pos == 0 usually means it's the first character rather than not found.

Comment 11

[Dão Gottwald [:dao]]

Ed, any insight regarding comment 4?

Comment 12

[Ed Lee :Mardak]

I don't quite understand what comment 4 is saying..

But we want to show keyword results immediately if we have match a keyword and the user types something to search. Additionally, we still want to show history suggestions that match the keyword and extra search params too.

Every time you type a character, it should be resetting the chunk offset so it triggers both keyword and adaptive searching. If we have a previous query, then it'll filter out those results before returning to normal history search.

Comment 13

[Marco Bonardo [:mak]]

Edward, what  i'm saying in comment 4 (or trying to say) is that with this change we won't search keywords anymore if we don't have a param. Thats somehow expectable, bug after searching first chunk we won't find anything (we don't search keyword since we have no pram). Since (mAutoCompleteFinishedSearch && prevMatchCount == 0) we won't search anymore for keywords even if you continue typing all keyword and add a param.

Comment 14

[Marco Bonardo [:mak]]

s/bug/but/ and s/pram/param/, i should type slowly.

Comment 15

[Ed Lee :Mardak]

Ah, I see. If your history is small enough and it finishes searching before you type out the param and end up with no results, it'll optimize and not search anymore even if you type more.

I don't think I ran into that and coded it that way. In fact, there's a related bug if you have a long name for the keyword search say... "k3yw0rd" and you search for "k3yw0r" and find nothing.. typing the "d" won't trigger a keyword search either. But I suppose that's less common as keywords are typically short.. (And I usually name my keyword as something that matches the site in a history search anyway, so I always get results.)