Fix #627 Add brave://search URL scheme

[KacperWybranski]

Summary of Changes

This pull request fixes part of #627.
It fix handling nonUrl text when sharing to Brave. Before, app opened blank tab. With this fix, app search for the text using default search engine in the browser.

Submitter Checklist:

• Unit Tests are updated to cover new or changed functionality
• User-facing strings use NSLocalizableString()

Test Plan:

• Open safari, go to a wikipedia article
• Select a word or a sentence, tap 'share', and share it with Brave
• Verify that text you selected was used as a search query in Brave app

Reviewer Checklist:

• Issues include necessary QA labels:
  • QA/(Yes|No)
  • release-notes/(include|exclude)
  • bug / enhancement
• Necessary security reviews have taken place.
• Adequate unit test coverage exists to prevent regressions.
• Adequate test plan exists for QA to validate (if applicable).
• Issue is assigned to a milestone (should happen at merge time).

[iccub]

Thanks for your PR and sorry for late reply, we had an important release to handle before jumping to contributor's code

I left some review, please check it out when you have time

[iccub]

This part is too duplicated, what do you think about adding new var urlOrText: String? then assign to it either url absolute string or the nonUrlText, and call rest of the code.

This way you won't need to call addingPercentEncoding two times

[KacperWybranski]

Good idea, I'll solve it this way

[iccub]

What is this change needed for?
Currently it seems to break other part of the app when you are on google.com for example, and tap on url bar, it reloads the app instead of focusing on url bar

[KacperWybranski]

I noticed that without this change, view is stuck with empty url bar and doesn't submit searching. Thanks for your comment, I haven't realized it breaks other part of the app, will try to solve that in different way.

[KacperWybranski]

I didn't remove one of the .addingPercentEncoding parts since later there is a .flatMap part which is different in both cases, but I managed to make it little simpler

[iccub]

Functionality wise code works for me now, good job
we can make more improvements to our code architecture in ShareToBraveViewController class

[iccub]

This work, but I think we can make the code cleaner now, if you look at logic below that, there is a lot of nested ifs and the code is not super readable,

For us differences between text and url are small, we can remove code duplication a little and improve architecture

What do you think if we create a new structure to ShareToBraveViewController class holding either URL or text and handling the scheme properly,
Sample code

private struct Scheme {
        private enum SchemeType {
            case url, query
        }
        
        private let type: SchemeType
        private let urlOrQuery: String
        
        init?(item: NSSecureCoding) {
            if let text = item as? String {
                urlOrQuery = text
                type = .query
            } else if let url = (item as? URL)?.absoluteString.firstURL?.absoluteString {
                urlOrQuery = url
                type = .url
            } else {
                return nil
            }
        }
        
        var schemeUrl: URL? {
            guard let percentEncodedUrlOrQuery = urlOrQuery
                    .addingPercentEncoding(withAllowedCharacters: .alphanumerics) else {
                return nil
            }
            
            // Can improve even further with adding param using `URLComponents`
            switch type {
            case .url:
                return URL(string: "brave://open-url?url=\(percentEncodedUrlOrQuery)")
            case .query:
                return URL(string: "brave://search?q=\(percentEncodedUrlOrQuery)")
            }
        }
    }

Then calling it from provider.loadItem is much cleaner, you can simply do:

guard let item = item, let schemeUrl = Scheme(item: item)?.schemeUrl else {
    self.cancel()
    return
 }
            
self.handleUrl(schemeUrl)

what do you think about it?

[iccub]

You can move it to first line in this function,
locationTextField?.text = text is set here and in early return, we can simplify it and call once at the beginning of this function

[iccub]

Thank you for adding unit test for it

[KacperWybranski]

Tried to added URLComponents logic you've mentioned, what do you think?
I also deleted ".addingPercentEncoding" part since encoding is solved by URLComponents type.

[iccub]

that's even better, nice find

[iccub]

Looks great, I will run this code for a bit and if all is good we will merge it

[iccub]

Thanks for this PR, merging it, should be live in version 1.28 early July