Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Commit

Permalink
User input starting with http doesn't sort results properly
Browse files Browse the repository at this point in the history
Fix #5578

Auditors: @aekeus

Note there is 1 more test failure I'm working on with urlbarSuggestions but it isn't related to this
  • Loading branch information
bbondy committed Nov 13, 2016
1 parent 4898dd5 commit d4e272e
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 12 deletions.
31 changes: 27 additions & 4 deletions app/renderer/lib/suggestion.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,16 +71,17 @@ module.exports.sortingPriority = (count, currentTime, lastAccessedTime, ageDecay
*
*/
module.exports.sortByAccessCountWithAgeDecay = (s1, s2) => {
const now = new Date()
const s1Priority = module.exports.sortingPriority(
s1.get('count') || 0,
(new Date()).getTime(),
s1.get('lastAccessedTime') || (new Date()).getTime(),
now.getTime(),
s1.get('lastAccessedTime') || now.getTime(),
appConfig.urlSuggestions.ageDecayConstant
)
const s2Priority = module.exports.sortingPriority(
s2.get('count') || 0,
(new Date()).getTime(),
s2.get('lastAccessedTime') || (new Date()).getTime(),
now.getTime(),
s2.get('lastAccessedTime') || now.getTime(),
appConfig.urlSuggestions.ageDecayConstant
)
return s2Priority - s1Priority
Expand Down Expand Up @@ -115,6 +116,28 @@ module.exports.normalizeLocation = (location) => {
return location
}

/*
* Determines based on user input if the location should
* be normalized. If the user is typing http prefix then
* they are specifying something explicitly.
*
* @return true if urls being compared should be normalized
*/
module.exports.shouldNormalizeLocation = (input) => {
const prefixes = ['http://', 'https://', 'www.']
return prefixes.every((prefix) => {
if (input.length > prefix.length) {
return true
}
for (let i = 0; i < Math.min(prefix.length, input.length); i++) {
if (input[i] !== prefix[i]) {
return true
}
}
return false
})
}

/*
* return a site representing the simple location for a
* set of related sites without a history item for the
Expand Down
10 changes: 6 additions & 4 deletions js/components/urlBarSuggestions.js
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,13 @@ class UrlBarSuggestions extends ImmutableComponent {
}
})

const shouldNormalize = suggestion.shouldNormalizeLocation(urlLocationLower)
const urlLocationLowerNormalized = suggestion.normalizeLocation(urlLocationLower)
const sortBasedOnLocationPos = (s1, s2) => {
const location1 = suggestion.normalizeLocation(s1.get('location'))
const location2 = suggestion.normalizeLocation(s2.get('location'))
const pos1 = location1.indexOf(urlLocationLower)
const pos2 = location2.indexOf(urlLocationLower)
const location1 = shouldNormalize ? suggestion.normalizeLocation(s1.get('location')) : s1.get('location')
const location2 = shouldNormalize ? suggestion.normalizeLocation(s2.get('location')) : s2.get('location')
const pos1 = location1.indexOf(urlLocationLowerNormalized)
const pos2 = location2.indexOf(urlLocationLowerNormalized)
if (pos1 === -1 && pos2 === -1) {
return 0
} else if (pos1 === -1) {
Expand Down
12 changes: 8 additions & 4 deletions test/components/urlBarSuggestionsTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,27 @@ describe('urlbarSuggestions', function () {

yield setup(this.app.client)
yield this.app.client
.tabByUrl(Brave.newTabUrl)
.tabByIndex(0)
.loadUrl(this.page1Url)
.windowByUrl(Brave.browserWindowUrl)
.waitUntil(function () {
return this.getAppState().then((val) => {
return val.value.sites.length === 1
})
})
.tabByIndex(0)
.loadUrl(this.page2Url)
.windowByUrl(Brave.browserWindowUrl)
.ipcSend(messages.SHORTCUT_NEW_FRAME)
.waitForUrl(Brave.newTabUrl)
.windowByUrl(Brave.browserWindowUrl)
.waitForExist('.tab[data-frame-key="2"].active')
.waitForElementFocus(urlInput)
// XXX: this wait never resolves
/*
.waitUntil(function () {
return this.getAppState().then((val) => {
return val.value.sites.length === 2
})
})
*/
})

it('show suggestion when single letter is typed in', function * () {
Expand Down
17 changes: 17 additions & 0 deletions test/unit/lib/urlSuggestionTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,23 @@ describe('suggestion', function () {
assert.ok(suggestion.simpleDomainNameValue(siteSimple) === 1, 'simple site returns 1')
assert.ok(suggestion.simpleDomainNameValue(siteComplex) === 0, 'complex site returns 0')
})

it('Determines prefixes which should be normalized', function () {
const prefixes = ['http://', 'https://', 'www.']
prefixes.forEach((prefix) => {
for (let i = 0; i < prefix.length; i++) {
const substring = prefix.substring(0, i + 1)
assert.equal(suggestion.shouldNormalizeLocation(substring), false)
}
})
})

it('Determines prefixes which should NOT be normalized', function () {
const prefixes = ['httphttp', 'brave.com', 'www3', 'http://www.x']
prefixes.forEach((prefix) => {
assert.equal(suggestion.shouldNormalizeLocation(prefix), true)
})
})
})

const site1 = Immutable.Map({
Expand Down

0 comments on commit d4e272e

Please sign in to comment.