This repository has been archived by the owner on May 10, 2024. It is now read-only.
Fix #1042: Implement DomainAndRegistry for eTLD+1 #5239
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Brandon-T
requested review from
kylehickinson,
nuo-xu,
StephenHeaps,
soner-yuksel and
iccub
iccub
reviewed
Apr 11, 2022
Brandon-T
force-pushed
the
bugfix/eTLDPlus1
branch
from
8861635 to
60b798a
Compare
iccub
approved these changes
Apr 11, 2022
|
Is there any way we can expose a way to extract an etld from a host string and not a url? I suppose I can cheat a bit and create a url with a fixed scheme if not. |
|
Merging this one after security review passes, any subsequent improvements to the url helper methods will be handled as new tickets cc @cuba |
25 tasks
Brandon-T
force-pushed
the
bugfix/eTLDPlus1
branch
from
81563f6 to
eb6d2f4
Compare
cuba
approved these changes
Apr 13, 2022
| XCTAssertEqual("d.kawasaki.jp", expected) | ||
| XCTAssertEqual("d.kawasaki.jp", url.host?.publicSuffix) | ||
| } | ||
|
|
||
| func testExceptionDomain() { | ||
| // TLD Entry: !city.kawasaki.jp | ||
| let url = "http://city.kawasaki.jp".asURL! | ||
| let expected = url.publicSuffix! |
There was a problem hiding this comment.
Maybe remove that ! You're testing it's not nil so I don't think you should put that ! there plus you don't really need it. Both would result in a failed test but one will give you some better test information.
| @@ -144,41 +179,47 @@ class NSURLExtensionsTests: XCTestCase { | |||
| let url = "http://bbc.co.uk".asURL! | |||
| let expected = url.baseDomain! | |||
There was a problem hiding this comment.
Same as above and all the rest below.
Brandon-T
force-pushed
the
bugfix/eTLDPlus1
branch
from
eb6d2f4 to
6d3377f
Compare
thypon
reviewed
Apr 18, 2022
Brandon-T
force-pushed
the
bugfix/eTLDPlus1
branch
from
6d3377f to
b50ef96
Compare
Brandon-T
force-pushed
the
bugfix/eTLDPlus1
branch
from
b50ef96 to
4efd31a
Compare
thypon
approved these changes
Apr 21, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Security Review
Summary of Changes
domainAndRegistryanddomainAndRegistryExcludingPrivateRegistries.registry(public suffix)This pull request fixes #1042
Submitter Checklist:
NSLocalizableString()Test Plan:
public_suffix_list: https://publicsuffix.org/list/public_suffix_list.datprintpopup abuse the user: https://brandon-t.github.io/WindowPrint.html)
- Test Sharing a URL (youtube, twitter, bbc.co.uk, etc) - Test History and recent searches showing properly (also test recent searches not different from previous build when upgrading) - Test BraveNews still worksReviewer Checklist:
QA/(Yes|No)bug/enhancement