I plan on doing sync inline autocompleting by conducting a separate, simultaneous autocomplete search. To do this, though, we need to allow result objects to request that they not be added to the input's popup (so they can be completed within the input, but not become a separate row).

This patch is built on top of the one in bug 660156.

Can we get a test to ensure that these do not show up in the UI please too?

The test is already designed to make sure it doesn't appear in the UI, but just in case I added another check. Are you able to review this and bug 660156 before your leave? If not, would Edward Lee be able to take over?

(In reply to comment #3) > Are you able to review this and bug 660156 before your leave? If not, would > Edward Lee be able to take over? Unlikely given that we don't have a concrete UX spec for bug 660156 yet. You'd need a toolkit peer, but getting feedback from Edward first on that bug is a good idea since he has the most experience with that use case.

Comment on attachment 536537 [details] [diff] [review] Patch 2 Review of attachment 536537 [details] [diff] [review]: ----------------------------------------------------------------- This approach looks fine, but I'm interested in seeing the test harness improved so it's 1) easier to review new tests and see what's actually being tested in a simpler manner and 2) easier to add new tests without copying and pasting a bunch of code. You are clearly writing a lot of tests for this module (and that's a good thing and appreciated). ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp @@ +1271,5 @@ > } > > + PRBool hiddenResult = PR_FALSE; > + if (aResult) > + aResult->GetHidden(&hiddenResult); global nit: brace all ifs @@ +1276,5 @@ > + > + if (!hiddenResult) { > + PRUint32 oldRowCount = mRowCount; > + // If the search failed, increase the match count > + // to include the error description nit: this should not wrap before 80 characters if it doesn't need to (and include punctuation please) ::: toolkit/components/autocomplete/nsIAutoCompleteSimpleResult.idl @@ +76,5 @@ > > /** > + * A writer for the readonly attribute 'hidden'. > + */ > + void setHidden(in boolean aHidden); Looking at how this works more, the whole point of this is to be used for type-ahead completion, right? Assuming that's true, I think setAsTypeAheadResult would be better here. ::: toolkit/components/autocomplete/tests/unit/test_hiddenResult.js @@ +66,5 @@ > + > + // nsISupports implementation > + QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports, > + Ci.nsIAutoCompleteInput]) > +} OK, so there's a lot of code here that is the same in every test. Can you pull it out into a head_ file please? @@ +121,5 @@ > + > + // nsISupports implementation > + QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports, > + Ci.nsIAutoCompleteResult]) > +} Same with this. We can create a common base object here, and just inherit from it: AutoCompleteResult.prototype = Object.create(ACResultBase.prototype); AutoCompleteResult.prototype.method = ... @@ +176,5 @@ > +/** > + * Helper to register an AutoCompleteSearch with the given name. > + * Allows the AutoCompleteController to find the search. > + */ > +function registerAutoCompleteSearch(aSearch) { This should be pulled into a head file too. @@ +196,5 @@ > + > + > + > +/** > + * Helper to unregister an AutoCompleteSearch. As well as this.

This should fix all the comments.

I needed to make some small changes to this patch while working on the overall feature. For example, replacing an NS_ENSURE_TRUE with a simple return to prevent a warning printed to the console for a harmless (and sometimes, expected) condition.

Comment on attachment 540399 [details] [diff] [review] Patch 4 Review of attachment 540399 [details] [diff] [review]: ----------------------------------------------------------------- You are free to ignore nits, other comments should be addressed or answered. The patch seems contained, the only "big" change is that CompleteDefaultIndex is invoked after popup is opened/closed rather than before, but I've not found relations between the two calls, so should not hurt. I mostly don't like the "hidden" naming, and the test has some broken assignment. ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp @@ +1436,5 @@ > result->GetDefaultIndex(&defaultIndex); > } > + if (defaultIndex < 0) { > + return NS_ERROR_FAILURE; > + } may you add a small comment inside the if, about when this is expected to happen, so we avoid someone in future adding back the warning. It's a bit unclear to me so far which change in your patch touched this, can we fix the caller too? @@ +1586,3 @@ > > + // Skip past the result completely if it is marked as hidden > + PRBool hiddenResult; init to PR_FALSE, some compiler is too picky ::: toolkit/components/autocomplete/nsIAutoCompleteResult.idl @@ +83,5 @@ > readonly attribute unsigned long matchCount; > > /** > + * If true, the results will not be displayed in the popup. However, > + * completing the default value may still occur. May you expand the comment a bit more here, esp. the fact that "it may still occur" doesn't say anything to the reader about when and how. ::: toolkit/components/autocomplete/nsIAutoCompleteSimpleResult.idl @@ +77,5 @@ > /** > + * A writer for the readonly attribute 'hidden', typically set because > + * a result is only intended for type-ahead completion. > + */ > + void setAsTypeAheadResult(in boolean aHidden); A find a bit confusing the continuous change across hidden and typeAheadResult naming, I see you fixed this based on Shawn's comment, but I think there should be some kind of consistency across this naming across all the patch. Sticking with typeAheadResult is fine, and I'd like to see the word "hidden" disappear, also because it's not really hidden, it just appears elsewhere. What about setAsTypeAheadResult for the setter and typeAheadOnly for the arg and the attribute? Hidden appears in a bunch of places across the patch, and it's not really clear what it means just by reading it, typeAheadOnly would be more self documenting. ::: toolkit/components/autocomplete/tests/unit/head_autocomplete.js @@ +15,5 @@ > +function AutoCompleteInputBase(aSearches) { > + this.searches = aSearches; > +} > +AutoCompleteInputBase.prototype = { > + constructor: AutoCompleteInputBase, Do you really have to specify it? This forces you to override it at each inheriting, that may not be needed. @@ +29,5 @@ > + completeDefaultIndex: false, > + > + // Text selection range > + selStart: 0, > + selEnd: 0, nit: prefix non public properties with _ @@ +31,5 @@ > + // Text selection range > + selStart: 0, > + selEnd: 0, > + get selectionStart() { > + return selStart; this.selStart @@ +34,5 @@ > + get selectionStart() { > + return selStart; > + }, > + get selectionEnd() { > + return selEnd; this.selEnd @@ +38,5 @@ > + return selEnd; > + }, > + selectTextRange: function(aStart, aEnd) { > + selStart = aStart; > + selEnd = aEnd; this.selStart and this.seleEnd @@ +60,5 @@ > + invalidate: function() {}, > + > + // nsISupports implementation > + QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports, > + Ci.nsIAutoCompletePopup]) generateQI doesn't need nsISupports, it's added automagically. This happens multiple times in the patch, won't comment further on it. @@ +71,5 @@ > + > + > + > +/** > + * nsIAutoCompleteResult implementation nit: no need for this much newlines, the javadoc is already somewhat separating stuff for readability @@ +145,5 @@ > + // AutoCompleteResult > + _result: null, > + > + > + startSearch: function(aSearchString, nit: double newline @@ +154,5 @@ > + var result = this._result; > + > + result.searchResult = Ci.nsIAutoCompleteResult.RESULT_SUCCESS; > + aListener.onSearchResult(this, result); > + }, nit: the bracing style of methods is not consistent, stick to one if possible @@ +190,5 @@ > + componentManager.registerFactory(cid, desc, name, aSearch); > + > + // Keep the id on the object so we can unregister later > + aSearch.cid = cid; > +} nit: This may get some cleanup. For example you could oneline Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator).generateUUID(); and remove some newlines to help readability @@ +201,5 @@ > +function unregisterAutoCompleteSearch(aSearch) { > + var componentManager = Components.manager > + .QueryInterface(Ci.nsIComponentRegistrar); > + componentManager.unregisterFactory(aSearch.cid, aSearch); > +} you may do this differently, unless you need some fancy sequence of steps. Modify registerAutoCompleteSearch so that it calls registerCleanupFunction with unregistration for that search, at that point it will be unregistered automatically by the harness (And you also already have a component manager reference there for the closure). ::: toolkit/components/autocomplete/tests/unit/test_hiddenResult.js @@ +39,5 @@ > + > + // Caret must be at the end. Autofill doesn't happen unless you're typing > + // characters at the end. > + var strLen = inputStr.length; > + input.selectTextRange(strLen, strLen); considering your autocomplete input replacement was doing a noop on selectTextRange, are you sure regarding this requirement (or better, that it's needed to enforce it)? If it sticks, please test the selection getters. @@ +63,5 @@ > + unregisterAutoCompleteSearch(search2); > + do_test_finished(); > + }; > + > + do_test_pending(); nit: putting this at the beginning of run_test makes immediately clear it's an async test

I forgot that I had already fixed the lack of "this" references and so forgot to upload the patch when I changed review to you. This should fix it all. Also, if this patch needs an sr, who do you suggest as an sr peer? I don't think this patch is substantial enough to warrant an sr, same with bug 660156.

gavin should probably sr this as a final step, when it has a positive review.

that said, any patch touching an idl should have a sr pass even if it's just adding a constant.

Comment on attachment 546733 [details] [diff] [review] Patch 5 Review of attachment 546733 [details] [diff] [review]: ----------------------------------------------------------------- much better, thanks. ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp @@ +1268,5 @@ > oldMatchCount = mMatchCounts[aSearchIndex]; > mMatchCounts[oldIndex] = matchCount; > } > > + PRBool hiddenResult = PR_FALSE; isTypeAheadResult please @@ +1588,3 @@ > > + // Skip past the result completely if it is marked as hidden > + PRBool hiddenResult = PR_FALSE; isTypeAheadResult as well ::: toolkit/components/autocomplete/tests/unit/test_hiddenResult.js @@ +5,5 @@ > + this._values = aValues; > + this.defaultIndex = (aTypeAheadResult ? 0 : -1); > + this._typeAheadResult = aTypeAheadResult; > +} > +AutoCompleteResult.prototype = Object.create(AutoCompleteResultBase.prototype); may you create 2 separate objects here as in the other patch? AutoCompleteResult and AutoCompleteTypeAheadResult, so we get rid of another anonymous bool arg and you can remove those useless comments below (hidden search, non-hidden search) @@ +29,5 @@ > + registerAutoCompleteSearch(search1); > + registerAutoCompleteSearch(search2); > + > + var controller = Cc["@mozilla.org/autocomplete/controller;1"]. > + getService(Ci.nsIAutoCompleteController); please move this near its first use @@ +40,5 @@ > + > + // Caret must be at the end. Autofill doesn't happen unless you're typing > + // characters at the end. > + var strLen = inputStr.length; > + input.selectTextRange(strLen, strLen); test selectionStart and selectionEnd getters here, since it matters that they work correctly @@ +60,5 @@ > + do_check_eq(controller.matchCount, 1); > + > + // Unregister searches > + unregisterAutoCompleteSearch(search1); > + unregisterAutoCompleteSearch(search2); I still think these should be done automatically in registerAutocompleteSearch, but I won't cry for that

Comment on attachment 546733 [details] [diff] [review] Patch 5 ehr sorry, please ask SR to gavin once you attach an updated patch.

I'm not very good at javascript, so I didn't understand what you meant when you wanted to get rid of the unregister function.

don't worry, I'm not very good at english :) Just to clarify, in xpcshell tests you can register cleanup functions that will be run by the test harness when your test finishes, in a real simple way: do_register_cleanup(function() { // do something }); so Ideally in registerSearch you may have invoked this method to have the harness automatically call unregisterSearch on test finish, automatically. But doesn't matter that much.

(In reply to comment #0) > I plan on doing sync inline autocompleting by conducting a separate, > simultaneous autocomplete search. And this will be achieved without sync IO by caching autocomplete info in memory, I guess? A design overview for the work associated with bug 566489 in general would be handy, is that written up anywhere? Does this patch still depend on bug 660156?

(In reply to comment #16) > (In reply to comment #0) > > I plan on doing sync inline autocompleting by conducting a separate, > > simultaneous autocomplete search. > > And this will be achieved without sync IO by caching autocomplete info in > memory, I guess? The idea is to move top frecent hostnames in a memory table and query that one synchronously. I think all the general design overview should indeed be added (or updated) in https://wiki.mozilla.org/Firefox/Features/URL_Autocomplete

(In reply to comment #16) > (In reply to comment #0) > > I plan on doing sync inline autocompleting by conducting a separate, > > simultaneous autocomplete search. > > And this will be achieved without sync IO by caching autocomplete info in > memory, I guess? > > A design overview for the work associated with bug 566489 in general would > be handy, is that written up anywhere? Does this patch still depend on bug > 660156? Gavin, I'm following limi's general plan in bug 659445 comment 8. Due to the order of patches in my mq, I'd say bug 660156 depends on this patch.

Bug 1042561 fixed a regression apparently caused by this.