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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor FXIOS-10467 - Remove force_cast violations from Sistem services & Logging #23150

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions BrowserKit/Sources/Common/Logger/CrashManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ public class DefaultCrashManager: CrashManager {
private var sentryWrapper: SentryWrapper
private var isSimulator: Bool
private var skipReleaseNameCheck: Bool
private let logger: Logger

// Only enable app hang tracking in Beta for now
private var shouldEnableAppHangTracking: Bool {
Expand All @@ -90,11 +91,13 @@ public class DefaultCrashManager: CrashManager {
public init(appInfo: BrowserKitInformation = BrowserKitInformation.shared,
sentryWrapper: SentryWrapper = DefaultSentry(),
isSimulator: Bool = DeviceInfo.isSimulator(),
skipReleaseNameCheck: Bool = false) {
skipReleaseNameCheck: Bool = false,
logger: Logger = DefaultLogger.shared) {
self.appInfo = appInfo
self.sentryWrapper = sentryWrapper
self.isSimulator = isSimulator
self.skipReleaseNameCheck = skipReleaseNameCheck
self.logger = logger
}

// MARK: - CrashManager protocol
Expand Down Expand Up @@ -128,8 +131,12 @@ public class DefaultCrashManager: CrashManager {
// Turn Sentry breadcrumbs off since we have our own log swizzling
options.enableAutoBreadcrumbTracking = false
options.beforeSend = { event in
if event.error.self is CustomCrashReport {
self.alterEventForCustomCrash(event: event, crash: event.error as! CustomCrashReport)
if let crashReport = event.error.self as? CustomCrashReport {
self.alterEventForCustomCrash(event: event, crash: crashReport)
} else {
self.logger.log("Encountered an error that is not a CustomCrashReport: \(String(describing: event.error))",
level: .fatal,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also here i'd go to .info

category: .lifecycle)
}
return event
}
Expand Down
39 changes: 35 additions & 4 deletions firefox-ios/Client/Helpers/MenuBuilderHelper.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,17 @@
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/

import Common
import UIKit
import Shared

class MenuBuilderHelper {
private let logger: Logger

init(logger: Logger = DefaultLogger.shared) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

That looks good, i'd only move the init after the structs, so the init is with the other methods

self.logger = logger
}

struct MenuIdentifiers {
static let history = UIMenu.Identifier("com.mozilla.firefox.menus.history")
static let bookmarks = UIMenu.Identifier("com.mozilla.firefox.menus.bookmarks")
Expand Down Expand Up @@ -67,7 +74,13 @@ class MenuBuilderHelper {
]
)
fileMenu.children.forEach {
($0 as! UIKeyCommand).wantsPriorityOverSystemBehavior = true
guard let fileMenuKeyCommand = $0 as? UIKeyCommand else {
logger.log("Failed to cast file menu child to UIKeyCommand in MenuBuilderHelper class",
level: .fatal,
Copy link
Collaborator

@FilippoZazzeroni FilippoZazzeroni Nov 15, 2024

Choose a reason for hiding this comment

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

i'd log those as a .info level since there is no crash there and we don't send them to sentry as well.
Same for the other logs. i'd also use .lifecycle i see there isn't a proper one for menu but i guess lifecycle is more general

category: .library)
return
}
fileMenuKeyCommand.wantsPriorityOverSystemBehavior = true
}

let findMenu = UIMenu(
Expand All @@ -90,7 +103,13 @@ class MenuBuilderHelper {
]
)
findMenu.children.forEach {
($0 as! UIKeyCommand).wantsPriorityOverSystemBehavior = true
guard let findMenuKeyCommand = $0 as? UIKeyCommand else {
logger.log("Failed to cast find menu child to UIKeyCommand in MenuBuilderHelper class",
level: .fatal,
category: .library)
return
}
findMenuKeyCommand.wantsPriorityOverSystemBehavior = true
}

var viewMenuChildren: [UIMenuElement] = [
Expand Down Expand Up @@ -137,7 +156,13 @@ class MenuBuilderHelper {

let viewMenu = UIMenu(options: .displayInline, children: viewMenuChildren)
viewMenu.children.forEach {
($0 as! UIKeyCommand).wantsPriorityOverSystemBehavior = true
guard let viewMenuKeyCommand = $0 as? UIKeyCommand else {
logger.log("Failed to cast view menu child to UIKeyCommand in MenuBuilderHelper class",
level: .fatal,
category: .library)
return
}
viewMenuKeyCommand.wantsPriorityOverSystemBehavior = true
}

let historyMenu = UIMenu(
Expand Down Expand Up @@ -255,7 +280,13 @@ class MenuBuilderHelper {

if #available(iOS 15, *) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can also remove the #if available now we support from iOS 15

windowMenu.children.forEach {
($0 as! UIKeyCommand).wantsPriorityOverSystemBehavior = true
guard let windowMenuKeyCommand = $0 as? UIKeyCommand else {
logger.log("Failed to cast window menu child to UIKeyCommand in MenuBuilderHelper class",
level: .fatal,
category: .library)
return
}
windowMenuKeyCommand.wantsPriorityOverSystemBehavior = true
}
}

Expand Down