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

Fixes duplicated root domains #10839

Merged
merged 1 commit into from
Sep 18, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Sep 7, 2017

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Resolves #10826
Resolves #9827

Auditors:

Test Plan: specified in #9827

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@NejcZdovc NejcZdovc added this to the 0.21.x (Nightly Channel) milestone Sep 7, 2017
@NejcZdovc NejcZdovc self-assigned this Sep 7, 2017
@NejcZdovc NejcZdovc force-pushed the hotfix/#10826-suggestions branch from f11c610 to d93dd7c Compare September 7, 2017 10:37
@codecov-io
Copy link

codecov-io commented Sep 7, 2017

Codecov Report

Merging #10839 into master will increase coverage by 0.02%.
The diff coverage is 95%.

@@            Coverage Diff             @@
##           master   #10839      +/-   ##
==========================================
+ Coverage   54.21%   54.24%   +0.02%     
==========================================
  Files         249      249              
  Lines       21816    21836      +20     
  Branches     3395     3399       +4     
==========================================
+ Hits        11827    11844      +17     
- Misses       9989     9992       +3
Flag Coverage Δ
#unittest 54.24% <95%> (+0.02%) ⬆️
Impacted Files Coverage Δ
js/lib/urlutil.js 85.53% <100%> (+0.37%) ⬆️
app/common/lib/suggestion.js 68.46% <100%> (+0.11%) ⬆️
app/common/lib/siteSuggestions.js 89.62% <83.33%> (-0.3%) ⬇️

@NejcZdovc
Copy link
Contributor Author

travis is passing now for all related tests

  20) urlBar tests autocomplete suffix should clear when it is not a prefix match:
     Error: Promise was rejected with the following reason: timeout
      at elements("#urlInput") - getValue.js:18:17
      at getValue("#urlInput") - brave.js:523:23

  21) urlBar tests autocomplete press escape key "before each" hook for "does select the urlbar text":
     Error: element (".urlBarSuggestions li") still not existing after 10000ms
      at elements(".urlBarSuggestions li") - isExisting.js:46:17
      at isExisting(".urlBarSuggestions li") - waitForExist.js:67:22

  22) urlBar tests autocomplete highlight suggestions with tab autofills from selected suggestion:
     Error: Promise was rejected with the following reason: timeout
      at elements("#urlInput") - getValue.js:18:17
      at getValue("#urlInput") - brave.js:523:23

  23) urlBar tests autocomplete highlight suggestions autofills from selected suggestion:
     Error: Promise was rejected with the following reason: timeout
      at elements("#urlInput") - getValue.js:18:17
      at getValue("#urlInput") - brave.js:523:23


let location = data.get('location') || ''
location = urlUtil.stripLocation(location)
return location + (data.get('partitionNumber') ? '|' + data.get('partitionNumber') : '')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid doing multiple lookups.

let location = data.get('location') || ''
let partitionNumber = data.get('partitionNumber')
return location + (partitionNumber ? '|' + partitionNumber : '')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@NejcZdovc NejcZdovc force-pushed the hotfix/#10826-suggestions branch from d93dd7c to b66f5a8 Compare September 10, 2017 14:29
@ghost ghost added the sprint/1 label Sep 13, 2017
stripLocation: (url) => {
return url
.replace(/\/*$/, '') // remove trailing /
.replace(/#*$/, '') // remove trailing #
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think you can combine these into one regex to be faster:
.replace(/\/?#?$/, '')

Copy link
Contributor Author

@NejcZdovc NejcZdovc Sep 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I saw that it would be great to first remove # and then /, so that would links like https://tweetdeck.twitter.com/# become https://tweetdeck.twitter.com.

So old code would be changed to:

return url
      .replace(/#*$/, '') // remove trailing  #
      .replace(/\/*$/, '')  // remove trailing /

where we would remove trailing # and then check new string for trailing /

This is what I put together .replace(/((#?\/?)|(\/#?))$/, '')

@bbondy What do you think about new regex?

@NejcZdovc NejcZdovc force-pushed the hotfix/#10826-suggestions branch from b66f5a8 to 2664db1 Compare September 17, 2017 10:25
Resolves brave#10826
Resolves brave#9827

Auditors:

Test Plan:
@NejcZdovc NejcZdovc force-pushed the hotfix/#10826-suggestions branch from 2664db1 to 83b3341 Compare September 18, 2017 05:13
@NejcZdovc NejcZdovc merged commit c2874cc into brave:master Sep 18, 2017
@bbondy bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants