After converting tests and getting a review pass, we should be able to enable the component in Nightly (by flipping a pref)

Here we should also ensure that once enabled it won't break mochitest-browser tests, especially switch-to-tab ones may depend on results sorting.

Tests are passing, this also fixes a minor glitch in the component with switch to tab styling and adds a small improvement to avoid duplicate uris in results. I added tests for both since we were missing them. It's easy to disable the component through the pref, so I think the sooner we can enable it, the better. (Clearly we still need the dependencies to land first).

The right one :/

Comment on attachment 8447151 [details] [diff] [review] patch v2 Review of attachment 8447151 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/UnifiedComplete.js @@ +441,5 @@ > + * The text to modify. > + * @return the modified spec. > + */ > +function stripHttpAndTrim(spec) > +{ Nit: opening brace should be on the same line as "function" @@ +737,5 @@ > } > > // Must check both id and url, cause keywords dinamically modify the url. > if ((!match.placeId || !this._usedPlaceIds.has(match.placeId)) && > + !this._usedURLs.has(stripHttpAndTrim(match.value))) { So you remove stripPrefix() in the bug that fixes the tests and re-add a slightly different version here? Wouldn't it make sense to put that fix there? @@ +746,5 @@ > // include the search string, and would be returned multiple times. Ids > // are faster too. > if (match.placeId) > this._usedPlaceIds.add(match.placeId); > + this._usedURLs.add(stripHttpAndTrim(match.value)); You could call stripHttpAndTrim() only once before the if-branch and save that to a variable. ::: toolkit/components/places/tests/unifiedcomplete/test_dupe_urls.js @@ +1,3 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ Nit: should be the public domain header. @@ +12,5 @@ > + yield check_autocomplete({ > + search: "moz", > + autofilled: "mozilla.org/", > + completed: "mozilla.org/", > + matches: [ { uri: NetUtil.newURI("http://mozilla.org/"), title: "mozilla.org/" } ] Nit: trailing white space

(In reply to Tim Taubert [:ttaubert] (away July 7th-18th) from comment #5) > @@ +737,5 @@ > > } > > > > // Must check both id and url, cause keywords dinamically modify the url. > > if ((!match.placeId || !this._usedPlaceIds.has(match.placeId)) && > > + !this._usedURLs.has(stripHttpAndTrim(match.value))) { > > So you remove stripPrefix() in the bug that fixes the tests and re-add a > slightly different version here? Wouldn't it make sense to put that fix > there? the final result is the same, since the test is here I left it here, this version is better than the original one.

I'm going to push everything here but the pref flip. I don't think it's sane to enable this just a few days before the merge, I'll push the pref flip just after we merge. So, I'm renaming this and filing a new bug just for the pref flip.

this one becomes QA-, I'm setting QA+ in bug 1040335