This is a followup bug for the inline autocomplete work in bug 566489. We need a few browser chrome tests that make sure we don't regress this new code.

no need for b-c tests, we can do xpcshell tests like the ones in toolkit/components/places/tests/autocomplete/

(In reply to Marco Bonardo [:mak] from comment #1) > no need for b-c tests, we can do xpcshell tests like the ones in > toolkit/components/places/tests/autocomplete/ Marco: Can you describe the tests you need - its been a few weeks and I am hazy on what they need to do.

functional tests for autocomplete, add urls and check that they are reported in the correct order, that http:// and www. are ignored, that autocompleting after an existing host completes to the url and so on.

Saving work. mak: how does this look?

Comment on attachment 591650 [details] [diff] [review] v 1 saving work Review of attachment 591650 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/tests/inline/test_autocomplete_functional.js @@ +11,5 @@ > +{ > + this.transitionType = > + aTransitionType === undefined ? TRANSITION_LINK : aTransitionType; > + this.visitDate = aVisitTime || Date.now() * 1000; > +} you can move these utils to the head_autocomplete.js file @@ +19,5 @@ > + > +add_autocomplete_test([ > + "Add urls, check for correct order", > + "moz", > + "moz", this looks wrong :) btw, inline searches on the start of a domain (www excluded) so here you should likely search for "visit" or "vis" to get matches, not moz the exepected value should be "visit2.mozilla.org" cause it's typed (for example) @@ +49,5 @@ > + do_throw("gHistory.updatePlaces() failed"); > + }, > + handleCompletion: function () { > + ensure_results("visit", "visit2.mozilla.org"); > + run_next_test(); you should not run_next_test, the test harness takes care of that for you. @@ +52,5 @@ > + ensure_results("visit", "visit2.mozilla.org"); > + run_next_test(); > + } > + }); > + } You may add a addVisits util to head_autocomplete.js taking an array of objects with infos you need.

Let me know about further checks to make. I will play around with inline autocomplete later today and see what else i come up with.

Comment on attachment 591853 [details] [diff] [review] v 2 patch Review of attachment 591853 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/tests/inline/test_autocomplete_functional.js @@ +36,5 @@ > + }; > + places.push(place); > + }); > + > + gHistory.updatePlaces(places, FAKE_CALLBACK); please wrap all of this in an addVisits helper function in head_autocomplete that just gets your urls array as input. you don't need FAKE_CALLBACK just do gHistory.updatePlaces(places); @@ +102,5 @@ > + > +// 4. search for www.me should yield www.me.mozilla.org/ > + > +add_autocomplete_test([ > + "Autocompleting after an existing host completes to the url", use a different description in any test, if not possible add an increasing number to the end, just to distinguish tests in the output @@ +125,5 @@ > + places.push(place); > + }); > + > + gHistory.updatePlaces(places, FAKE_CALLBACK); > + } Add some test that adds a visit to a page and a bookmark to another one, and check the bookmark is returned Also add tests for urls with a path, not just the domain, we autocomplete to the next slash depending on the search string

Not sure why I was getting this error earlier: ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'JavaScript component does not have a method named: "handleResult"' when calling method: [mozIVisitInfoCallback::handleResult]" nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)" location: "<unknown>" data: no] ************************************************************ ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'JavaScript component does not have a method named: "handleCompletion"' when calling method: [mozIVisitInfoCallback::handleCompletion]" nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)" location: "<unknown>" data: no] ************************************************************ It is gone now though. Addressed review comments.

Comment on attachment 591877 [details] [diff] [review] v 3 Patch Review of attachment 591877 [details] [diff] [review]: ----------------------------------------------------------------- Once these are addressed, and it passes, I'd say it'd be pretty good. We can add more tests while we proceed. ::: toolkit/components/places/tests/inline/head_autocomplete.js @@ +27,5 @@ > + aTransitionType === undefined ? TRANSITION_LINK : aTransitionType; > + this.visitDate = aVisitTime || Date.now() * 1000; > +} > + > +function add_visits(aUrls) just for coherence with addBookmark, make this addVisits @@ +30,5 @@ > + > +function add_visits(aUrls) > +{ > + let places = []; > + aUrls.forEach(function(url) { nit: add a whitespace after "function" for anonymous functions @@ +38,5 @@ > + visits: [ > + new VisitInfo(url.transition), > + ], > + }; > + places.push(place); nit: you may avoid making the temp var and just places.push({ bla bla }); ::: toolkit/components/places/tests/inline/test_autocomplete_functional.js @@ +15,5 @@ > + { url: NetUtil.newURI("http://visit2.mozilla.org"), > + transition: TRANSITION_TYPED, > + }]; > + > + add_visits(urls); global-nit: you may avoid the newline before addVisit, or even directly passing the array as addVisits([ { ]); whatever, I don't mind @@ +25,5 @@ > + "visit1", > + "visit1.mozilla.org/", > + function () { > + > + let urls = [{ url: NetUtil.newURI("http://www.visit1.mozilla.org"), no newline after function() opening please, at a maximum if you want to increase redability move the opening brace to a new line @@ +70,5 @@ > + "bookmark", > + "bookmark.mozilla.org/", > + function () { > + > + let urls = [{ url: NetUtil.newURI("http://bookmark.mozilla.org/foo"), to better differentiate here you should maybe use bookmark1.mozilla.org, otherwise you can't tell what is returned (we always crop at the first / after your search string) @@ +114,5 @@ > + ]; > + > + add_visits(urls); > + } > +]); add a test that wants to complete the query string after a ? and maybe even one with a # fragment

Comments addressed, all checks pass

Forgot to hg qref... mak: can you look at the last test that checks for #fragment ?

Comment on attachment 591931 [details] [diff] [review] v 4.1 Patch Review of attachment 591931 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/tests/inline/head_autocomplete.js @@ +36,5 @@ > + uri: url.url, > + title: "test for " + url.url, > + visits: [ > + new VisitInfo(url.transition), > + ], indent just 2 spaces as usual all these properties ::: toolkit/components/places/tests/inline/test_autocomplete_functional.js @@ +26,5 @@ > + "visit1.mozilla.org/", > + function () > + { > + > + let urls = [{ url: NetUtil.newURI("http://www.visit1.mozilla.org"), please avoid these newlines? @@ +133,5 @@ > +]); > + > +add_autocomplete_test([ > + "Check to make sure we autocomplete after #", > + "smokey.mozilla.org/foo?bacon=delicious#bar", I'd probably search up to ba and let it complete to bar Does it fail, why should I especially look at it?

(In reply to Marco Bonardo [:mak] from comment #12) > Comment on attachment 591931 [details] [diff] [review] > Does it fail, why should I especially look at it? No failures. Just added another check and wanted you to peek at it before landing.

ping? can this land?

(In reply to Marco Bonardo [:mak] from comment #14) > ping? can this land? yes indeed.

This can be checked into m-c

Thank you https://hg.mozilla.org/integration/mozilla-inbound/rev/d34882ca9d04