Skip to content
Merged
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
15 changes: 10 additions & 5 deletions docs/api/notification.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,9 @@ app.whenReady().then(() => {
### `new Notification([options])`

* `options` Object (optional)
* `id` string (optional) _macOS_ - A unique identifier for the notification, mapping to `UNNotificationRequest`'s [`identifier`](https://developer.apple.com/documentation/usernotifications/unnotificationrequest/identifier) property. Defaults to a random UUID if not provided or if an empty string is passed. This can be used to remove or update previously delivered notifications.
* `groupId` string (optional) _macOS_ - A string identifier used to visually group notifications together in Notification Center. Maps to `UNNotificationContent`'s [`threadIdentifier`](https://developer.apple.com/documentation/usernotifications/unnotificationcontent/threadidentifier) property.
* `id` string (optional) _macOS_ _Windows_ - A unique identifier for the notification. On macOS, maps to `UNNotificationRequest`'s [`identifier`](https://developer.apple.com/documentation/usernotifications/unnotificationrequest/identifier) property. On Windows, maps to the toast notification's [`Tag`](https://learn.microsoft.com/en-us/uwp/api/windows.ui.notifications.toastnotification.tag) property. Defaults to a random UUID if not provided or if an empty string is passed. This can be used to remove or update previously delivered notifications.
* `groupId` string (optional) _macOS_ _Windows_ - A string identifier used to visually group notifications together in Notification Center / Action Center. On macOS, maps to `UNNotificationContent`'s [`threadIdentifier`](https://developer.apple.com/documentation/usernotifications/unnotificationcontent/threadidentifier) property. On Windows, maps to the toast notification's [`Group`](https://learn.microsoft.com/en-us/uwp/api/windows.ui.notifications.toastnotification.group) property.
* `groupTitle` string (optional) _Windows_ - A title for the notification group header. When both `groupId` and `groupTitle` are specified, Windows will display a header above the notification that groups related notifications together. Maps to the toast notification's [`header`](https://learn.microsoft.com/en-us/windows/apps/design/shell/tiles-and-notifications/toast-headers) element.
* `title` string (optional) - A title for the notification, which will be displayed at the top of the notification window when it is shown.
* `subtitle` string (optional) _macOS_ - A subtitle for the notification, which will be displayed below the title.
* `body` string (optional) - The body text of the notification, which will be displayed below the title or subtitle.
Expand Down Expand Up @@ -329,13 +330,17 @@ app.whenReady().then(() => {

### Instance Properties

#### `notification.id` _macOS_ _Readonly_
#### `notification.id` _macOS_ _Windows_ _Readonly_

A `string` property representing the unique identifier of the notification. This is set at construction time — either from the `id` option or as a generated UUID if none was provided.

#### `notification.groupId` _macOS_ _Readonly_
#### `notification.groupId` _macOS_ _Windows_ _Readonly_

A `string` property representing the group identifier of the notification. Notifications with the same `groupId` will be visually grouped together in Notification Center.
A `string` property representing the group identifier of the notification. Notifications with the same `groupId` will be visually grouped together in Notification Center (macOS) or Action Center (Windows).

#### `notification.groupTitle` _Windows_ _Readonly_

A `string` property representing the title of the notification group header.

#### `notification.title`

Expand Down
31 changes: 30 additions & 1 deletion shell/browser/api/electron_api_notification.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <windows.h>

#include "base/no_destructor.h"
#include "base/strings/utf_string_conversions.h"
#include "shell/browser/javascript_environment.h"
#include "shell/browser/notifications/win/windows_toast_activator.h"
#endif
Expand Down Expand Up @@ -76,6 +77,7 @@ Notification::Notification(gin::Arguments* args) {
if (args->GetNext(&opts)) {
opts.Get("id", &id_);
opts.Get("groupId", &group_id_);
opts.Get("groupTitle", &group_title_);
opts.Get("title", &title_);
opts.Get("subtitle", &subtitle_);
opts.Get("body", &body_);
Expand Down Expand Up @@ -108,7 +110,32 @@ gin_helper::Handle<Notification> Notification::New(
thrower.ThrowError("Cannot create Notification before app is ready");
return {};
}
return gin_helper::CreateHandle(thrower.isolate(), new Notification(args));

auto handle =
gin_helper::CreateHandle(thrower.isolate(), new Notification(args));

#if BUILDFLAG(IS_WIN)
constexpr size_t kMaxTagLength = 64;
auto* notif = handle.get();
if (!notif->id_.empty() &&
base::UTF8ToWide(notif->id_).length() > kMaxTagLength) {
thrower.ThrowError(
"Notification id exceeds Windows limit of 64 UTF-16 characters");
return {};
}
if (!notif->group_id_.empty() &&
base::UTF8ToWide(notif->group_id_).length() > kMaxTagLength) {
thrower.ThrowError(
"Notification groupId exceeds Windows limit of 64 UTF-16 characters");
return {};
}
if (!notif->group_title_.empty() && notif->group_id_.empty()) {
thrower.ThrowError("Notification groupTitle requires groupId to be set");
return {};
}
#endif
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We're parsing the options dict twice — once here via PeekNext + ConvertFromV8 to validate, and then again inside the Notification constructor. Imo that's a bit redundant and fragile since the two parse sites could diverge.

The gin::Dictionary(nullptr) + the IsEmpty() || IsUndefined() || ConvertFromV8(...)` conditional is also a bit hard to follow — the first two branches fall through into the body with a null-initialized dictionary, which feels like it shouldn't work.

Could we instead construct the Notification first and then validate the already-parsed members before returning the handle? Something like:

auto handle = gin_helper::CreateHandle(thrower.isolate(), new Notification(args));

#if BUILDFLAG(IS_WIN)
  constexpr size_t kMaxTagLength = 64;
  auto* notif = handle.get();
  if (!notif->id_.empty() &&
      base::UTF8ToWide(notif->id_).length() > kMaxTagLength) {
    thrower.ThrowError(
        "Notification id exceeds Windows limit of 64 UTF-16 characters");
    return {};
  }
  if (!notif->group_id_.empty() &&
      base::UTF8ToWide(notif->group_id_).length() > kMaxTagLength) {
    thrower.ThrowError(
        "Notification groupId exceeds Windows limit of 64 UTF-16 characters");
    return {};
  }
#endif

return handle;

That way we validate against the actual values that'll be used at Show() time, and there's a single parse path for the options.

I recognize the downside here that we construct the object before potentially discarding it on the error path, but the constructor is side-effect-free, and CreateHandle + return {} is already the pattern used for the "app not ready" early return above, so it should be safe?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done


return handle;
}

// Setters
Expand Down Expand Up @@ -259,6 +286,7 @@ void Notification::Show() {
options.urgency = urgency_;
options.toast_xml = toast_xml_;
options.group_id = group_id_;
options.group_title = group_title_;
notification_->Show(options);
}
}
Expand Down Expand Up @@ -349,6 +377,7 @@ void Notification::FillObjectTemplate(v8::Isolate* isolate,
.SetMethod("close", &Notification::Close)
.SetProperty("id", &Notification::id)
.SetProperty("groupId", &Notification::group_id)
.SetProperty("groupTitle", &Notification::group_title)
.SetProperty("title", &Notification::title, &Notification::SetTitle)
.SetProperty("subtitle", &Notification::subtitle,
&Notification::SetSubtitle)
Expand Down
2 changes: 2 additions & 0 deletions shell/browser/api/electron_api_notification.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ class Notification final : public gin_helper::DeprecatedWrappable<Notification>,
// Prop Getters
const std::string& id() const { return id_; }
const std::string& group_id() const { return group_id_; }
const std::u16string& group_title() const { return group_title_; }
const std::u16string& title() const { return title_; }
const std::u16string& subtitle() const { return subtitle_; }
const std::u16string& body() const { return body_; }
Expand Down Expand Up @@ -117,6 +118,7 @@ class Notification final : public gin_helper::DeprecatedWrappable<Notification>,
private:
std::string id_;
std::string group_id_;
std::u16string group_title_;
std::u16string title_;
std::u16string subtitle_;
std::u16string body_;
Expand Down
1 change: 1 addition & 0 deletions shell/browser/notifications/notification.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ struct NotificationOptions {
std::u16string close_button_text;
std::u16string toast_xml;
std::string group_id;
std::u16string group_title;

NotificationOptions();
NotificationOptions(const NotificationOptions&);
Expand Down
5 changes: 2 additions & 3 deletions shell/browser/notifications/win/windows_toast_activator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -539,9 +539,8 @@ void HandleToastActivation(const std::wstring& invoked_args,

Notification* target = nullptr;
for (auto* n : presenter->notifications()) {
std::wstring tag_hash =
base::NumberToWString(base::FastHash(n->notification_id()));
if (tag_hash == tag_str) {
std::wstring tag = base::UTF8ToWide(n->notification_id());
if (tag == tag_str) {
target = n;
break;
}
Expand Down
65 changes: 45 additions & 20 deletions shell/browser/notifications/win/windows_toast_notification.cc
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ void DebugLog(std::string_view log_msg) {
}

std::wstring GetTag(const std::string_view notification_id) {
return base::NumberToWString(base::FastHash(notification_id));
return base::UTF8ToWide(notification_id);
}

// See https://www.hresult.info for HRESULT error codes.
Expand Down Expand Up @@ -292,7 +292,7 @@ void WindowsToastNotification::CreateToastNotificationOnBackgroundThread(
ui_runner->PostTask(
FROM_HERE,
base::BindOnce(&WindowsToastNotification::SetupAndShowOnUIThread,
weak_notification, toast_notification));
weak_notification, toast_notification, options.group_id));
}

// Creates the toast XML document on the background thread. Returns true on
Expand Down Expand Up @@ -327,7 +327,8 @@ bool WindowsToastNotification::CreateToastXmlDocument(
std::u16string toast_xml_str =
GetToastXml(notification_id, options.title, options.msg, icon_path,
options.timeout_type, options.silent, options.actions,
options.has_reply, options.reply_placeholder);
options.has_reply, options.reply_placeholder,
options.group_id, options.group_title);

DebugLog("Toast XML (generated) id=" + notification_id + ": " +
base::UTF16ToUTF8(toast_xml_str));
Expand Down Expand Up @@ -398,7 +399,13 @@ bool WindowsToastNotification::CreateToastNotification(
return false;
}

ScopedHString group(kGroup);
// Use provided group_id if non-empty, otherwise fall back to default
std::wstring group_str =
options.group_id.empty() ? kGroup : base::UTF8ToWide(options.group_id);
ScopedHString group(group_str);
DebugLog(
base::StrCat({"Setting group: ", base::WideToUTF8(group_str),
options.group_id.empty() ? " (default)" : " (custom)"}));
hr = toast2->put_Group(group);
if (FAILED(hr)) {
std::string err = base::StrCat(
Expand All @@ -408,7 +415,10 @@ bool WindowsToastNotification::CreateToastNotification(
return false;
}

ScopedHString tag(GetTag(notification_id));
std::wstring tag_str = GetTag(notification_id);
ScopedHString tag(tag_str);
DebugLog(base::StrCat({"Setting tag: ", base::WideToUTF8(tag_str),
" (from id: ", notification_id, ")"}));
hr = toast2->put_Tag(tag);
if (FAILED(hr)) {
std::string err = base::StrCat(
Expand Down Expand Up @@ -447,7 +457,8 @@ bool WindowsToastNotification::CreateToastNotification(
// does not allow calls from background threads.
void WindowsToastNotification::SetupAndShowOnUIThread(
base::WeakPtr<Notification> weak_notification,
ComPtr<ABI::Windows::UI::Notifications::IToastNotification> notification) {
ComPtr<ABI::Windows::UI::Notifications::IToastNotification> notification,
const std::string& group_id) {
auto* notif = static_cast<WindowsToastNotification*>(weak_notification.get());
if (!notif)
return;
Expand All @@ -463,6 +474,7 @@ void WindowsToastNotification::SetupAndShowOnUIThread(
}

notif->toast_notification_ = notification;
notif->group_id_ = group_id;

// Show notification on UI thread (must be called on UI thread)
hr = (*toast_notifier_)->Show(notification.Get());
Expand Down Expand Up @@ -538,8 +550,14 @@ void WindowsToastNotification::Remove() {
if (!GetAppUserModelID(&app_id))
return;

ScopedHString group(kGroup);
ScopedHString tag(GetTag(notification_id()));
// Use stored group_id if set, otherwise fall back to default
std::wstring group_str =
group_id_.empty() ? kGroup : base::UTF8ToWide(group_id_);
ScopedHString group(group_str);
std::wstring tag_str = GetTag(notification_id());
ScopedHString tag(tag_str);
DebugLog(base::StrCat({"Removing with group: ", base::WideToUTF8(group_str),
", tag: ", base::WideToUTF8(tag_str)}));
notification_history->RemoveGroupedTagWithId(tag, group, app_id);
}

Expand All @@ -558,7 +576,9 @@ std::u16string WindowsToastNotification::GetToastXml(
bool silent,
const std::vector<NotificationAction>& actions,
bool has_reply,
const std::u16string& reply_placeholder) {
const std::u16string& reply_placeholder,
const std::string& group_id,
const std::u16string& group_title) {
XmlWriter xml_writer;
xml_writer.StartWriting();

Expand All @@ -570,6 +590,15 @@ std::u16string WindowsToastNotification::GetToastXml(
xml_writer.AddAttribute(kScenario, kReminder);
}

// Add header element if both groupId and groupTitle are present
if (!group_id.empty() && !group_title.empty()) {
Comment thread
bitdisaster marked this conversation as resolved.
xml_writer.StartElement("header");
xml_writer.AddAttribute("id", group_id);
xml_writer.AddAttribute("title", base::UTF16ToUTF8(group_title));
xml_writer.AddAttribute("arguments", "");
Comment thread
bitdisaster marked this conversation as resolved.
xml_writer.EndElement(); // </header>
}

// <visual>
xml_writer.StartElement(kVisual);
// <binding template="<template>">
Expand Down Expand Up @@ -642,9 +671,9 @@ std::u16string WindowsToastNotification::GetToastXml(
// <action>
xml_writer.StartElement(kAction);
xml_writer.AddAttribute(kActivationType, kActivationTypeForeground);
std::string args = base::StrCat(
{"type=action&action=", base::NumberToString(i),
"&tag=", base::NumberToString(base::FastHash(notification_id))});
std::string args =
base::StrCat({"type=action&action=", base::NumberToString(i),
"&tag=", notification_id});
xml_writer.AddAttribute(kArguments, args.c_str());
xml_writer.AddAttribute(kContent, base::UTF16ToUTF8(act.text));
xml_writer.EndElement(); // <action>
Expand All @@ -666,9 +695,9 @@ std::u16string WindowsToastNotification::GetToastXml(
// The button that submits the selection.
xml_writer.StartElement(kAction);
xml_writer.AddAttribute(kActivationType, kActivationTypeForeground);
std::string args = base::StrCat(
{"type=action&action=", base::NumberToString(i),
"&tag=", base::NumberToString(base::FastHash(notification_id))});
std::string args =
base::StrCat({"type=action&action=", base::NumberToString(i),
"&tag=", notification_id});
xml_writer.AddAttribute(kArguments, args.c_str());
xml_writer.AddAttribute(
kContent,
Expand All @@ -682,9 +711,7 @@ std::u16string WindowsToastNotification::GetToastXml(
// <action>
xml_writer.StartElement(kAction);
xml_writer.AddAttribute(kActivationType, kActivationTypeForeground);
std::string args =
base::StrCat({"type=reply&tag=",
base::NumberToString(base::FastHash(notification_id))});
std::string args = base::StrCat({"type=reply&tag=", notification_id});
xml_writer.AddAttribute(kArguments, args.c_str());
// TODO(codebytere): we should localize this.
xml_writer.AddAttribute(kContent, "Reply");
Expand Down Expand Up @@ -800,10 +827,8 @@ IFACEMETHODIMP ToastEventHandler::Invoke(
}

std::string notif_id;
std::string notif_hash;
if (notification_) {
notif_id = notification_->notification_id();
notif_hash = base::NumberToString(base::FastHash(notif_id));
}

bool structured = arguments_w.find(L"&tag=") != std::wstring::npos ||
Expand Down
10 changes: 8 additions & 2 deletions shell/browser/notifications/win/windows_toast_notification.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@ class WindowsToastNotification : public Notification {
const bool silent,
const std::vector<NotificationAction>& actions,
bool has_reply,
const std::u16string& reply_placeholder);
const std::u16string& reply_placeholder,
const std::string& group_id,
const std::u16string& group_title);
static HRESULT XmlDocumentFromString(
const wchar_t* xmlString,
ABI::Windows::Data::Xml::Dom::IXmlDocument** doc);
Expand Down Expand Up @@ -102,7 +104,8 @@ class WindowsToastNotification : public Notification {
toast_notification);
static void SetupAndShowOnUIThread(
base::WeakPtr<Notification> weak_notification,
ComPtr<ABI::Windows::UI::Notifications::IToastNotification> notification);
ComPtr<ABI::Windows::UI::Notifications::IToastNotification> notification,
const std::string& group_id);
static void PostNotificationFailedToUIThread(
base::WeakPtr<Notification> weak_notification,
const std::string& error,
Expand All @@ -124,6 +127,9 @@ class WindowsToastNotification : public Notification {
ComPtr<ToastEventHandler> event_handler_;
ComPtr<ABI::Windows::UI::Notifications::IToastNotification>
toast_notification_;

// Stored for Remove() to use when removing from Action Center
std::string group_id_;
};

class ToastEventHandler : public RuntimeClass<RuntimeClassFlags<ClassicCom>,
Expand Down
Loading
Loading