Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Ref #1533 - Set location view text field to scheme-less domain url #1628

Merged
merged 8 commits into from
Oct 16, 2019

Conversation

chickdan
Copy link
Contributor

@chickdan chickdan commented Oct 11, 2019

Summary of Changes

This pull request fixes issue #1533

This change sets the urlTextField using the domainURL. Setting it here allows for the full url to still be accessible when the user taps into the location view.

Submitter Checklist:

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

Test Plan:

Screenshots:

Shortened domainURL
Simulator Screen Shot - iPhone Xs - 2019-10-10 at 23 49 05

Full url when location view tapped
Simulator Screen Shot - iPhone Xs - 2019-10-10 at 23 49 12

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).

@chickdan chickdan changed the title Set location view text field to scheme-less domain url Ref #1533 Set location view text field to scheme-less domain url Oct 11, 2019
@chickdan chickdan changed the title Ref #1533 Set location view text field to scheme-less domain url Ref #1533 - Set location view text field to scheme-less domain url Oct 11, 2019
iccub
iccub previously approved these changes Oct 11, 2019
Copy link
Contributor

@iccub iccub left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, the code works well :)
I'm adding blocked label, waiting on final decision from the product team

@iccub iccub added the blocked If a ticket is blocked for some reason, if not using a sub-block label, please provide info in issue label Oct 11, 2019
@iccub iccub dismissed their stale review October 14, 2019 09:10

have updates from security team

@@ -321,7 +321,7 @@ class TabLocationView: UIView {

fileprivate func updateTextWithURL() {
(urlTextField as? DisplayTextField)?.hostString = url?.host ?? ""
urlTextField.text = url?.schemelessAbsoluteString.trim("/")
urlTextField.text = url?.domainURL.schemelessAbsoluteString.trim("/")
Copy link
Contributor

Choose a reason for hiding this comment

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

This domainURL strips m. and mobile. schemes as well.
I talked to security team and they want to hide scheme only with www. case.

So one additional helper method needs to be added to URLExtensions.swift to remove only the www part

@chickdan
Copy link
Contributor Author

@iccub I've pushed an update for the requested change. Please let me know if you have any suggestions for improvement.

* E.g., https://m.foo.com/bar/baz?noo=abc#123 => https://m.foo.com/
* Or https://mobile.foo.com/bar/baz?noo=abc#123 => https://mobile.foo.com/
*/
public var mobileDomainURL: URL {
Copy link
Contributor

Choose a reason for hiding this comment

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

All of this code is duplicated and the only difference is between "^(www)\\.", "^(www|mobile|m)\\." regexes
What do you think about some DRY refactor?

  • change domainURL to a function func domainURL(stripWWWSubdomainOnly: Bool = false) -> URL
  • same for normalizedHost
  • in normalizedHost add something like let regex = stripWWWSubdomainOnly ? "^(www)\\." : "^(www|mobile|m)\\."
  • update documentation comment in domainURL
  • update testdomainURL unit test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestions! I've made the requested changes and added a new unit test to account for the stripWWWSubdomainOnly parameter.

@iccub iccub removed the blocked If a ticket is blocked for some reason, if not using a sub-block label, please provide info in issue label Oct 15, 2019
Copy link
Contributor

@iccub iccub left a comment

Choose a reason for hiding this comment

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

Great stuff, thanks :)

@iccub
Copy link
Contributor

iccub commented Oct 16, 2019

Just needs a rebase and we are good

@chickdan
Copy link
Contributor Author

conflicts resolved :)

@iccub
Copy link
Contributor

iccub commented Oct 16, 2019

CI still failing with

/Users/travis/build/brave/brave-ios/BraveShared/Analytics/UrpService.swift:28:44: cannot call value of non-function type 'URL?'
        guard let hostUrl = try? host.asURL(), let normalizedHost = hostUrl.normalizedHost() else { return nil }

@iccub iccub merged commit b103cdf into brave:development Oct 16, 2019
@chickdan chickdan deleted the locationView_display_domain_url branch October 16, 2019 18:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants