The current algorithm avoids search suggestions if: 1. the string is shorter than 2 chars 2. the first "token" is a whitelisted domain (like localhost) 3. if the string includes any of "/", "@", ":", "." 4. we are in a private window There are other cases we would like to handle, for example if the user typed a commonly used scheme like http, https, ftp. Any ideas for further cases we should handle are welcome.

as a side note, the "string" in 3. is actually the typed string minus "http://www.", since that's what the awesomebar usually looks for, and that is likely part of the problem here.

Marco/Florian, do you have any more ideas on top of those in comment 0?

No further ideas from me. What I meant in comment 1 is that the original code we added to avoid entries that may scare the user, is examining a filtered search string, that doesn't include http and similar prefixes (_searchString), while it should probably examine _trimmedOriginalSearchString. The reason I didn't originally do that, is that _trimmedOriginalSearchString may contain restriction chars, and if a restriction char overlaps with one of the chars that make the string look like a url (for example @ can be both part of a url or the restriction char for match.url) we may do the wrong thing. In practice the only restriction char that matters here is the one for searches (currently "$", even if I'd like to change it to "?"), since for any other restriction the suggestions would not be provided at all, so the problem is minor than I thought. http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/toolkit/components/places/UnifiedComplete.js#1270 Basically it's likely this bug would be partially fixed by testing the original typed string, rather than the stripped one, since then typing "http://test" would end up examinining *that* string, not "test". That's clearly not enough, considered we also want to exclude common schemes typed at the beginning of the string, but it would be a good start.

(In reply to Mark Banner (:standard8) from comment #2) > Marco/Florian, do you have any more ideas on top of those in comment 0? No additional idea; from my point of view http/https are the most important ones.

I've put something together based on matching the start of the url. I added lots of tests to make sure that we were covering everything, and we could see the before/after effects. Please test it out and see if it works as expected.

Comment on attachment 8866359 [details] Bug 1280441 - Improve some of the test documentation for unified complete. https://reviewboard.mozilla.org/r/138000/#review141540 ::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js:116 (Diff revision 1) > + * @param {Object} match The expected match for the result. > + * @param {nsIURI} match.uri The expected uri. > + * @param {String} match.title The title of the entry. > + * @param {String} match.tags The tags for the entry. > + * @param {String} match.style The style of the entry. > + * @param {Object} result The result to compare the result against with the same See the comment for check_autocomplete. ::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js:152 (Diff revision 1) > } > > +/** > + * Helper function to test an autocomplete entry and check the resultant matches. > + * > + * @param {Object} test The details of the test to run, wih the following items: s/items/properties/? ::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js:153 (Diff revision 1) > > +/** > + * Helper function to test an autocomplete entry and check the resultant matches. > + * > + * @param {Object} test The details of the test to run, wih the following items: > + * @param {String} test.search The string to enter for autocompleting. Not a great fan of this notation, since it looks like this takes multiple params. I'd prefer something like: @param {Object} test An object representing the test to run, in the following form: { search: {String} The string to.... param: {String} ... ... }

Comment on attachment 8866361 [details] Bug 1280441 - Avoid suggesting searches from the URL bar when they look like urls (http, https, ftp) or the start of urls. https://reviewboard.mozilla.org/r/138004/#review141542 ::: toolkit/components/places/UnifiedComplete.js:1322 (Diff revision 1) > + // like urls. > + if ((this._trimmedOriginalSearchString.length <= DISALLOWED_URL_PREFIXES_EXACT_MATCH_MAX_LENGTH && > + DISALLOWED_URL_PREFIXES_EXACT_MATCH.some(prefix => this._trimmedOriginalSearchString == prefix)) || > + DISALLOWED_URL_PREFIXES_START_MATCH.some(prefix => this._trimmedOriginalSearchString.startsWith(prefix))) { > + return true; > + } As per the brief IRC discussion, let's make this simpler, just exclude perfect matches for "http", "https", "ftp", and startsWith(same_as_before + ":") This will simplify the code and in the end it's what we care about. I don't think "ht", "htt" or "ft" cause any harm, they can be about totally different stuff than urls (as you suggested on IRC I may be willing to search for httpd).

Comment on attachment 8866359 [details] Bug 1280441 - Improve some of the test documentation for unified complete. https://reviewboard.mozilla.org/r/138000/#review141660 ::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js:112 (Diff revision 2) > -// A helper for check_autocomplete to check a specific match against data from > -// the controller. > +/** > + * A helper for check_autocomplete to check a specific match against data from > + * the controller. > + * > + * @param {Object} match The expected match for the result, in the following > + * form: nit: let this flow in the previous line, even if it goes slightly over 80 chars, we're not so strict :)

Comment on attachment 8866360 [details] Bug 1280441 - Add tests for what happens when existing http/https/ftp prefixes are entered. https://reviewboard.mozilla.org/r/138002/#review141662

Comment on attachment 8866361 [details] Bug 1280441 - Avoid suggesting searches from the URL bar when they look like urls (http, https, ftp) or the start of urls. https://reviewboard.mozilla.org/r/138004/#review141666 ::: toolkit/components/places/UnifiedComplete.js:106 (Diff revision 2) > const QUERYINDEX_TYPED = 7; > const QUERYINDEX_PLACEID = 8; > const QUERYINDEX_SWITCHTAB = 9; > const QUERYINDEX_FRECENCY = 10; > > +const DISALLOWED_URL_PREFIXES = [ this could probably be named better, but I have no fantasy! DISALLOWED_URLLIKE_PREFIXES would be (imo) a first trivial improvement. Also a comment above it explaining the expected behavior may help future archeology. ::: toolkit/components/places/tests/unifiedcomplete/test_search_suggestions.js (Diff revision 2) > - > - yield check_autocomplete({ > - search: "htt", > - searchParam: "enable-actions", > - matches: [ > - makeSearchMatch("htt", { engineName: ENGINE_NAME, heuristic: true }), why removing the "htt" test, it would be good to check we return suggestions for it.

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dca652d1fce1 Improve some of the test documentation for unified complete. r=mak https://hg.mozilla.org/integration/autoland/rev/27a97108e1a4 Add tests for what happens when existing http/https/ftp prefixes are entered. r=mak https://hg.mozilla.org/integration/autoland/rev/24bb92425086 Avoid suggesting searches from the URL bar when they look like urls (http, https, ftp) or the start of urls. r=mak

https://hg.mozilla.org/mozilla-central/rev/dca652d1fce1 https://hg.mozilla.org/mozilla-central/rev/27a97108e1a4 https://hg.mozilla.org/mozilla-central/rev/24bb92425086