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.

Sign up for GitHub

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

Notifications are not shown on macOS caused by #3651 #3892

Merged
merged 1 commit into from
Nov 7, 2019
+58 −3
Merged

Notifications are not shown on macOS caused by #3651 #3892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion components/brave_ads/browser/notification_helper_mac.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ class NotificationHelperMac
NotificationHelperMac();
~NotificationHelperMac() override;

bool IsNotificationsEnabled() const;
bool IsEnabled() const;
bool IsAuthorized() const;

// NotificationHelper impl
bool ShouldShowNotifications() override;
Expand Down
58 changes: 56 additions & 2 deletions components/brave_ads/browser/notification_helper_mac.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,12 @@
return true;
}

if (!IsNotificationsEnabled()) {
if (!IsAuthorized()) {
LOG(INFO) << "Notification not made: User denied authorization";
return false;
}

if (!IsEnabled()) {
LOG(INFO) << "Notification not made: Notifications are disabled";
return false;
}
Expand All @@ -64,7 +69,52 @@

///////////////////////////////////////////////////////////////////////////////

bool NotificationHelperMac::IsNotificationsEnabled() const {
bool NotificationHelperMac::IsAuthorized() const {
#if !defined(OFFICIAL_BUILD)
LOG(WARNING) << "Unable to detect the status of native notifications on non"
" official builds as the app is not code signed";
return true;
#else
// TODO(https://openradar.appspot.com/27768556): We must mock this function
// using NotificationHelperMock as a workaround to UNUserNotificationCenter
// throwing an exception during tests

if (@available(macOS 10.14, *)) {
__block bool is_authorized = false;

dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);

UNUserNotificationCenter *notificationCenter =
[UNUserNotificationCenter currentNotificationCenter];

[notificationCenter requestAuthorizationWithOptions:UNAuthorizationOptionAlert
completionHandler:^(BOOL granted, NSError * _Nullable error) {
if (granted) {
LOG(INFO) << "User granted authorization to show notifications";
} else {
LOG(WARNING) << "User denied authorization to show notifications";
}

is_authorized = granted;

dispatch_semaphore_signal(semaphore);
}];

dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER);
Copy link
Collaborator

@kylehickinson kylehickinson Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This (and the same usage below) is fine for now, but we should move away from this ASAP, as if Apple change the callbacks from UNUserNotificationCenter to execute async on main thread, this wait will block main thread and thus this process forever.

It is explicitly stated on this APIs that they may execute on a background thread: https://developer.apple.com/documentation/usernotifications/unusernotificationcenter/1649527-requestauthorizationwithoptions?language=objc

However this API has been executed on a background thread in iOS for years now, so though its unlikely to be changed any time soon, it can always happen as it doesn't explicitly say it will always run on a background thread

Fine with this as long as its a temporary solution.

dispatch_release(semaphore);

if (!is_authorized) {
LOG(WARNING) << "User is not authorized to show notifications";
}

return is_authorized;
}

return true;
#endif
}

bool NotificationHelperMac::IsEnabled() const {
#if !defined(OFFICIAL_BUILD)
LOG(WARNING) << "Unable to detect the status of native notifications on non"
" official builds as the app is not code signed";
Expand Down Expand Up @@ -116,6 +166,10 @@
dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER);
dispatch_release(semaphore);

if (!is_authorized) {
LOG(WARNING) << "Notifications not authorized";
}

return is_authorized;
}

Expand Down