Tokenize with urlParse and for non urls

app/common/lib/siteSuggestions.js

@@ -3,7 +3,9 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 const Bloodhound = require('bloodhound-js')
+const {isUrl} = require('../../../js/lib/appUrlUtil')
 const siteTags = require('../../../js/constants/siteTags')
+const urlParse = require('../urlParse')
 
 let initialized = false
 let engine
@@ -37,15 +39,24 @@ const init = (sites) => {
   return promise
 }
 
+const getPartsFromNonUrlInput = (input) =>
+  input.toLowerCase().split(/[,-.\s\\/?&]/)
+
 const getTagToken = (tag) => '|' + tag + '|'
+
 const tokenizeInput = (data) => {
   let url = data || ''
   let parts = []
 
   if (typeof data === 'object' && data !== null) {
+    // When lastAccessTime is 1 it is a default built-in entry which we don't want
+    // to appear in suggestions.
+    if (data.lastAccessedTime === 1) {
+      return []
+    }
     url = data.location
     if (data.title) {
-      parts = data.title.toLowerCase().split(/\s/)
+      parts = getPartsFromNonUrlInput(data.title)
     }
     if (data.tags) {
       parts = parts.concat(data.tags.map(getTagToken))
@@ -56,12 +67,27 @@ const tokenizeInput = (data) => {
     }
   }
 
-  if (url) {
-    url = url.toLowerCase().replace(/^https?:\/\//i, '')
-    parts = parts.concat(url.split(/[.\s\\/?&]/))
+  if (url && isUrl(url)) {
+    const parsedUrl = urlParse(url.toLowerCase())
+    if (parsedUrl.hash) {
+      parts.push(parsedUrl.hash.slice(1))
+    }
+    if (parsedUrl.host) {
+      parts = parts.concat(parsedUrl.host.split('.'))
+    }
+    if (parsedUrl.pathname) {
+      parts = parts.concat(parsedUrl.pathname.split(/[.\s\\/]/))
+    }
+    if (parsedUrl.query) {
+      parts = parts.concat(parsedUrl.query.split(/[&=]/))
+    }
+    if (parsedUrl.protocol) {
+      parts = parts.concat(parsedUrl.protocol)
+    }
+  } else if (url) {
+    parts = parts.concat(getPartsFromNonUrlInput(url))
   }
-
-  return parts
+  return parts.filter(x => !!x)
 }
 
 const add = (data) => {
@@ -81,8 +107,8 @@ const query = (input, options = {}) => {
   }
 
   return new Promise((resolve, reject) => {
-    const {getSortForSuggestions, normalizeLocation} = require('./suggestion')
-    input = normalizeLocation((input || '').toLowerCase())
+    const {getSortForSuggestions} = require('./suggestion')
+    input = (input || '').toLowerCase()
     lastQueryOptions = Object.assign({}, options, {
       input,
       internalSort: getSortForSuggestions(input)

app/common/lib/suggestion.js

@@ -99,18 +99,18 @@ const sortByAccessCountWithAgeDecay = (s1, s2) => {
 }
 
 /*
- * Return a 1 if the url is 'simple' as in without query, search or
- * hash components. Return 0 otherwise.
+ * Return true1 the url is 'simple' as in without query, search or
+ * hash components. Return false otherwise.
  *
- * @param {ImmutableObject} site - object represent history entry
+ * @param {object} An already normalized simple URL
  *
  */
-const simpleDomainNameValue = (site) => {
+const isSimpleDomainNameValue = (site) => {
   const parsed = urlParse(getURL(site))
   if (parsed.hash === null && parsed.search === null && parsed.query === null && parsed.pathname === '/') {
-    return 1
+    return true
   } else {
-    return 0
+    return false
   }
 }
 
@@ -231,7 +231,16 @@ const getSortForSuggestions = (userInputLower) => (s1, s2) => {
     if (pos1 !== pos2) {
       return pos1 - pos2
     }
-    return url1.length - url2.length
+    const url1IsSimple = isSimpleDomainNameValue(s1)
+    const url2IsSimple = isSimpleDomainNameValue(s2)
+    // If either both false or both true then rely on count w/ decay sort
+    if (url1IsSimple === url2IsSimple) {
+      return sortByAccessCountWithAgeDecay(s1, s2)
+    }
+    if (url1IsSimple) {
+      return -1
+    }
+    return 1
   }
   if (pos1 !== -1 && pos2 === -1) {
     return -1
@@ -434,7 +443,7 @@ module.exports = {
   sortingPriority,
   sortByAccessCountWithAgeDecay,
   getSortForSuggestions,
-  simpleDomainNameValue,
+  isSimpleDomainNameValue,
   normalizeLocation,
   shouldNormalizeLocation,
   createVirtualHistoryItems,

app/renderer/components/navigation/urlBar.js

@@ -234,11 +234,14 @@ class UrlBar extends React.Component {
 
   updateAutocomplete (newValue) {
     let suggestion = ''
+    let suggestionNormalized = ''
     if (this.props.suggestionList && this.props.suggestionList.size > 0) {
-      suggestion = normalizeLocation(this.props.suggestionList.getIn([this.activeIndex || 0, 'location'])) || ''
+      suggestion = this.props.suggestionList.getIn([this.activeIndex || 0, 'location']) || ''
+      suggestionNormalized = normalizeLocation(suggestion)
     }
-    if (suggestion.startsWith(newValue)) {
-      const newSuffix = suggestion.substring(newValue.length)
+    const newValueNormalized = normalizeLocation(newValue)
+    if (suggestionNormalized.startsWith(newValueNormalized) && suggestionNormalized.length > 0) {
+      const newSuffix = suggestionNormalized.substring(newValueNormalized.length)
       this.setValue(newValue, newSuffix)
       this.urlInput.setSelectionRange(newValue.length, newValue.length + newSuffix.length + 1)
       return true

test/navbar-components/urlBarSuggestionsTest.js

@@ -46,7 +46,7 @@ describe('urlBarSuggestions', function () {
   it('show suggestion when single letter is typed in', function * () {
     yield this.app.client.ipcSend('shortcut-focus-url')
       .waitForElementFocus(urlInput)
-      .setInputText(urlInput, 'a')
+      .setInputText(urlInput, 'about:about')
       .waitForExist(urlBarSuggestions)
   })
 
@@ -103,13 +103,13 @@ describe('urlBarSuggestions', function () {
       .keys(Brave.keys.DOWN)
       .waitForExist(urlBarSuggestions + ' li.suggestionItem[data-index="1"].selected')
       .keys(Brave.keys.ENTER)
-      .tabByIndex(1).getUrl().should.become(this.page1Url)
+      .tabByIndex(1).getUrl().should.become(this.page2Url)
   })
 
   it('selects a location auto complete result but not for titles', function * () {
     yield this.app.client
       .setValue(urlInput, 'http://')
-      .waitForInputText(urlInput, Brave.server.urlOrigin())
+      .waitForInputText(urlInput, this.page1Url)
       .waitForExist(urlBarSuggestions + ' li.selected')
       .setValue(urlInput, 'Page')
       .waitForElementCount(urlBarSuggestions + ' li.selected', 0)
@@ -124,17 +124,16 @@ describe('urlBarSuggestions', function () {
     // so that finally, if the rest of the 1st option is entered via keyboard, it overwrites the suggestion from the mouse
     yield this.app.client
       .keys(pagePartialUrl)
-      .waitForInputText(urlInput, page2Url) // after entering partial URL matching two options, 1st is tentatively filled in (_without_ moving cursor to end)
+      .waitForInputText(urlInput, page1Url) // after entering partial URL matching two options, 1st is tentatively filled in (_without_ moving cursor to end)
       .waitForExist(urlBarSuggestions + ' li.suggestionItem')
-      .moveToObject(urlBarSuggestions + ' li.suggestionItem:not(.selected)')
       .waitForInputText(urlInput, page1Url) // mousing over 2nd option tentatively completes URL with 2nd option (_without_ moving cursor to end)
       .keys('2.html')
       .waitForInputText(urlInput, page2Url) // without moving mouse, typing rest of 1st option URL overwrites the autocomplete from mouseover
   })
 
   it('selection is not reset when pressing non-input key', function * () {
     const pagePartialUrl = Brave.server.url('page')
-    const page1Url = Brave.server.url('page1.html')
+    const page2Url = Brave.server.url('page2.html')
     aboutHistoryState.setHistory(Immutable.fromJS({
       about: {
         history: {
@@ -148,10 +147,10 @@ describe('urlBarSuggestions', function () {
       .setValue(urlInput, pagePartialUrl)
       .waitForVisible(urlBarSuggestions)
       .keys(Brave.keys.DOWN)
-      .waitForInputText(urlInput, page1Url)
+      .waitForInputText(urlInput, page2Url)
       .keys(Brave.keys.CONTROL)
       .keys(Brave.keys.CONTROL)
-      .waitForSelectedText('1.html')
+      .waitForSelectedText('2.html')
   })
 
   it('non-prefixed active suggestion loads the suggestion when enter is pressed', function * () {

test/navbar-components/urlBarTest.js

@@ -271,7 +271,7 @@ describe('urlBar tests', function () {
       it('does not show suggestions', function * () {
         yield this.app.client
           .keys('brave')
-          .waitForVisible(urlBarSuggestions, 1)
+          .waitForVisible(urlBarSuggestions)
           .ipcSend('shortcut-focus-url')
           .waitForElementFocus(urlInput)
           .waitForElementCount(urlBarSuggestions, 0)
@@ -295,12 +295,12 @@ describe('urlBar tests', function () {
         .waitForVisible(urlBarSuggestions)
         // highlight for autocomplete brianbondy.com
         .moveToObject(urlBarSuggestions, 0, 100)
-      yield selectsText(this.app.client, 'rianbondy.com')
-        .keys('ra')
+      yield selectsText(this.app.client, 'rave.com/test3')
+        .keys('rian')
         .execute(function (urlBarSuggestions) {
           document.querySelector(urlBarSuggestions).scrollTop = 200
         }, urlBarSuggestions)
-      yield selectsText(this.app.client, 've.com')
+      yield selectsText(this.app.client, 'bondy.com/test4')
     })
   })
 

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

@@ -50,11 +50,14 @@ describe('siteSuggestions lib', function () {
     it('lowercases tokens', function () {
       assert.deepEqual(tokenizeInput('BRaD HaTES PRIMES'), ['brad', 'hates', 'primes'])
     })
-    it('does not include http', function () {
-      assert.deepEqual(tokenizeInput('http://bradrichter.co/I/hate/primes.html'), ['bradrichter', 'co', 'i', 'hate', 'primes', 'html'])
+    it('includes protocol', function () {
+      assert.deepEqual(tokenizeInput('https://bradrichter.co/I/hate/primes.html'), ['bradrichter', 'co', 'i', 'hate', 'primes', 'html', 'https:'])
     })
-    it('does not include https as a token', function () {
-      assert.deepEqual(tokenizeInput('https://bradrichter.co/I/hate/primes.html'), ['bradrichter', 'co', 'i', 'hate', 'primes', 'html'])
+    it('includes query', function () {
+      assert.deepEqual(tokenizeInput('https://bradrichter.co/I/hate/primes.html?test=abc&test2=abcd'), ['bradrichter', 'co', 'i', 'hate', 'primes', 'html', 'test', 'abc', 'test2', 'abcd', 'https:'])
+    })
+    it('does not include hash', function () {
+      assert.deepEqual(tokenizeInput('https://bradrichter.co/I/hate/primes.html?test=abc#testing'), ['testing', 'bradrichter', 'co', 'i', 'hate', 'primes', 'html', 'test', 'abc', 'https:'])
     })
     it('spaces get tokenized', function () {
       assert.deepEqual(tokenizeInput('brad\thates primes'), ['brad', 'hates', 'primes'])
@@ -68,14 +71,11 @@ describe('siteSuggestions lib', function () {
     it('\\ gets tokenized', function () {
       assert.deepEqual(tokenizeInput('brad\\hates\\primes'), ['brad', 'hates', 'primes'])
     })
-    it('? gets tokenized', function () {
-      assert.deepEqual(tokenizeInput('brad?hates?primes'), ['brad', 'hates', 'primes'])
-    })
-    it('& gets tokenized', function () {
-      assert.deepEqual(tokenizeInput('brad&hates&primes'), ['brad', 'hates', 'primes'])
-    })
     it('can tokenize site objects', function () {
-      assert.deepEqual(tokenizeInput(site1), ['do', 'not', 'use', '3', 'for', 'items', 'because', 'it', 'is', 'prime', 'www', 'bradrichter', 'co', 'bad_numbers', '3'])
+      assert.deepEqual(tokenizeInput(site1), ['do', 'not', 'use', '3', 'for', 'items', 'because', 'it', 'is', 'prime', 'www', 'bradrichter', 'co', 'bad_numbers', '3', 'https:'])
+    })
+    it('non URLs get tokenized', function () {
+      assert.deepEqual(tokenizeInput('hello world Greatest...Boss...Ever'), ['hello', 'world', 'greatest', 'boss', 'ever'])
     })
   })
 
@@ -127,7 +127,7 @@ describe('siteSuggestions lib', function () {
       checkResult('brave brad', [], cb)
     })
   })
-  describe('query sorts results', function () {
+  describe('query sorts results by location', function () {
     before(function (cb) {
       const sites = Immutable.fromJS([{
         location: 'https://brave.com/twi'
@@ -173,6 +173,63 @@ describe('siteSuggestions lib', function () {
       })
     })
   })
+  describe('query sorts results by count', function () {
+    before(function (cb) {
+      this.page2 = {
+        location: 'https://brave.com/page2',
+        count: 20
+      }
+      const sites = Immutable.fromJS([{
+        location: 'https://brave.com/page1',
+        count: 5
+      }, this.page2, {
+        location: 'https://brave.com/page3',
+        count: 2
+      }])
+      init(sites).then(cb.bind(null, null))
+    })
+    it('highest count first', function (cb) {
+      query('https://brave.com/page').then((results) => {
+        assert.deepEqual(results[0], this.page2)
+        cb()
+      })
+    })
+  })
+  describe('query respects lastAccessTime', function () {
+    before(function (cb) {
+      this.site = {
+        location: 'https://bravebrowser.com/page2',
+        lastAccessedTime: 1494958046427,  // most recent
+        count: 1
+      }
+      const sites = Immutable.fromJS([{
+        location: 'https://bravez.com/page1',
+        lastAccessedTime: 1,
+        count: 1
+      }, {
+        location: 'https://bravebrowser.com/page1',
+        lastAccessedTime: 1494957046426,
+        count: 1
+      }, this.site, {
+        location: 'https://bravebrowser.com/page3',
+        lastAccessedTime: 1494957046437,
+        count: 1
+      }])
+      init(sites).then(cb.bind(null, null))
+    })
+    it('items with lastAccessTime of 1 get ignored (signifies preloaded default)', function (cb) {
+      query('https://bravez.com/page').then((results) => {
+        assert.equal(results.length, 0)
+        cb()
+      })
+    })
+    it('most recently accessed get sorted first', function (cb) {
+      query('bravebrowser').then((results) => {
+        assert.deepEqual(results[0], this.site)
+        cb()
+      })
+    })
+  })
   describe('add sites after init', function () {
     before(function (cb) {
       const sites = [site1, site2, site3, site4]

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

@@ -68,12 +68,12 @@ describe('suggestion unit tests', function () {
     })
   })
 
-  describe('simpleDomainNameValue', function () {
+  describe('isSimpleDomainNameValue', function () {
     it('sorts simple sites higher than complex sites', function () {
       const siteSimple = Immutable.Map({ location: 'http://www.site.com' })
       const siteComplex = Immutable.Map({ location: 'http://www.site.com/?foo=bar#a' })
-      assert.ok(suggestion.simpleDomainNameValue(siteSimple) === 1, 'simple site returns 1')
-      assert.ok(suggestion.simpleDomainNameValue(siteComplex) === 0, 'complex site returns 0')
+      assert.ok(suggestion.isSimpleDomainNameValue(siteSimple) === true, 'simple site returns 1')
+      assert.ok(suggestion.isSimpleDomainNameValue(siteComplex) === false, 'complex site returns 0')
     })
   })