Search completion; refactor SearchCompleter activation.

5 files changed, 35 insertions, 53 deletions

diff --git a/background_scripts/completion.coffee b/background_scripts/completion.coffee
index 011ff72b..c1f76b81 100644
--- a/background_scripts/completion.coffee
+++ b/background_scripts/completion.coffee
@@ -136,8 +136,7 @@ class BookmarkCompleter
   # These bookmarks are loaded asynchronously when refresh() is called.
   bookmarks: null
 
-  filter: (queryTerms, @onComplete) ->
-    @queryTerms = queryTerms.filter (t) -> 0 < t.length
+  filter: ({ @queryTerms }, @onComplete) ->
     @currentSearch = { queryTerms: @queryTerms, onComplete: @onComplete }
     @performSearch() if @bookmarks
 
@@ -198,8 +197,7 @@ class BookmarkCompleter
     RankingUtils.wordRelevancy(suggestion.queryTerms, suggestion.url, suggestion.title)
 
 class HistoryCompleter
-  filter: (queryTerms, onComplete) ->
-    queryTerms = queryTerms.filter (t) -> 0 < t.length
+  filter: ({ queryTerms }, onComplete) ->
     @currentSearch = { queryTerms: @queryTerms, onComplete: @onComplete }
     results = []
     HistoryCache.use (history) =>
@@ -233,8 +231,7 @@ class DomainCompleter
   #     If `referenceCount` goes to zero, the domain entry can and should be deleted.
   domains: null
 
-  filter: (queryTerms, onComplete) ->
-    queryTerms = queryTerms.filter (t) -> 0 < t.length
+  filter: ({ queryTerms }, onComplete) ->
     return onComplete([]) unless queryTerms.length == 1
     if @domains
       @performSearch(queryTerms, onComplete)
@@ -336,8 +333,7 @@ tabRecency = new TabRecency()
 
 # Searches through all open tabs, matching on title and URL.
 class TabCompleter
-  filter: (queryTerms, onComplete) ->
-    queryTerms = queryTerms.filter (t) -> 0 < t.length
+  filter: ({ queryTerms }, onComplete) ->
     # NOTE(philc): We search all tabs, not just those in the current window. I'm not sure if this is the
     # correct UX.
     chrome.tabs.query {}, (tabs) =>
@@ -371,10 +367,11 @@ class SearchEngineCompleter
       handler: "customSearchEngineKeywords"
       keywords: key for own key of @searchEngines
 
-  filter: (queryTerms, onComplete) ->
+  filter: ({ queryTerms, query }, onComplete) ->
+    return onComplete [] if queryTerms.length == 0
     suggestions = []
 
-    { keyword, searchUrl, description } = @getSearchEngineMatches queryTerms
+    { keyword, searchUrl, description } = @getSearchEngineMatches queryTerms, query
     custom = searchUrl? and keyword?
     searchUrl ?= Settings.get "searchUrl"
     haveDescription = description? and 0 < description.length
@@ -382,7 +379,6 @@ class SearchEngineCompleter
 
     queryTerms = queryTerms[1..] if custom
     query = queryTerms.join " "
-    return onComplete [] if queryTerms.length == 0
 
     # For custom search engines, we add an auto-selected suggestion.
     if custom
@@ -399,9 +395,6 @@ class SearchEngineCompleter
         # Suppress the "w" from "w query terms" in the vomnibar input.
         suppressLeadingKeyword: true
 
-    # We filter out the empty strings late so that we can distinguish between, for example, "w" and "w ".
-    queryTerms = queryTerms.filter (t) -> 0 < t.length
-
     # Exclude results from other completers if this is a custom search engine and we have a completer.
     filter =
       if custom and CompletionEngines.haveCompletionEngine searchUrl
@@ -431,7 +424,6 @@ class SearchEngineCompleter
         characterCount = query.length - queryTerms.length + 1
         relavancy = 0.6 * (Math.min(characterCount, 10.0)/10.0)
 
-        queryTerms = queryTerms.filter (t) -> 0 < t.length
         if 0 < existingSuggestions.length
           existingSuggestionsMinScore = existingSuggestions[existingSuggestions.length-1].relevancy
           if relavancy < existingSuggestionsMinScore and MultiCompleter.maxResults <= existingSuggestions.length
@@ -457,8 +449,14 @@ class SearchEngineCompleter
           count = Math.min 6, Math.max 3, MultiCompleter.maxResults - existingSuggestions.length
           onComplete suggestions[...count]
 
-  getSearchEngineMatches: (queryTerms) ->
-    (1 < queryTerms.length and @searchEngines[queryTerms[0]]) or {}
+  getSearchEngineMatches: (queryTerms, query = queryTerms.join " ") ->
+    # To allow users to write queries with leading search-engine keywords, leading whitespace disables custom
+    # search engines; for example, " w" is a regular query.
+    return {} if /^\s/.test query
+    # Trailing whitespace is significant when activating a custom search engine; for example, "w" (just a
+    # regular query) is different from "w " (a custom search engine).
+    length = queryTerms.length + (if /\s$/.test query then 1 else 0)
+    (1 < length and @searchEngines[queryTerms[0]]) or {}
 
   # Static data and methods for parsing the configured search engines.  We keep a cache of the search-engine
   # mapping in @searchEnginesMap.
@@ -507,11 +505,12 @@ class MultiCompleter
       # other completers.
       filter: null
 
-    (queryTerms, onComplete) ->
+    (request, onComplete) ->
       @debug = true
       # Allow only one query to run at a time, and remember the most recent query.
       return @mostRecentQuery = arguments if @filterInProgress
 
+      { queryTerms } = request
       RegexpCache.clear()
       @mostRecentQuery = null
       @filterInProgress = true
@@ -524,7 +523,7 @@ class MultiCompleter
       # results, then calling any continuations.
       for completer, index in @completers
         do (index) =>
-          completer.filter queryTerms, (newSuggestions = [], { continuation, filter } = defaultCallbackOptions) =>
+          completer.filter request, (newSuggestions = [], { continuation, filter } = defaultCallbackOptions) =>
 
             # Store the results.
             suggestions.push newSuggestions...
@@ -546,7 +545,6 @@ class MultiCompleter
               unless suggestions.length == 0 and shouldRunContinuations
                 onComplete
                   results: @prepareSuggestions queryTerms, suggestions
-                  mayCacheResults: continuations.length == 0
                   expectMoreResults: shouldRunContinuations
 
               # Run any continuations, unless there's a pending query.
@@ -560,7 +558,6 @@ class MultiCompleter
                       results: @prepareSuggestions queryTerms, suggestions
                       # FIXME(smblott) This currently assumes that there is at most one continuation.  We
                       # should really be counting pending/completed continuations.
-                      mayCacheResults: true
                       expectMoreResults: false
 
               # Admit subsequent queries, and launch any pending query.
diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee
index 34db5a20..612f6170 100644
--- a/background_scripts/main.coffee
+++ b/background_scripts/main.coffee
@@ -61,7 +61,7 @@ completers =
 
 completionHandlers =
   filter: (completer, request, port) ->
-    completer.filter request.queryTerms, (response) ->
+    completer.filter request, (response) ->
       port.postMessage extend request, extend response, handler: "completions"
 
   refresh: (completer, _, port) -> completer.refresh port
diff --git a/lib/utils.coffee b/lib/utils.coffee
index 033fdd2b..4c2a7a14 100644
--- a/lib/utils.coffee
+++ b/lib/utils.coffee
@@ -201,7 +201,8 @@ Function::curry = ->
 Array.copy = (array) -> Array.prototype.slice.call(array, 0)
 
 String::startsWith = (str) -> @indexOf(str) == 0
-String::ltrim = () -> @replace /^\s+/, ""
+String::ltrim = -> @replace /^\s+/, ""
+String::rtrim = -> @replace /\s+$/, ""
 
 globalRoot = window ? global
 globalRoot.extend = (hash1, hash2) ->
diff --git a/pages/vomnibar.coffee b/pages/vomnibar.coffee
index 4781c273..bc773909 100644
--- a/pages/vomnibar.coffee
+++ b/pages/vomnibar.coffee
@@ -200,12 +200,14 @@ class VomnibarUI
       window.clearTimeout @updateTimer
       @updateTimer = null
 
+  isCustomSearch: ->
+    queryTerms = @input.value.ltrim().split /\s+/
+    1 < queryTerms.length and queryTerms[0] in @keywords
+
   update: (updateSynchronously = false, callback = null) =>
-    # If the query text is a custom search keyword, then we need to force a synchronous update (so that the
+    # If the query text becomes a custom search, then we need to force a synchronous update (so that the
     # interface is snappy).
-    if @keywords? and not @suppressedLeadingKeyword?
-      queryTerms = @input.value.ltrim().split /\s+/
-      updateSynchronously ||= 1 < queryTerms.length and queryTerms[0] in @keywords
+    updateSynchronously ||= @isCustomSearch() and not @suppressedLeadingKeyword?
     if updateSynchronously
       @updateCompletions callback
     else if not @updateTimer?
@@ -242,7 +244,6 @@ class BackgroundCompleter
   constructor: (@name) ->
     @port = chrome.runtime.connect name: "completions"
     @messageId = null
-    @cache ?= new SimpleCache 1000 * 60 * 5
     @reset()
 
     @port.onMessage.addListener (msg) =>
@@ -260,33 +261,18 @@ class BackgroundCompleter
               else
                 @completionActions.navigateToUrl.curry result.url
 
-          # Cache the results (but only if the background completer tells us that it's ok to do so).
-          if msg.mayCacheResults
-            console.log "cache set:", msg.query if @debug
-            @cache.set msg.query, msg.results
-          else
-            console.log "not setting cache:", msg.query if @debug
-
           # We ignore messages which arrive too late.
           if msg.id == @messageId
             @mostRecentCallback msg.results
 
   filter: (query, @mostRecentCallback) ->
-    # We retain trailing whitespace so that we can tell the difference between "w" and "w " (for custom search
-    # engines).
-    queryTerms = query.ltrim().split(/\s+/)
-    query = queryTerms.join " "
-    if @cache.has query
-      console.log "cache hit:", query if @debug
-      @mostRecentCallback @cache.get query
-    else
-      @messageId = Utils.createUniqueId()
-      @port.postMessage
-        name: @name
-        handler: "filter"
-        id: @messageId
-        query: query
-        queryTerms: queryTerms
+    queryTerms = query.trim().split(/\s+/).filter (s) -> 0 < s.length
+    @port.postMessage
+      handler: "filter"
+      name: @name
+      id: @messageId = Utils.createUniqueId()
+      queryTerms: queryTerms
+      query: query
 
   refresh: (@lastUI) ->
     @reset()
@@ -294,8 +280,6 @@ class BackgroundCompleter
     @port.postMessage name: @name, handler: "refresh"
 
   reset: ->
-    # We only cache results for the duration of a single vomnibar activation, so clear the cache now.
-    @cache.clear()
 
   cancel: ->
     # Inform the background completer that it may (should it choose to do so) abandon any pending query
diff --git a/tests/unit_tests/completion_test.coffee b/tests/unit_tests/completion_test.coffee
index b1962daf..b9a062f2 100644
--- a/tests/unit_tests/completion_test.coffee
+++ b/tests/unit_tests/completion_test.coffee
@@ -486,7 +486,7 @@ context "TabRecency",
 # A convenience wrapper around completer.filter() so it can be called synchronously in tests.
 filterCompleter = (completer, queryTerms) ->
   results = []
-  completer.filter(queryTerms, (completionResults) -> results = completionResults)
+  completer.filter({ queryTerms }, (completionResults) -> results = completionResults)
   results
 
 hours = (n) -> 1000 * 60 * 60 * n