Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FXIOS-8821 [SwiftLint] for_where #19889

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion focus-ios/.swiftlint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ only_rules: # Only enforce these rules, ignore all others
- empty_enum_arguments
# - empty_parameters
# - empty_parentheses_with_trailing_closure
# - for_where
- for_where
- force_try
- implicit_getter
- inclusive_language
Expand Down
19 changes: 9 additions & 10 deletions focus-ios/Blockzilla/Extensions/StringExtensions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -192,29 +192,28 @@ extension String {
}

public func utf8HostToAscii() -> String {
// Check if the entire string is already in Unicode Scala
if isValidUnicodeScala(self) {
return self
}
var labels = self.components(separatedBy: ".")
for (i, part) in labels.enumerated() {
if !isValidUnicodeScala(part) {
let a = encode(part)
labels[i] = String.prefixPunycode + a
}
for (index, part) in labels.enumerated() where !isValidUnicodeScala(part) {
let encodedPart = encode(part)
labels[index] = String.prefixPunycode + encodedPart
}
let resultString = labels.joined(separator: ".")
return resultString
}

public func asciiHostToUTF8() -> String {
var labels = self.components(separatedBy: ".")
for (index, part) in labels.enumerated() {
let transformedLabels = self.components(separatedBy: ".").enumerated().map { (index, part) -> String in
if isValidPunycodeScala(part) {
let changeStr = String(part[part.index(part.startIndex, offsetBy: 4)...])
labels[index] = decode(changeStr)
return decode(changeStr)
} else {
return part
}
}
let resultString = labels.joined(separator: ".")
return resultString
return transformedLabels.joined(separator: ".")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -192,28 +192,27 @@ private class TPStatsBlocklists {
return nil
}

domainSearch: for rule in rules {
// First, test the top-level filters to see if this URL might be blocked.
if rule.regex.firstMatch(in: resourceString, options: .anchored, range: resourceRange) != nil {
// Check the domain exceptions. If a domain exception matches, this filter does not apply.
for domainRegex in (rule.domainExceptions ?? []) {
if domainRegex.firstMatch(in: resourceString, options: [], range: resourceRange) != nil {
continue domainSearch
}
for rule in rules {
guard let _ = rule.regex.firstMatch(in: resourceString, options: .anchored, range: resourceRange) else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add the comment back in:
// First, test the top-level filters to see if this URL might be blocked.

continue
}
let hasDomainException = rule.domainExceptions?.contains { domainRegex in
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add the comment back here as well:

// Check the domain exceptions. If a domain exception matches, this filter does not apply.

domainRegex.firstMatch(in: resourceString, options: [], range: resourceRange) != nil
} ?? false
guard !hasDomainException else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Although guarding can make the early exits more readable in this case the inversion makes me a bit confused.
I would use if hasDomainException { continue } implying that if we indeed find an exception we skip to the next rule.

continue
}
if let baseDomain = url.baseDomain, !permittedDomains.isEmpty {
Copy link
Contributor

Choose a reason for hiding this comment

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

And this comment should be added back too:

// Check the whitelist.

let range = NSRange(location: 0, length: baseDomain.count)
let isPermittedDomain = permittedDomains.contains { ignoreDomain in
ignoreDomain.firstMatch(in: baseDomain, options: [], range: range) != nil
}

// Check the whitelist.
if let baseDomain = url.baseDomain, !permittedDomains.isEmpty {
let range = NSRange(location: 0, length: baseDomain.count)
for ignoreDomain in permittedDomains {
if ignoreDomain.firstMatch(in: baseDomain, options: [], range: range) != nil {
return nil
}
}
guard !isPermittedDomain else {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it is a permitted domain we should return nil instead of continuing.

continue
}

return rule.list
}

return rule.list
}

return nil
Expand Down
12 changes: 4 additions & 8 deletions focus-ios/Blockzilla/Utilities/KeyboardHelper.swift
Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,11 @@ public class KeyboardHelper: NSObject {
* Delegates are weakly held.
*/
public func addDelegate(delegate: KeyboardHelperDelegate) {
for weakDelegate in delegates {
// Reuse any existing slots that have been deallocated.
if weakDelegate.delegate == nil {
weakDelegate.delegate = delegate
return
}
if let availableSlot = delegates.first(where: { $0.delegate == nil }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the comment back:

// Reuse any existing slots that have been deallocated.

availableSlot.delegate = delegate
} else {
delegates.append(WeakKeyboardDelegate(delegate))
}

delegates.append(WeakKeyboardDelegate(delegate))
}

@objc func keyboardWillShow(notification: NSNotification) {
Expand Down
18 changes: 5 additions & 13 deletions focus-ios/Blockzilla/Utilities/SearchHistoryUtils.swift
Original file line number Diff line number Diff line change
Expand Up @@ -27,29 +27,21 @@ class SearchHistoryUtils {

static func pushSearchToStack(with searchedText: String) {
// Check whether the lastSearch is the current search. If not, remove subsequent searches
if let lastSearch = currentStack.last, !lastSearch.isCurrentSearch {
for index in 0..<currentStack.count {
if currentStack[index].isCurrentSearch {
currentStack.removeSubrange(index+1..<currentStack.count)
break
}
}
if let lastIndex = currentStack.firstIndex(where: { !$0.isCurrentSearch }) {
currentStack.removeSubrange(lastIndex + 1..<currentStack.count)
Comment on lines +30 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

This change doesn't look correct. The stack should only be cleaned up if the lastSearch is not the current search.

Suggested change
if let lastIndex = currentStack.firstIndex(where: { !$0.isCurrentSearch }) {
currentStack.removeSubrange(lastIndex + 1..<currentStack.count)
// Check whether the lastSearch is the current search. If not, remove subsequent searches
if let lastSearch = currentStack.last,
!lastSearch.isCurrentSearch,
let lastIndex = currentStack.firstIndex(where: { !$0.isCurrentSearch }) {
currentStack.removeSubrange(lastIndex + 1..<currentStack.count)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, the change might not be correct. We want go and look for searches to be removed only if the last search is not the current search. Then we look for the first search that is current search and remove everything after it (not including it). After that we mark everything as not being current search and only afteeer that we add the new search and mark it accordingly. Ugh that was a lot. A similar implementation the might be more clear can be:

static func pushSearchToStack(with searchedText: String) {
    // Check if the stack is empty and directly append if it is
    guard !currentStack.isEmpty else {
        currentStack.append(textSearched(text: searchedText, isCurrentSearch: true))
        return
    }

    // Find the last `current search` and truncate the stack beyond this point
    if let lastIndex = currentStack.lastIndex(where: { $0.isCurrentSearch }) {
        currentStack = Array(currentStack.prefix(upTo: lastIndex + 1))
    }

    // Mark all as not current in a single pass and append new search
    currentStack = currentStack.map { textSearched(text: $0.text, isCurrentSearch: false) }
    currentStack.append(textSearched(text: searchedText, isCurrentSearch: true))
}

To show an example we start with
0: "apple" (isCurrentSearch = false)
1: "banana" (isCurrentSearch = false)
2: "cherry" (isCurrentSearch = true)
3: "date" (isCurrentSearch = false)

Then we truncate after index 2
0: "apple" (isCurrentSearch = false)
1: "banana" (isCurrentSearch = false)
2: "cherry" (isCurrentSearch = true)

We set everything to false
0: "apple" (isCurrentSearch = false)
1: "banana" (isCurrentSearch = false)
2: "cherry" (isCurrentSearch = false)

Then we add the search
0: "apple" (isCurrentSearch = false)
1: "banana" (isCurrentSearch = false)
2: "cherry" (isCurrentSearch = false)
3: "elderberry" (isCurrentSearch = true)

}

for index in 0..<currentStack.count {
for index in currentStack.indices {
currentStack[index].isCurrentSearch = false
}

currentStack.append(textSearched(text: searchedText, isCurrentSearch: true))
}

static func pullSearchFromStack() -> String? {
for search in currentStack {
if search.isCurrentSearch {
return search.text
}
for search in currentStack where search.isCurrentSearch {
return search.text
}

return nil
}

Expand Down
7 changes: 3 additions & 4 deletions focus-ios/Blockzilla/Utilities/WebCacheUtils.swift
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,9 @@ class WebCacheUtils {
// Delete other remnants in the cache directory, such as HSTS.plist.
if let cachesPath = NSSearchPathForDirectoriesInDomains(.cachesDirectory, .userDomainMask, true).first {
let contents = (try? FileManager.default.contentsOfDirectory(atPath: cachesPath)) ?? []
for file in contents {
if !PermittedFolderList.contains(file) {
FileManager.default.removeItemAndContents(path: "\(cachesPath)/\(file)")
}
let filesToRemove = contents.filter { !PermittedFolderList.contains($0) }
filesToRemove.forEach {
FileManager.default.removeItemAndContents(path: "\(cachesPath)/\($0)")
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,6 @@ public extension UIView {
}

func removeGradient() {
if let buttonSublayers = self.layer.sublayers {
for layer in buttonSublayers {
if layer is CAGradientLayer {
layer.removeFromSuperlayer()
}
}
}
self.layer.sublayers?.compactMap { $0 as? CAGradientLayer }.forEach { $0.removeFromSuperlayer() }
}
}