Whitespace in url fixes #3167

[Sh1d0w]

• Submitted a ticket for my issue if one did not already exist. Brave can't handle URLs with whitespace in it #3167
• Used Github auto-closing keywords in the commit message.
• Ran git rebase -i to squash commits if needed.

cc @bbondy @bridiver @bsclifton

This PR allows to open valid URLs whith whitespace in it such as

https://s10.postimg.org/snp1s3aah/Screenshot+from 2016-08-13 17-50-27.png

The original behaviour to query the search engine is preserved if you type a word or set of words. But if the string starts with scheme, no matter it has whitespace in it it will try to load the url.

What is the behaviour now?

String typed in the url bar	Before	Now
google.com	open https://google.com	open https://google.com
test	searches for test	searches for test
test test	searches for test test	searches for test test
http://example.com/test image.jpg	searches for http://example.com/test image.jpg via default search engine	opens http://example.com/test image.jpg

[neeklamy]

Should probably update this comment about whitespace and searching too.

[bridiver]

Thanks for the PR! Can you add some unit tests for this in urlUtilTest.js? I'm also not sure this is 100% correct because while spaces are legal in the path (encoded as %20), they are not legal in a domain name or the scheme. my protocol://blah, test domain.com, http: //domain.com, etc.. should return false for isUrl

[Sh1d0w]

@bridiver You are completely right. I have fixed this behaviour and added tests as you requested.

Do you want me to refactor appUrlUtil all across the codebase or it is in the scope of another ticket?

[bridiver]

I think those might just be examples and not an exhaustive list. We also have registered and app protocols like bitcoin, 1password, etc... @bbondy ?

[bridiver]

Thanks! Can you add just a few more tests to cover all the changes? The other cases I can think of are

1. multiple words
2. space in schema
3. an arbitrary registered protocol
   if those pass I think this is good to merge

[bridiver]

can you do an npm run lint and fix any issues? The CI tests are failing on some lint issues

[Sh1d0w]

@bridiver I think linting is failing generally even on master, it is not a fault on my branch.

Can you please tell me what are the registered protocols for lastpass, bitcoint ect, so I can add them to the test as well? I believe they should invoke chrome-extension:// protocol, no?

[bridiver]

at least some of the lint errors appear to be related to these changes https://travis-ci.org/brave/browser-laptop/builds/152446285
don't worry about any lint errors that are not related to you changes, but feel free to fix them :)
I wouldn't worry too much about the exact protocols, but maybe add chrome-extension just to verify that - is valid for an extension. Maybe also check that chrome extension:// fails?

[bridiver]

lint normally runs as a precommit hook on other platforms, but doesn't run on windows. I'm switching it to a pre-push hook that should work everywhere

[Sh1d0w]

@bridiver Fixed linting. Added few more test cases.

Let me know if you find anything else that seems not ok.

[bridiver]

Looks good! Just waiting for CI to finish and I'll merge if everything looks good there. I know some tests are broken in master right now so I'm only looking for yours to pass.

[Sh1d0w]

@bridiver Cool, thanks man!

[bridiver]

++