Skip to content
This repository has been archived by the owner. It is now read-only.

Fixes URL Parsing failing for query string and file name with space #13898

Merged
merged 1 commit into from Jun 18, 2018

Conversation

@AlexWang-16
Copy link

AlexWang-16 commented Apr 23, 2018

Fixes #13897

Screenshots of fixes

  1. Resolving google search URL with query string with space

google_search_fix

  1. Resolving windows path with space in file name

path_windows_fix

  1. Resolving Mac/Linux absolute path with space in file name

path_mac_fix

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.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

  • run test on urlutil by executing: npm test -- --grep="urlutil"

  • Type in address bar: C:\path\to\file should show file:///c:/path/to/file

  • Type in address bar C:\path\to\file\with space should show file:///C:/path/to/file/with%20space

  • Type in address bar http://www.google.ca/search?q=dog cat should result in a google search of "dog cat"

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

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
@AlexWang-16 AlexWang-16 changed the title Issue 13897 Issue #13897 Apr 23, 2018
@NejcZdovc NejcZdovc requested review from bsclifton and diracdeltas Apr 23, 2018
@AlexWang-16 AlexWang-16 changed the title Issue #13897 Fixes #13897 Apr 23, 2018
@diracdeltas

This comment has been minimized.

Copy link
Member

diracdeltas commented Apr 24, 2018

@AlexWang-16 can you rebase this? we moved some of the URL util tests around

@AlexWang-16

This comment has been minimized.

Copy link
Author

AlexWang-16 commented Apr 25, 2018

@diracdeltas I'm trying to rebase these commits after pulling the latest additions from the master branch of this project and resolved all conflicts.

I've rebased all of my own commits, but now I'm running into a lot of conflicts as it attempts to rebase commits that are before mine. I've tried skipping a few, but other files are also showing conflicts, thus I've stopped the process to gain more clarification.

I'm new to rebasing so I don't want to mess up the original code base. Currently I'm performing rebase on my own branch so I think I have at least that part correct (please correct me if I am not).

Would you please provide me with some guidance on how to proceed?

Here's the log.

@NejcZdovc

This comment has been minimized.

Copy link
Member

NejcZdovc commented Apr 25, 2018

@AlexWang-16 I would suggest that you squash commits into one and then rebase only once. This way you are less error prone.

@AlexWang-16

This comment has been minimized.

Copy link
Author

AlexWang-16 commented Apr 26, 2018

Thanks @NejcZdovc. I've now squashed and rebased my commits. However, I'm noticing that a urlutil test is not passing.

  1. muon tests "before all" hook:
    Error: Promise was rejected with the following reason: timeout
    at windowHandles() - brave.js:278:32

Error seems to be coming from windowHandlesOrig.apply(this) call, which was traced to line 251 of brave.js. However, I'm not able to infer to the reason of why the promise was rejected. My educated guess would be that there was some error while retrieving the window handle.

Yet, this attempt at guessing what the problem may be does not provide me with any clues on how to solve this problem.

Any thoughts on this?

@diracdeltas

This comment has been minimized.

Copy link
Member

diracdeltas commented Apr 26, 2018

@AlexWang-16 to run the webdriver tests, you have to run npm run watch in a separate terminal before running the tests. to just run the unittests, do npm run unittest

Copy link
Member

diracdeltas left a comment

lgtm, though i didn't test on a windows machine to see if the paths are resolvable. thanks for adding tests.

@bsclifton bsclifton changed the title Fixes #13897 Fixes URL Parsing failing for query string and file name with space Apr 30, 2018
@bsclifton bsclifton added this to the Completed work milestone Apr 30, 2018
@bsclifton bsclifton mentioned this pull request May 1, 2018
0 of 10 tasks complete
@diracdeltas diracdeltas modified the milestones: Completed work, 0.25.x (coming soon) May 8, 2018
* Added tests to ensure Windows file path works
* Added tests to ensure Unix aboslute path works
* Added test to ensure entering http://www.google.ca/search?q=dog cat
results in proper google search of "dog cat"
* Added test case to ensure isNotURL() returns true when the input is a pure query string

Fixes #13897

Auditors:

* NejcZdovc
* diracdeltas
* bsclifton

Test Plan:
* run test on urlutil by executing: npm test -- --grep="urlutil"

* Type in address bar: C:\path\to\file should show
file:///c:/path/to/file

* Type in address bar C:\path\to\file\with space should show
file:///C:/path/to/file/with%20space

* Type in address bar http://www.google.ca/search?q=dog cat should
result in a google search of "dog cat"
@codecov codecov bot deleted a comment from codecov-io Jun 18, 2018
@codecov codecov bot deleted a comment from codecov-io Jun 18, 2018
Copy link
Member

bsclifton left a comment

++

@bsclifton bsclifton merged commit fa9f022 into brave:master Jun 18, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
bsclifton added a commit that referenced this pull request Jun 18, 2018
Fixes URL Parsing failing for query string and file name with space
@bsclifton

This comment has been minimized.

Copy link
Member

bsclifton commented Jun 18, 2018

master fa9f022
0.24.x 485a11b

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.