feat: allow null when subscribing notification (electron#33641)

* feat: allow null when subscribing notification * docs: document null event
solarwindscloud · Feb 22, 2023 · 2347ea8 · 2347ea8
1 parent 0225c0c
commit 2347ea8
Show file tree

Hide file tree

Showing 4 changed files with 67 additions and 15 deletions.
diff --git a/docs/api/system-preferences.md b/docs/api/system-preferences.md
@@ -84,7 +84,7 @@ that contains the user information dictionary sent along with the notification.
 
 ### `systemPreferences.subscribeNotification(event, callback)` _macOS_
 
-* `event` string
+* `event` string | null
 * `callback` Function
   * `event` string
   * `userInfo` Record<string, unknown>
@@ -109,9 +109,11 @@ example values of `event` are:
 * `AppleColorPreferencesChangedNotification`
 * `AppleShowScrollBarsSettingChanged`
 
+If `event` is null, the `NSDistributedNotificationCenter` doesn’t use it as criteria for delivery to the observer. See [docs](https://developer.apple.com/documentation/foundation/nsnotificationcenter/1411723-addobserverforname?language=objc)  for more information.
+
 ### `systemPreferences.subscribeLocalNotification(event, callback)` _macOS_
 
-* `event` string
+* `event` string | null
 * `callback` Function
   * `event` string
   * `userInfo` Record<string, unknown>
@@ -122,9 +124,11 @@ Returns `number` - The ID of this subscription
 Same as `subscribeNotification`, but uses `NSNotificationCenter` for local defaults.
 This is necessary for events such as `NSUserDefaultsDidChangeNotification`.
 
+If `event` is null, the `NSNotificationCenter` doesn’t use it as criteria for delivery to the observer. See [docs](https://developer.apple.com/documentation/foundation/nsnotificationcenter/1411723-addobserverforname?language=objc) for more information.
+
 ### `systemPreferences.subscribeWorkspaceNotification(event, callback)` _macOS_
 
-* `event` string
+* `event` string | null
 * `callback` Function
   * `event` string
   * `userInfo` Record<string, unknown>
@@ -135,6 +139,8 @@ Returns `number` - The ID of this subscription
 Same as `subscribeNotification`, but uses `NSWorkspace.sharedWorkspace.notificationCenter`.
 This is necessary for events such as `NSWorkspaceDidActivateApplicationNotification`.
 
+If `event` is null, the `NSWorkspaceNotificationCenter` doesn’t use it as criteria for delivery to the observer. See [docs](https://developer.apple.com/documentation/foundation/nsnotificationcenter/1411723-addobserverforname?language=objc) for more information.
+
 ### `systemPreferences.unsubscribeNotification(id)` _macOS_
 
 * `id` Integer

diff --git a/shell/browser/api/electron_api_system_preferences.h b/shell/browser/api/electron_api_system_preferences.h
@@ -76,17 +76,17 @@ class SystemPreferences
   void PostNotification(const std::string& name,
                         base::DictionaryValue user_info,
                         gin::Arguments* args);
-  int SubscribeNotification(const std::string& name,
+  int SubscribeNotification(v8::Local<v8::Value> maybe_name,
                             const NotificationCallback& callback);
   void UnsubscribeNotification(int id);
   void PostLocalNotification(const std::string& name,
                              base::DictionaryValue user_info);
-  int SubscribeLocalNotification(const std::string& name,
+  int SubscribeLocalNotification(v8::Local<v8::Value> maybe_name,
                                  const NotificationCallback& callback);
   void UnsubscribeLocalNotification(int request_id);
   void PostWorkspaceNotification(const std::string& name,
                                  base::DictionaryValue user_info);
-  int SubscribeWorkspaceNotification(const std::string& name,
+  int SubscribeWorkspaceNotification(v8::Local<v8::Value> maybe_name,
                                      const NotificationCallback& callback);
   void UnsubscribeWorkspaceNotification(int request_id);
   v8::Local<v8::Value> GetUserDefault(v8::Isolate* isolate,
@@ -130,7 +130,7 @@ class SystemPreferences
   ~SystemPreferences() override;
 
 #if BUILDFLAG(IS_MAC)
-  int DoSubscribeNotification(const std::string& name,
+  int DoSubscribeNotification(v8::Local<v8::Value> maybe_name,
                               const NotificationCallback& callback,
                               NotificationCenterKind kind);
   void DoUnsubscribeNotification(int request_id, NotificationCenterKind kind);

diff --git a/shell/browser/api/electron_api_system_preferences_mac.mm b/shell/browser/api/electron_api_system_preferences_mac.mm
@@ -154,10 +154,11 @@ AVMediaType ParseMediaType(const std::string& media_type) {
 }
 
 int SystemPreferences::SubscribeNotification(
-    const std::string& name,
+    v8::Local<v8::Value> maybe_name,
     const NotificationCallback& callback) {
   return DoSubscribeNotification(
-      name, callback, NotificationCenterKind::kNSDistributedNotificationCenter);
+      maybe_name, callback,
+      NotificationCenterKind::kNSDistributedNotificationCenter);
 }
 
 void SystemPreferences::UnsubscribeNotification(int request_id) {
@@ -174,9 +175,9 @@ AVMediaType ParseMediaType(const std::string& media_type) {
 }
 
 int SystemPreferences::SubscribeLocalNotification(
-    const std::string& name,
+    v8::Local<v8::Value> maybe_name,
     const NotificationCallback& callback) {
-  return DoSubscribeNotification(name, callback,
+  return DoSubscribeNotification(maybe_name, callback,
                                  NotificationCenterKind::kNSNotificationCenter);
 }
 
@@ -196,10 +197,11 @@ AVMediaType ParseMediaType(const std::string& media_type) {
 }
 
 int SystemPreferences::SubscribeWorkspaceNotification(
-    const std::string& name,
+    v8::Local<v8::Value> maybe_name,
     const NotificationCallback& callback) {
   return DoSubscribeNotification(
-      name, callback, NotificationCenterKind::kNSWorkspaceNotificationCenter);
+      maybe_name, callback,
+      NotificationCenterKind::kNSWorkspaceNotificationCenter);
 }
 
 void SystemPreferences::UnsubscribeWorkspaceNotification(int request_id) {
@@ -208,14 +210,25 @@ AVMediaType ParseMediaType(const std::string& media_type) {
 }
 
 int SystemPreferences::DoSubscribeNotification(
-    const std::string& name,
+    v8::Local<v8::Value> maybe_name,
     const NotificationCallback& callback,
     NotificationCenterKind kind) {
   int request_id = g_next_id++;
   __block NotificationCallback copied_callback = callback;
 
+  v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
+  std::string name_str;
+  if (!(maybe_name->IsNull() ||
+        gin::ConvertFromV8(isolate, maybe_name, &name_str))) {
+    isolate->ThrowException(v8::Exception::Error(
+        gin::StringToV8(isolate, "Must pass null or a string")));
+    return -1;
+  }
+
+  auto* name = maybe_name->IsNull() ? nil : base::SysUTF8ToNSString(name_str);
+
   g_id_map[request_id] = [GetNotificationCenter(kind)
-      addObserverForName:base::SysUTF8ToNSString(name)
+      addObserverForName:name
                   object:nil
                    queue:nil
               usingBlock:^(NSNotification* notification) {

diff --git a/spec-main/api-system-preferences-spec.ts b/spec-main/api-system-preferences-spec.ts
@@ -116,6 +116,39 @@ describe('systemPreferences module', () => {
     });
   });
 
+  ifdescribe(process.platform === 'darwin')('systemPreferences.subscribeNotification(event, callback)', () => {
+    it('throws an error if invalid arguments are passed', () => {
+      const badArgs = [123, {}, ['hi', 'bye'], new Date()];
+      for (const bad of badArgs) {
+        expect(() => {
+          systemPreferences.subscribeNotification(bad as any, () => {});
+        }).to.throw('Must pass null or a string');
+      }
+    });
+  });
+
+  ifdescribe(process.platform === 'darwin')('systemPreferences.subscribeLocalNotification(event, callback)', () => {
+    it('throws an error if invalid arguments are passed', () => {
+      const badArgs = [123, {}, ['hi', 'bye'], new Date()];
+      for (const bad of badArgs) {
+        expect(() => {
+          systemPreferences.subscribeNotification(bad as any, () => {});
+        }).to.throw('Must pass null or a string');
+      }
+    });
+  });
+
+  ifdescribe(process.platform === 'darwin')('systemPreferences.subscribeWorkspaceNotification(event, callback)', () => {
+    it('throws an error if invalid arguments are passed', () => {
+      const badArgs = [123, {}, ['hi', 'bye'], new Date()];
+      for (const bad of badArgs) {
+        expect(() => {
+          systemPreferences.subscribeWorkspaceNotification(bad as any, () => {});
+        }).to.throw('Must pass null or a string');
+      }
+    });
+  });
+
   ifdescribe(process.platform === 'darwin')('systemPreferences.getSystemColor(color)', () => {
     it('throws on invalid system colors', () => {
       const color = 'bad-color';