1551049 - review search suggestion filtering criteria added in bug 1184960

Status: NEW

(Firefox :: Address Bar, task, P3)

Description

[Mike Connor [:mconnor]]

High level, I think most of this is pretty smart, and I'm glad we made the changes. However, I think we should look at whether we can be a little more nuanced around when and how we determine what is and isn't a search vs. a URL.

As a few (English-centric) examples:

"Google vs." (compare to "Google vs")
"5.8 cm"
"a/b testing"
"he/him"

All of these work much better in Chrome. We shouldn't break legitimate search terms like these.

Comment 1

[Harry Twyford]

Marco points out in this review that we should also allow searching for strings that start with "@" and consider it in this bug. This would be valuable when searching for Twitter handles. This is a little more difficult than just removing the early return Marco is commenting on in the review. Right now, UnifiedComplete handles searching for token alias engines when the "@" symbol is entered. We probably don't want search engine suggestions mixed in with token alias engine results, and we don't have a good way of restricting UrlbarProviderSearchSuggestions based on results from UnifiedComplete other than clunky handling in the Muxer.

If we implemented bug 1628065, UrlbarProviderTokenAliasEngines could be a restricting provider until it narrows down its results to one engine. At that point, we could start returning suggestions from that engine even if the query starts with "@".

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I was indeed pointing out that the comment saying we skip because it's URL-like it's a bit confusing, if the reason is tokens suggestions we should update the comment.

Comment 3

[Harry Twyford]

From Marco's review on my patch for bug 1628079:

I think we should go back to the whiteboard with Connor, and figure out the answer to "Which strings we don't want to fetch suggestions for", and if the answer is that the whole search string should not look like a url, then we just use URIFixup and throw away these other checks, otherwise we'll have to additionally check tokens like we do here (but maybe in a less strict way?). In other words, I think we must invert the question in bug 1551049 and understand which privacy hits we're concerned about.

Engineering should meet with Product and discuss what kinds of searches we don't want to fetch suggestions for so we can decide how to toggle UrlbarProviderSearchSuggestions.isActive. That might enttail using URIFixup, which is the scope of bug 1628079.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

After the recent changes in bug 1628079 all the entries in comment 0 work.
Comment 1 is still potentially open but with a much lower priority.
We should also eventually evaluate if we want to exclude more cases.

Comment 5

[Johnny Callueng]

Attachment: same problem

Thanks from apderoxu1987.