Use bloodhound lib for suggestions

Fix #7453

app/browser/reducers/urlBarSuggestionsReducer.js

@@ -6,9 +6,13 @@
 
 const appConstants = require('../../../js/constants/appConstants')
 const {generateNewSuggestionsList, generateNewSearchXHRResults} = require('../../common/lib/suggestion')
+const {init} = require('../../common/lib/siteSuggestions')
 
 const urlBarSuggestionsReducer = (state, action) => {
   switch (action.actionType) {
+    case appConstants.APP_SET_STATE:
+      init(Object.values(action.appState.get('sites').toJS()))
+      break
     case appConstants.APP_URL_BAR_TEXT_CHANGED:
       generateNewSuggestionsList(state, action.windowId, action.tabId, action.input)
       generateNewSearchXHRResults(state, action.windowId, action.tabId, action.input)

app/common/lib/siteSuggestions.js

@@ -3,17 +3,32 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 const Bloodhound = require('bloodhound-js')
+
 let initialized = false
 let engine
+let internalSort
+let lastQueryInput
 
-const init = (sites) => {
-  if (initialized) {
-    return Promise.resolve()
+// Same as sortByAccessCountWithAgeDecay but if one is a prefix of the
+// other then it is considered always sorted first.
+const sortForSuggestions = (s1, s2) => {
+  return internalSort(s1, s2)
+}
+
+const getSiteIdentity = (data) => {
+  if (typeof data === 'string') {
+    return data
   }
+  return (data.location || '') + (data.partitionNumber ? '|' + data.partitionNumber : '')
+}
+
+const init = (sites) => {
   engine = new Bloodhound({
-    local: sites,
+    local: sites.toJS ? sites.toJS() : sites,
+    sorter: sortForSuggestions,
     queryTokenizer: tokenizeInput,
-    datumTokenizer: tokenizeInput
+    datumTokenizer: tokenizeInput,
+    identify: getSiteIdentity
   })
   const promise = engine.initialize()
   promise.then(() => {
@@ -46,7 +61,10 @@ const query = (input) => {
     return Promise.resolve([])
   }
   return new Promise((resolve, reject) => {
-    engine.search(input, function (results) {
+    lastQueryInput = input || ''
+    const {getSortForSuggestions} = require('./suggestion')
+    internalSort = getSortForSuggestions(lastQueryInput.toLowerCase())
+    engine.search(lastQueryInput, function (results) {
       resolve(results)
     }, function (err) {
       reject(err)

app/common/lib/suggestion.js

@@ -11,11 +11,11 @@ const {isUrl, aboutUrls, isNavigatableAboutPage, isSourceAboutUrl} = require('..
 const suggestionTypes = require('../../../js/constants/suggestionTypes')
 const getSetting = require('../../../js/settings').getSetting
 const settings = require('../../../js/constants/settings')
-const {isBookmark, isDefaultEntry, isHistoryEntry} = require('../../../js/state/siteUtil')
 const config = require('../../../js/constants/config')
 const top500 = require('../../../js/data/top500')
 const fetchSearchSuggestions = require('./fetchSearchSuggestions')
 const {getFrameByTabId, getTabsByWindowId} = require('../../common/state/tabState')
+const {query} = require('./siteSuggestions')
 
 const sigmoid = (t) => {
   return 1 / (1 + Math.pow(Math.E, -t))
@@ -83,15 +83,15 @@ const sortingPriority = (count, currentTime, lastAccessedTime, ageDecayConstant)
 const sortByAccessCountWithAgeDecay = (s1, s2) => {
   const now = new Date()
   const s1Priority = sortingPriority(
-    s1.get('count') || 0,
+    s1.count || 0,
     now.getTime(),
-    s1.get('lastAccessedTime') || now.getTime(),
+    s1.lastAccessedTime || now.getTime(),
     appConfig.urlSuggestions.ageDecayConstant
   )
   const s2Priority = sortingPriority(
-    s2.get('count') || 0,
+    s2.count || 0,
     now.getTime(),
-    s2.get('lastAccessedTime') || now.getTime(),
+    s2.lastAccessedTime || now.getTime(),
     appConfig.urlSuggestions.ageDecayConstant
   )
   return s2Priority - s1Priority
@@ -214,43 +214,45 @@ const createVirtualHistoryItems = (historySites) => {
     return !!site
   })
   return Immutable.fromJS(_.object(virtualHistorySites.map((site) => {
-    return [site.location, site]
+    return [site.get('location'), site]
   })))
 }
 
-const sortBasedOnLocationPos = (urlLocationLower) => (s1, s2) => {
-  const shouldNormalize = shouldNormalizeLocation(urlLocationLower)
-  const urlLocationLowerNormalized = normalizeLocation(urlLocationLower)
-
-  const location1 = shouldNormalize ? normalizeLocation(getURL(s1)) : getURL(s1)
-  const location2 = shouldNormalize ? normalizeLocation(getURL(s2)) : getURL(s2)
-  const pos1 = location1.indexOf(urlLocationLowerNormalized)
-  const pos2 = location2.indexOf(urlLocationLowerNormalized)
-  if (pos1 === -1 && pos2 === -1) {
-    return 0
-  } else if (pos1 === -1) {
-    return 1
-  } else if (pos2 === -1) {
-    return -1
-  } else {
-    if (pos1 - pos2 !== 0) {
+// Same as sortByAccessCountWithAgeDecay but if one is a prefix of the
+// other then it is considered always sorted first.
+const getSortForSuggestions = (userInputLower) => (s1, s2) => {
+  const {sortByAccessCountWithAgeDecay, normalizeLocation} = require('./suggestion')
+  const url1 = normalizeLocation(getURL(s1))
+  const url2 = normalizeLocation(getURL(s2))
+  const pos1 = url1.indexOf(userInputLower)
+  const pos2 = url2.indexOf(userInputLower)
+  if (pos1 !== -1 && pos2 !== -1) {
+    if (pos1 !== pos2) {
       return pos1 - pos2
-    } else {
-      // sort site.com higher than site.com/somepath
-      const sdnv1 = simpleDomainNameValue(s1)
-      const sdnv2 = simpleDomainNameValue(s2)
-      if (sdnv1 !== sdnv2) {
-        return sdnv2 - sdnv1
-      } else {
-        // If there's a tie on the match location, use the age
-        // decay modified access count
-        return sortByAccessCountWithAgeDecay(s1, s2)
-      }
     }
+    return url1.length - url2.length
+  }
+  if (pos1 !== -1 && pos2 === -1) {
+    return -1
   }
+  if (pos1 === -1 && pos2 !== -1) {
+    return 1
+  }
+  return sortByAccessCountWithAgeDecay(s1, s2)
 }
 
-const getURL = (x) => typeof x === 'object' && x !== null ? x.get('location') || x.get('url') : x
+// Currently we sort only sites that are not immutableJS and
+const getURL = (x) => {
+  if (typeof x === 'string') {
+    return x
+  }
+
+  if (x.get) {
+    return x.get('location') || x.get('url')
+  }
+
+  return x.location || x.url
+}
 
 const getMapListToElements = (urlLocationLower) => ({data, maxResults, type,
     sortHandler = (x) => x, filterValue = (site) => {
@@ -285,69 +287,29 @@ const getMapListToElements = (urlLocationLower) => ({data, maxResults, type,
 }
 
 const getHistorySuggestions = (state, urlLocationLower) => {
-  return new Promise((resolve, reject) => {
-    const sortHandler = sortBasedOnLocationPos(urlLocationLower)
-    const mapListToElements = getMapListToElements(urlLocationLower)
-    let suggestionsList = Immutable.List()
-    // NOTE: Iterating sites can take a long time! Please be mindful when
-    // working with the history and bookmark suggestion code.
+  /*
+   * todo:
     const historySuggestionsOn = getSetting(settings.HISTORY_SUGGESTIONS)
     const bookmarkSuggestionsOn = getSetting(settings.BOOKMARK_SUGGESTIONS)
-    const shouldIterateSites = historySuggestionsOn || bookmarkSuggestionsOn
-    if (shouldIterateSites) {
-      // Note: Bookmark sites are now included in history. This will allow
-      // sites to appear in the auto-complete regardless of their bookmark
-      // status. If history is turned off, bookmarked sites will appear
-      // in the bookmark section.
-      const sitesFilter = (site) => {
-        const location = site.get('location')
-        if (!location) {
-          return false
-        }
-        const title = site.get('title')
-        return location.toLowerCase().indexOf(urlLocationLower) !== -1 ||
-          (title && title.toLowerCase().indexOf(urlLocationLower) !== -1)
-      }
-
-      let historySites = new Immutable.List()
-      let bookmarkSites = new Immutable.List()
-      const sites = state.get('sites')
-      sites.forEach(site => {
-        if (!sitesFilter(site)) {
-          return
-        }
-        if (historySuggestionsOn && isHistoryEntry(site) && !isDefaultEntry(site)) {
-          historySites = historySites.push(site)
-          return
-        }
-        if (bookmarkSuggestionsOn && isBookmark(site) && !isDefaultEntry(site)) {
-          bookmarkSites = bookmarkSites.push(site)
-        }
-      })
-
-      if (historySites.size > 0) {
-        historySites = historySites.concat(createVirtualHistoryItems(historySites))
+  */
 
-        suggestionsList = suggestionsList.concat(mapListToElements({
-          data: historySites,
-          maxResults: config.urlBarSuggestions.maxHistorySites,
-          type: suggestionTypes.HISTORY,
-          sortHandler,
-          filterValue: null
-        }))
-      }
+  return new Promise((resolve, reject) => {
+    const sortHandler = getSortForSuggestions(urlLocationLower)
+    const mapListToElements = getMapListToElements(urlLocationLower)
+    query(urlLocationLower).then((results) => {
+      results = makeImmutable(results)
+      results = results.take(config.urlBarSuggestions.maxHistorySites)
+      results = results.concat(createVirtualHistoryItems(results))
 
-      if (bookmarkSites.size > 0) {
-        suggestionsList = suggestionsList.concat(mapListToElements({
-          data: bookmarkSites,
-          maxResults: config.urlBarSuggestions.maxBookmarkSites,
-          type: suggestionTypes.BOOKMARK,
-          sortHandler,
-          filterValue: null
-        }))
-      }
-    }
-    resolve(suggestionsList)
+      const suggestionsList = mapListToElements({
+        data: results,
+        maxResults: config.urlBarSuggestions.maxHistorySites,
+        type: suggestionTypes.HISTORY,
+        sortHandler,
+        filterValue: null
+      })
+      resolve(suggestionsList)
+    })
   })
 }
 
@@ -365,7 +327,7 @@ const getAboutSuggestions = (state, urlLocationLower) => {
 
 const getOpenedTabSuggestions = (state, windowId, urlLocationLower) => {
   return new Promise((resolve, reject) => {
-    const sortHandler = sortBasedOnLocationPos(urlLocationLower)
+    const sortHandler = getSortForSuggestions(urlLocationLower)
     const mapListToElements = getMapListToElements(urlLocationLower)
     const tabs = getTabsByWindowId(state, windowId)
     let suggestionsList = Immutable.List()
@@ -476,11 +438,11 @@ const generateNewSearchXHRResults = (state, windowId, tabId, input) => {
 module.exports = {
   sortingPriority,
   sortByAccessCountWithAgeDecay,
+  getSortForSuggestions,
   simpleDomainNameValue,
   normalizeLocation,
   shouldNormalizeLocation,
   createVirtualHistoryItems,
-  sortBasedOnLocationPos,
   getMapListToElements,
   getHistorySuggestions,
   getAboutSuggestions,

js/constants/config.js

@@ -28,8 +28,7 @@ module.exports = {
   defaultUrl: 'about:newtab',
   urlBarSuggestions: {
     maxOpenedFrames: 2,
-    maxBookmarkSites: 2,
-    maxHistorySites: 3,
+    maxHistorySites: 5,
     maxAboutPages: 2,
     maxSearch: 3,
     maxTopSites: 3

test/unit/app/common/lib/siteSuggestionsTest.js

@@ -1,6 +1,7 @@
 /* global describe, before, it */
 const {tokenizeInput, init, query} = require('../../../../../app/common/lib/siteSuggestions')
 const assert = require('assert')
+const Immutable = require('immutable')
 
 const site1 = {
   location: 'https://www.bradrichter.co/bad_numbers/3',
@@ -113,4 +114,50 @@ describe('siteSuggestions lib', function () {
       checkResult('brave brad', [], cb)
     })
   })
+  describe('query sorts results', function () {
+    before(function (cb) {
+      const sites = Immutable.fromJS([{
+        location: 'https://brave.com/twi'
+      }, {
+        location: 'https://twitter.com/brave'
+      }, {
+        location: 'https://twitter.com/brianbondy'
+      }, {
+        location: 'https://twitter.com/_brianclif'
+      }, {
+        location: 'https://twitter.com/cezaraugusto'
+      }, {
+        location: 'https://bbondy.com/twitter'
+      }, {
+        location: 'https://twitter.com'
+      }, {
+        location: 'https://twitter.com/i/moments'
+      }])
+      init(sites).then(cb.bind(null, null))
+    })
+    it('orders shortest match first', function (cb) {
+      query('twitter.com').then((results) => {
+        assert.deepEqual(results[0], { location: 'https://twitter.com' })
+        cb()
+      })
+    })
+    it('matches prefixes first', function (cb) {
+      query('twi').then((results) => {
+        assert.deepEqual(results[0], { location: 'https://twitter.com' })
+        cb()
+      })
+    })
+    it('closest to the left match wins', function (cb) {
+      query('twitter.com brian').then((results) => {
+        assert.deepEqual(results[0], { location: 'https://twitter.com/brianbondy' })
+        cb()
+      })
+    })
+    it('matches based on tokens and not exactly', function (cb) {
+      query('twitter.com/moments').then((results) => {
+        assert.deepEqual(results[0], { location: 'https://twitter.com/i/moments' })
+        cb()
+      })
+    })
+  })
 })