From da41b7e2821ee7800a0f992394296f7042cc5ffa Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Sun, 21 May 2017 20:36:43 -0400 Subject: [PATCH] Fix various suggestion sort issues Fix #8994 Auditors: @bsclifton - Handle bookmarks that appear twice in folders with different counts. - Handle comparing items with no counts. - Handle comparing items with no lastAccessedTime - Handle sorting 2 sites when one is a substring of the other, match the shorter one first. - Fixed an issue with Immutable data sometimes being used in the sorting functions, and adds an assert to make sure it's correctly used. - Support `javascript:` bookmarklets - Avoid re-sorting suggestions that are generated - Adds lots of tests --- app/common/lib/siteSuggestions.js | 7 +- app/common/lib/suggestion.js | 69 ++++--- test/navbar-components/urlBarTest.js | 2 +- .../app/common/lib/siteSuggestionsTest.js | 172 ++++++++++++------ test/unit/app/common/lib/suggestionTest.js | 113 ++++++++---- 5 files changed, 251 insertions(+), 112 deletions(-) diff --git a/app/common/lib/siteSuggestions.js b/app/common/lib/siteSuggestions.js index f826ccf9f33..e2c40e21255 100644 --- a/app/common/lib/siteSuggestions.js +++ b/app/common/lib/siteSuggestions.js @@ -25,8 +25,13 @@ const getSiteIdentity = (data) => { } const init = (sites) => { + sites = sites.toJS ? sites.toJS() : sites + // Sort sites with smaller count first because later ones will overwrite with correct counts based on the site identity. + // This can happen when a user bookmarks the same site multiple times, but only one of the items are getting counts + // incremented by normal operations. + sites = sites.sort((s1, s2) => (s1.count || 0) - (s2.count || 0)) engine = new Bloodhound({ - local: sites.toJS ? sites.toJS() : sites, + local: sites, sorter: sortForSuggestions, queryTokenizer: tokenizeInput, datumTokenizer: tokenizeInput, diff --git a/app/common/lib/suggestion.js b/app/common/lib/suggestion.js index 935fb3188b0..b68045a5d43 100644 --- a/app/common/lib/suggestion.js +++ b/app/common/lib/suggestion.js @@ -17,6 +17,7 @@ const fetchSearchSuggestions = require('./fetchSearchSuggestions') const {getFrameByTabId, getTabsByWindowId} = require('../../common/state/tabState') const {query} = require('./siteSuggestions') const debounce = require('../../../js/lib/debounce') +const assert = require('assert') const sigmoid = (t) => { return 1 / (1 + Math.pow(Math.E, -t)) @@ -34,12 +35,12 @@ const ONE_DAY = 1000 * 60 * 60 * 24 * */ const sortingPriority = (count, currentTime, lastAccessedTime, ageDecayConstant) => { + count = Math.max(count, Number.EPSILON) // number of days since last access (with fractional component) - const ageInDays = (currentTime - (lastAccessedTime || currentTime)) / ONE_DAY - // decay factor based on age - const ageFactor = 1 - ((sigmoid(ageInDays / ageDecayConstant) - 0.5) * 2) + const ageInDays = (currentTime - (lastAccessedTime || 1)) / ONE_DAY + // Decay factor based on age, having a maximum of just less than 1 ensures the return will not be 0 + const ageFactor = 1 - ((Math.min(sigmoid(ageInDays / ageDecayConstant), 1 - Number.EPSILON) - 0.5) * 2) // sorting priority - // console.log(count, ageInDays, ageFactor, count * ageFactor) return count * ageFactor } @@ -86,13 +87,13 @@ const sortByAccessCountWithAgeDecay = (s1, s2) => { const s1Priority = sortingPriority( s1.count || 0, now.getTime(), - s1.lastAccessedTime || now.getTime(), + s1.lastAccessedTime || 0, appConfig.urlSuggestions.ageDecayConstant ) const s2Priority = sortingPriority( s2.count || 0, now.getTime(), - s2.lastAccessedTime || now.getTime(), + s2.lastAccessedTime || 0, appConfig.urlSuggestions.ageDecayConstant ) return s2Priority - s1Priority @@ -172,12 +173,12 @@ var virtualSite = (sites) => { // if there are no simple locations then we will build and return one if (simple.length === 0) { // we need to create a virtual history item - return Immutable.Map({ + return { location: sites[0].protocol + '//' + sites[0].host, count: 0, title: sites[0].host, lastAccessedTime: (new Date()).getTime() - }) + } } } @@ -186,17 +187,17 @@ var virtualSite = (sites) => { * The simple locations will be the root domain for a location * without parameters or path * - * @param {ImmutableList[ImmutableMap]} - history + * @param {object} - history */ -const createVirtualHistoryItems = (historySites) => { - historySites = makeImmutable(historySites || {}) +const createVirtualHistoryItems = (historySites, urlLocationLower) => { + historySites = historySites || [] // parse each history item const parsedHistorySites = [] historySites.forEach((site) => { - if (site && site.get('location')) { + if (site && site.location) { parsedHistorySites.push( - urlParse(site.get('location')) + urlParse(site.location) ) } }) @@ -215,9 +216,12 @@ const createVirtualHistoryItems = (historySites) => { virtualHistorySites = _.filter(virtualHistorySites, (site) => { return !!site }) - return Immutable.fromJS(_.object(virtualHistorySites.map((site) => { - return [site.get('location'), site] - }))) + + if (urlLocationLower) { + virtualHistorySites = virtualHistorySites.filter((vs) => vs.location.indexOf(urlLocationLower) !== -1) + } + + return virtualHistorySites } /** @@ -290,8 +294,11 @@ const sortBySimpleURL = (s1, s2) => { if (!url1IsSecure && url2IsSecure) { return 1 } + } - // Prefer smaller less complicated domains + // Prefer smaller less complicated domains + // hostname could be null in cases like javascript: bookmarklets + if (s1.parsedUrl.hostname && s2.parsedUrl.hostname) { const parts1 = s1.parsedUrl.hostname.split('.') const parts2 = s2.parsedUrl.hostname.split('.') let parts1Size = parts1.length @@ -308,9 +315,8 @@ const sortBySimpleURL = (s1, s2) => { if (parts1Size > parts2Size) { return 1 } - return sortByAccessCountWithAgeDecay(s1, s2) } - return 0 + return sortByAccessCountWithAgeDecay(s1, s2) } /** @@ -327,6 +333,14 @@ const getSortByPath = (userInputLower) => { if (pos1 === -1 && pos2 !== -1) { return 1 } + const indexOf1 = path1.indexOf(path2) + const indexOf2 = path2.indexOf(path1) + if (indexOf1 === -1 && indexOf2 !== -1) { + return -1 + } + if (indexOf1 !== -1 && indexOf2 === -1) { + return 1 + } // Can't determine what is the best match return 0 } @@ -344,6 +358,9 @@ const getSortForSuggestions = (userInputLower) => { const sortByPath = getSortByPath(userInputLower) return (s1, s2) => { + if (s1.toJS || s2.toJS) { + assert('These sorting functions are not meant for Immutable data!') + } s1.parsedUrl = s1.parsedUrl || urlParse(getURL(s1) || '') s2.parsedUrl = s2.parsedUrl || urlParse(getURL(s2) || '') @@ -379,7 +396,7 @@ const getURL = (x) => { } const getMapListToElements = (urlLocationLower) => ({data, maxResults, type, - sortHandler = (x) => x, filterValue = (site) => { + filterValue = (site) => { return site.toLowerCase().indexOf(urlLocationLower) !== -1 } }) => { @@ -398,7 +415,6 @@ const getMapListToElements = (urlLocationLower) => ({data, maxResults, type, } return makeImmutable(filteredData - .sort(sortHandler) .take(maxResults) .map((site) => { return Immutable.fromJS({ @@ -412,7 +428,6 @@ const getMapListToElements = (urlLocationLower) => ({data, maxResults, type, const getHistorySuggestions = (state, urlLocationLower) => { return new Promise((resolve, reject) => { - const sortHandler = getSortForSuggestions(urlLocationLower) const mapListToElements = getMapListToElements(urlLocationLower) const options = { historySuggestionsOn: getSetting(settings.HISTORY_SUGGESTIONS), @@ -420,15 +435,15 @@ const getHistorySuggestions = (state, urlLocationLower) => { } query(urlLocationLower, options).then((results) => { + results = results.concat(createVirtualHistoryItems(results, urlLocationLower)) + const sortHandler = getSortForSuggestions(urlLocationLower) + results = results.sort(sortHandler) + results = results.slice(0, config.urlBarSuggestions.maxHistorySites) results = makeImmutable(results) - results = results.take(config.urlBarSuggestions.maxHistorySites) - results = results.concat(createVirtualHistoryItems(results)) - const suggestionsList = mapListToElements({ data: results, maxResults: config.urlBarSuggestions.maxHistorySites, type: options.historySuggestionsOn ? suggestionTypes.HISTORY : suggestionTypes.BOOKMARK, - sortHandler, filterValue: null }) resolve(suggestionsList) @@ -450,7 +465,6 @@ const getAboutSuggestions = (state, urlLocationLower) => { const getOpenedTabSuggestions = (state, windowId, urlLocationLower) => { return new Promise((resolve, reject) => { - const sortHandler = getSortForSuggestions(urlLocationLower) const mapListToElements = getMapListToElements(urlLocationLower) const tabs = getTabsByWindowId(state, windowId) let suggestionsList = Immutable.List() @@ -459,7 +473,6 @@ const getOpenedTabSuggestions = (state, windowId, urlLocationLower) => { data: tabs, maxResults: config.urlBarSuggestions.maxOpenedFrames, type: suggestionTypes.TAB, - sortHandler, filterValue: (tab) => !isSourceAboutUrl(tab.get('url')) && !tab.get('active') && ( diff --git a/test/navbar-components/urlBarTest.js b/test/navbar-components/urlBarTest.js index c35e1d0ba18..fb3f536b5e7 100644 --- a/test/navbar-components/urlBarTest.js +++ b/test/navbar-components/urlBarTest.js @@ -332,7 +332,7 @@ describe('urlBar tests', function () { .waitForVisible(urlBarSuggestions) // highlight for autocomplete brianbondy.com .moveToObject(urlBarSuggestions, 0, 100) - yield selectsText(this.app.client, 'rave.com/test3') + yield selectsText(this.app.client, 'rave.com/test2') .keys('rian') .execute(function (urlBarSuggestions) { document.querySelector(urlBarSuggestions).scrollTop = 200 diff --git a/test/unit/app/common/lib/siteSuggestionsTest.js b/test/unit/app/common/lib/siteSuggestionsTest.js index 96e5fc48586..9f6d521f4c7 100644 --- a/test/unit/app/common/lib/siteSuggestionsTest.js +++ b/test/unit/app/common/lib/siteSuggestionsTest.js @@ -19,6 +19,13 @@ const site3 = { } const site4 = { + location: 'https://www.designers.com/brad', + title: 'Brad Saves The World!', + count: 50 +} + +// Same as site4 but added after in init, should be ignored. +const site5 = { location: 'https://www.designers.com/brad', title: 'Brad Saves The World!' } @@ -112,9 +119,10 @@ describe('siteSuggestions lib', function () { checkResult('hello', [], cb) }) }) + describe('query', function () { before(function (cb) { - const sites = [site1, site2, site3, site4] + const sites = [site1, site2, site3, site4, site5] init(sites).then(cb.bind(null, null)) }) it('can query with empty string', function (cb) { @@ -197,59 +205,120 @@ describe('siteSuggestions lib', function () { }) }) describe('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)) + describe('with lastAccessedTime', function () { + before(function (cb) { + const lastAccessedTime = 1494958046427 + this.page2 = { + location: 'https://brave.com/page2', + lastAccessedTime, + count: 20 + } + const sites = Immutable.fromJS([{ + location: 'https://brave.com/page1', + lastAccessedTime, + count: 5 + }, this.page2, { + location: 'https://brave.com/page3', + lastAccessedTime, + count: 2 + }]) + init(sites).then(cb.bind(null, null)) + }) + it('highest count first', function (cb) { + query('https://brave.com/page').then((results) => { + siteEqual(results[0], this.page2) + cb() + }) + }) }) - it('highest count first', function (cb) { - query('https://brave.com/page').then((results) => { - siteEqual(results[0], this.page2) - cb() + describe('without last access time', 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) => { + siteEqual(results[0], this.page2) + cb() + }) }) }) }) - describe('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() + describe('sorts results by lastAccessTime', function () { + describe('with counts', 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) => { + siteEqual(results[0], this.site) + cb() + }) }) }) - it('most recently accessed get sorted first', function (cb) { - query('bravebrowser').then((results) => { - siteEqual(results[0], this.site) - cb() + describe('without counts', function () { + before(function (cb) { + this.site = { + location: 'https://bravebrowser.com/page2', + lastAccessedTime: 1494958046427 // most recent + } + const sites = Immutable.fromJS([{ + location: 'https://bravez.com/page1', + lastAccessedTime: 1 + }, { + location: 'https://bravebrowser.com/page1', + lastAccessedTime: 1494957046426 + }, this.site, { + location: 'https://bravebrowser.com/page3', + lastAccessedTime: 1494957046437 + }]) + 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) => { + siteEqual(results[0], this.site) + cb() + }) }) }) }) @@ -267,11 +336,12 @@ describe('siteSuggestions lib', function () { it('can be found', function (cb) { checkResult('slack', [{ location: 'https://slack.com' }], cb) }) - it('adding twice results in 1 result only', function (cb) { + it('adding twice results in 1 result only with latest results', function (cb) { add({ - location: 'https://slack.com' + location: 'https://slack.com', + count: 30 }) - checkResult('slack', [{ location: 'https://slack.com' }], cb) + checkResult('slack', [{ location: 'https://slack.com', count: 30 }], cb) }) it('can add simple strings', function (cb) { add({ diff --git a/test/unit/app/common/lib/suggestionTest.js b/test/unit/app/common/lib/suggestionTest.js index ecf536bfc75..bfc81e980db 100644 --- a/test/unit/app/common/lib/suggestionTest.js +++ b/test/unit/app/common/lib/suggestionTest.js @@ -10,7 +10,7 @@ const Immutable = require('immutable') const urlParse = require('url').parse const {makeImmutable} = require('../../../../../app/common/state/immutableUtil') const fakeElectron = require('../../../lib/fakeElectron') -const _ = require('underscore') + let suggestion require('../../../braveUnit') @@ -98,66 +98,69 @@ describe('suggestion unit tests', function () { }) describe('createVirtualHistoryItems', function () { - const site1 = Immutable.Map({ + const site1 = { location: 'http://www.foo.com/1', count: 0, lastAccessedTime: 0, title: 'www.foo/com/1' - }) + } - const site2 = Immutable.Map({ + const site2 = { location: 'http://www.foo.com/2', count: 0, lastAccessedTime: 0, title: 'www.foo/com/2' - }) + } - const site3 = Immutable.Map({ + const site3 = { location: 'http://www.foo.com/3', count: 0, lastAccessedTime: 0, title: 'www.foo/com/3' - }) + } it('handles input being null/undefined', function () { - const emptyResult = Immutable.Map() + const emptyResult = [] assert.deepEqual(suggestion.createVirtualHistoryItems(), emptyResult) assert.deepEqual(suggestion.createVirtualHistoryItems(undefined), emptyResult) assert.deepEqual(suggestion.createVirtualHistoryItems(null), emptyResult) }) it('handles entries with unparseable "location" field', function () { - const badInput = makeImmutable({ - site1: { + const badInput = [ + { location: undefined - }, - site2: { + }, { location: null - }, - site3: { + }, { location: '' - }, - site4: { + }, { location: 'httphttp://lol.com' } - }) + ] + assert.ok(suggestion.createVirtualHistoryItems(badInput)) }) - it('calls immutableUtil.makeImmutable', function () { - const callCount = makeImmutableSpy.withArgs({}).callCount - suggestion.createVirtualHistoryItems() - assert.equal(makeImmutableSpy.withArgs({}).callCount, callCount + 1) + it('shows virtual history item', function () { + var history = [site1, site2, site3] + var virtual = suggestion.createVirtualHistoryItems(history) + assert.ok(virtual.length > 0, 'virtual location created') + assert.ok(virtual[0].location === 'http://www.foo.com') + assert.ok(virtual[0].title === 'www.foo.com') + assert.ok(virtual[0].lastAccessedTime > 0) }) - it('shows virtual history item', function () { - var history = Immutable.List([site1, site2, site3]) - var virtual = suggestion.createVirtualHistoryItems(history).toJS() - var keys = _.keys(virtual) - assert.ok(keys.length > 0, 'virtual location created') - assert.ok(virtual[keys[0]].location === 'http://www.foo.com') - assert.ok(virtual[keys[0]].title === 'www.foo.com') - assert.ok(virtual[keys[0]].lastAccessedTime > 0) + it('filters virtual sites to only matching input', function () { + var history = [{ + location: 'https://www.google.com/test', + title: 'Google 1!' + }, { + location: 'https://www.google.com/blah', + title: 'Google 2!' + }] + var virtual = suggestion.createVirtualHistoryItems(history, 'google.ca') + assert.equal(virtual.length, 0) }) }) @@ -181,6 +184,11 @@ describe('suggestion unit tests', function () { it('matches even domain name for input string', function () { assert.equal(this.sort('https://brave.com/facebook', 'https://twitter.com'), 1) }) + it('sorts shortest path when one is a prefix of another and they have a match', function () { + assert(this.sort('https://www.twitter.com/brian', 'https://www.twitter.com/brian/test1') < 0) + assert(this.sort('https://www.twitter.com/brian/test1', 'https://www.twitter.com/brian') > 0) + assert.equal(this.sort('https://www.twitter.com/brian/test1', 'https://www.twitter.com/brian/test1'), 0) + }) }) describe('sortBySimpleURL', function () { before(function () { @@ -256,17 +264,20 @@ describe('suggestion unit tests', function () { it('does not throw error for file:// URL', function () { assert(this.sort('https://google.com', 'file://') < 0) }) + it('does not throw error for javascript: bookmarklets', function () { + assert.equal(this.sort('javascript:alert(document.links[0].href)', 'javascript:alert(document.links[1].href)'), 0) + }) it('sorts simple domains that match equally on subdomains as the same', function () { const url1 = 'https://facebook.github.com' const url2 = 'https://facebook.brave.com' const sort = suggestion.getSortByDomain('facebook', 'facebook') - assert(sort({ + assert.equal(sort({ location: url1, parsedUrl: urlParse(url1) }, { location: url2, parsedUrl: urlParse(url2) - }) === 0) + }), 0) }) it('sorts simple domains that match equally but have different activity based on activity', function () { const url1 = 'https://facebook.github.com' @@ -341,6 +352,46 @@ describe('suggestion unit tests', function () { } assert(sort('https://facebook.github.io/', 'https://www.facebook.com/') > 0) }) + it('sorts better matched domains based on more simple with paths ignoring www.', function () { + const userInputLower = 'facebook' + const internalSort = suggestion.getSortForSuggestions(userInputLower, userInputLower) + const sort = (url1, url2) => { + return internalSort( + { location: url1, parsedUrl: urlParse(url1) }, + { location: url2, parsedUrl: urlParse(url2) } + ) + } + assert(sort('https://facebook.github.io/brian', 'https://www.facebook.com/brian') > 0) + }) + it('hierarchical input does not select shortest match domain', function () { + const userInputLower = 'facebook.com/brian' + const internalSort = suggestion.getSortForSuggestions(userInputLower, userInputLower) + const sort = (url1, url2) => { + return internalSort( + { location: url1, parsedUrl: urlParse(url1) }, + { location: url2, parsedUrl: urlParse(url2) } + ) + } + assert.equal(sort('https://www.facebook.com/brian2', 'https://www.facebook.com/brian/test1'), 0) + }) + it('prefix matches1', function () { + const userInputLower = 'twi' + const url1 = 'https://twitter.com/' + const url2 = 'https://www.twilio.com/' + const sort = suggestion.getSortForSuggestions(userInputLower, userInputLower) + assert(sort({ + location: url1, + title: 'Twitter. It\'s what\'s happening.', + parsedUrl: urlParse(url1), + count: 2, + lastAccessedTime: 1495376582112 + }, { + location: url2, + title: 'Twilio - APIs for Text Messaging, VoIP & Voice in the Cloud', + parsedUrl: urlParse(url2), + lastAccessedTime: 0 + }) < 0) + }) }) }) })