Search completion; fix typos and small bugs.

Bugs:
  - The serch-engine completer wasn't being configured correctly for the
    case when we don't have a completion engine.
  - Handle Control-Shift-Left/Right in vomnibar.

Also: better comments.

5 files changed, 35 insertions, 31 deletions

diff --git a/background_scripts/completion.coffee b/background_scripts/completion.coffee
index e38a4e16..85829c75 100644
--- a/background_scripts/completion.coffee
+++ b/background_scripts/completion.coffee
@@ -361,14 +361,18 @@ class SearchEngineCompleter
   cancel: ->
     CompletionSearch.cancel()
 
-  # This looks up the custom search engine and, if one is found, then notes it and removes its keyword from
-  # the query terms.  It also sets request.completers to indicate that only this completer should run.
+  # This looks up the custom search engine and, if one is found, notes it and removes its keyword from the
+  # query terms.  It also sets request.completers to indicate that only this completer should run (but only if
+  # it finds a completion engine).
   triageRequest: (request) ->
     @searchEngines.use (engines) =>
       { queryTerms, query } = request
       keyword = queryTerms[0]
+      # Note. For a keyword "w", we match "w search terms" and "w ", but not "w" on its own.
       if keyword and engines[keyword] and (1 < queryTerms.length or /\s$/.test query)
-        request.completers = [ this ]
+        # This is a query for a custom search engine.  If we have a completion engine, then we *only* want
+        # suggestions from SearchEngineCompleter.
+        request.completers = [ this ] if CompletionSearch.haveCompletionEngine engines[keyword].searchUrl
         extend request,
           queryTerms: queryTerms[1..]
           keyword: keyword
@@ -394,7 +398,9 @@ class SearchEngineCompleter
 
       callback engines
 
-      # Let the front-end vomnibar know the search-engine keywords.
+      # Let the front-end vomnibar know the search-engine keywords.  It needs to know them so that, when the
+      # query goes from "w" to "w ", the vomnibar synchronously launches the next filter() request (all of which avoids
+      # an ugly delay).
       port.postMessage
         handler: "keywords"
         keywords: key for own key of engines
@@ -453,7 +459,6 @@ class SearchEngineCompleter
         insertText: if useExclusiveVomnibar then query else null
         # We suppress the leading keyword, for example "w something" becomes "something" in the vomnibar.
         suppressLeadingKeyword: true
-        selectCommonMatches: false
         customSearchEnginePrimarySuggestion: true
         # Toggles for the legacy behaviour.
         autoSelect: not useExclusiveVomnibar
@@ -470,7 +475,6 @@ class SearchEngineCompleter
           relevancy: relevancy *= 0.9
           insertText: suggestion
           highlightTerms: false
-          selectCommonMatches: true
           customSearchEngineCompletionSuggestion: true
 
     # If we have cached suggestions, then we can bundle them immediately (otherwise we'll have to fetch them
@@ -495,9 +499,11 @@ class SearchEngineCompleter
           onComplete suggestions.map mkSuggestion
 
 # A completer which calls filter() on many completers, aggregates the results, ranks them, and returns the top
-# 10. Queries from the vomnibar frontend script come through a multi completer.
+# 10. All queries from the vomnibar come through a multi completer.
 class MultiCompleter
   maxResults: 10
+  filterInProgress: false
+  mostRecentQuery: null
 
   constructor: (@completers) ->
   refresh: (port) -> completer.refresh? port for completer in @completers
@@ -531,8 +537,8 @@ class MultiCompleter
           filters.push filter if filter?
           callback()
 
-    # Once all completers have finished, process the results and post them, and run any continuations or
-    # pending queries.
+    # Once all completers have finished, process the results and post them, and run any continuations or a
+    # pending query.
     jobs.onReady =>
       suggestions = suggestions.filter filter for filter in filters
       shouldRunContinuations = 0 < continuations.length and not @mostRecentQuery?
@@ -563,7 +569,7 @@ class MultiCompleter
             results: suggestions
             mayCacheResults: true
 
-      # Admit subsequent queries, and launch any pending query.
+      # Admit subsequent queries and launch any pending query.
       @filterInProgress = false
       if @mostRecentQuery
         @filter @mostRecentQuery...
diff --git a/background_scripts/completion_engines.coffee b/background_scripts/completion_engines.coffee
index 14e65692..048e9020 100644
--- a/background_scripts/completion_engines.coffee
+++ b/background_scripts/completion_engines.coffee
@@ -1,6 +1,6 @@
 
 # A completion engine provides search suggestions for a search engine.  A search engine is identified by a
-# "searchUrl", e.g. Settings.get("searchUrl"), or a custom search engine.
+# "searchUrl", e.g. Settings.get("searchUrl"), or a custom search engine URL.
 #
 # Each completion engine defines three functions:
 #
@@ -14,7 +14,7 @@
 #      returns a list of suggestions (a list of strings).  This method is always executed within the context
 #      of a try/catch block, so errors do not propagate.
 #
-# Each new completion engine must be add to the list "CompletionEngines" at the bottom of this file.
+# Each new completion engine must be added to the list "CompletionEngines" at the bottom of this file.
 #
 # The lookup logic which uses these completion engines is in "./completion_search.coffee".
 #
diff --git a/background_scripts/completion_search.coffee b/background_scripts/completion_search.coffee
index 2d2ee439..b6514cd2 100644
--- a/background_scripts/completion_search.coffee
+++ b/background_scripts/completion_search.coffee
@@ -2,7 +2,7 @@
 CompletionSearch =
   debug: false
   inTransit: {}
-  completionCache: new SimpleCache 2 * 60 * 60 * 1000, 5000 # Two hour, 5000 entries.
+  completionCache: new SimpleCache 2 * 60 * 60 * 1000, 5000 # Two hours, 5000 entries.
   engineCache:new SimpleCache 1000 * 60 * 60 * 1000 # 1000 hours.
 
   # The amount of time to wait for new requests before launching the current request (for example, if the user
@@ -35,7 +35,7 @@ CompletionSearch =
     not @lookupEngine(searchUrl).dummy
 
   # This is the main entry point.
-  #  - searchUrl is the search engine's URL, e.g. Settings.get("searchUrl"), or a custome search engine's URL.
+  #  - searchUrl is the search engine's URL, e.g. Settings.get("searchUrl"), or a custom search engine's URL.
   #    This is only used as a key for determining the relevant completion engine.
   #  - queryTerms are the query terms.
   #  - callback will be applied to a list of suggestion strings (which may be an empty list, if anything goes
@@ -82,7 +82,6 @@ CompletionSearch =
 
       if reusePreviousSuggestions
         console.log "reuse previous query:", @mostRecentQuery if @debug
-        @mostRecentQuery = queryTerms.join " "
         return callback @completionCache.set completionCacheKey, @mostRecentSuggestions
 
     # That's all of the caches we can try.  Bail if the caller is looking for synchronous results.
diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee
index 913f1de5..6ee0e8e7 100644
--- a/background_scripts/main.coffee
+++ b/background_scripts/main.coffee
@@ -64,7 +64,7 @@ completionHandlers =
     completer.filter request, (response) ->
       # We use try here because this may fail if the sender has already navigated away from the original page.
       # This can happen, for example, when posting completion suggestions from the SearchEngineCompleter
-      # (which can be slow).
+      # (which is done asynchronously).
       try
         port.postMessage extend request, extend response, handler: "completions"
 
diff --git a/pages/vomnibar.coffee b/pages/vomnibar.coffee
index acf45648..7c9eef83 100644
--- a/pages/vomnibar.coffee
+++ b/pages/vomnibar.coffee
@@ -91,8 +91,8 @@ class VomnibarUI
       @suppressedLeadingKeyword = queryTerms[0]
       @input.value = queryTerms[1..].join " "
 
-    # For suggestions from search-engine completion, we copy the suggested text into the input when the item
-    # is selected, and revert when it is not.  This allows the user to select a suggestion and then continue
+    # For suggestions for custom search engines, we copy the suggested text into the input when the item is
+    # selected, and revert when it is not.  This allows the user to select a suggestion and then continue
     # typing.
     if 0 <= @selection and @completions[@selection].insertText?
       @previousInputValue ?=
@@ -113,8 +113,8 @@ class VomnibarUI
     for i in [0...@completionList.children.length]
       @completionList.children[i].className = (if i == @selection then "vomnibarSelected" else "")
 
-  # This adds prompted text to the vomnibar input.  The propted text is a continuation of the text the user
-  # has typed already, taken from one of the search suggestions.  It is highlight (using the selection) and
+  # This adds prompted text to the vomnibar input.  The prompted text is a continuation of the text the user
+  # has already typed, taken from one of the search suggestions.  It is highlight (using the selection) and
   # will be included with the query should the user type <Enter>.
   addPromptedText: (response) ->
     # Bail if we don't yet have the background completer's final word on the current query.
@@ -168,8 +168,7 @@ class VomnibarUI
       return "enter"
     else if event.keyCode == keyCodes.backspace || event.keyCode == keyCodes.deleteKey
       return "delete"
-    else if key in [ "left", "right" ] and event.ctrlKey and
-        not (event.altKey or event.metaKey or event.shiftKey)
+    else if key in [ "left", "right" ] and event.ctrlKey and not (event.altKey or event.metaKey)
       return "control-#{key}"
 
     null
@@ -219,8 +218,8 @@ class VomnibarUI
         @hide -> completion.performAction openInNewTab
     else if action == "delete"
       if @suppressedLeadingKeyword? and @input.value.length == 0
-        # Normally, with custom search engines, the keyword (e,g, the "w" of "w query terms") suppressed.  If
-        # the input is empty, then show the keyword again.
+        # Normally, with custom search engines, the keyword (e,g, the "w" of "w query terms") is suppressed.
+        # If the input is empty, then show the keyword again.
         @input.value = @suppressedLeadingKeyword
         @suppressedLeadingKeyword = null
         @updateCompletions()
@@ -249,13 +248,13 @@ class VomnibarUI
     true
 
   onKeypress: (event) =>
-    # Handle typing with prompted text.
+    # Handle typing together with prompted text.
     unless event.altKey or event.ctrlKey or event.metaKey
       if @inputContainsASelectionRange()
-        # As the user types characters which the match prompted text, we suppress the keyboard event and
-        # simulate it by advancing the start of the selection (but only if the typed character matches).  This
-        # avoids flicker (if we were to allow the event through) as the selection is first collapsed then
-        # restored.
+        # As the user types characters which the match the prompted text, we suppress the keyboard event and
+        # simulate it by advancing the start of the selection (but only if the typed character matches).
+        # If we were to allow the event through, we would get flicker, as the selection is first collapsed and
+        # then (shortly afterwards) restored.
         if @input.value[@input.selectionStart][0].toLowerCase() == (String.fromCharCode event.charCode).toLowerCase()
           @input.setSelectionRange @input.selectionStart + 1, @input.selectionEnd
           @updateOnInput()
@@ -267,7 +266,7 @@ class VomnibarUI
   inputContainsASelectionRange: ->
     @input.selectionStart? and @input.selectionEnd? and @input.selectionStart != @input.selectionEnd
 
-  # Return the text of the input, with any selected text removed.
+  # Return the text of the input, with any prompted text removed.
   getInputWithoutPromptedText: ->
     if @inputContainsASelectionRange()
       @input.value[0...@input.selectionStart] + @input.value[@input.selectionEnd..]
@@ -275,7 +274,7 @@ class VomnibarUI
       @input.value
 
   # Return the background-page query corresponding to the current input state.  In other words, reinstate any
-  # search engine keyword which is currently being suppressed, and strip any propted text.
+  # search engine keyword which is currently being suppressed, and strip any prompted text.
   getInputValueAsQuery: ->
     (if @suppressedLeadingKeyword? then @suppressedLeadingKeyword + " " else "") + @getInputWithoutPromptedText()