feat: full support for persistent HTML5 notifications #35810

YuriL180821 · 2022-09-26T07:11:20Z

Description of Change

1.Full support persistent notifications
2. Fix for the #13041
3. Full support HTML5 notifications functionality according to spec https://developer.mozilla.org/en-US/docs/Web/API/Notifications_API including support for newly added properties:

actions (including replies),
data
image
renotify
requireInteraction

Support for the specific app event notification-activation which allows notifications handling into app module
Support for start of the application from the notification when app is down(cold-start)
~~6. Double application registration(MacOS) for correct support behavior for Alerts and Banners~~
~~7. XPC Alert service integration~~

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: This pull request introduced previously absent functionality for persistent notifications and brings support for such properties as

actions (including replies),
data
image
renotify
requireInteraction

For additional information see previous pull request #34671

YuriL180821 · 2022-09-26T07:14:09Z

For additional information see previous pull request #34671

YuriL180821 · 2022-09-26T09:59:31Z

Hello Electron team!
I have concerns about correct config of existing electron static analyzer
As example
(cpplint) Is this a non-const reference? If so, make const or use a pointer: std::vector& bmpBits [runtime/references] [2]
bool GetBitmapBits(std::vector<BYTE>& bmpBits, HBITMAP hBitmap, PBITMAPINFO pBitmapInfo)
Function extracts bitmap bits and stores them into out parameter std::vector& bmpBits, also function return logic result of extraction. Why does I have lint error here regarding to make parameter as const? Who decided this? As trivial example - lets consider ordinary swap - should I have const references in it? Think it is obviously than no. Please adapt configuration of CI to exclude such false positive cases.

MarshallOfSound · 2022-09-26T10:25:06Z

Why does I have lint error here regarding to make parameter as const?

The error isn't saying "make it const" it's saying either it should be a const reference, or it should be passed as a pointer. This comes from the google style guide which we inherit and follow.

Basically if an argument is an "input" to a method it should be a const reference, if it is an "output" as in your case (it is modified as a result of the call) it should be a pointer.

In your specific case to avoid useless vector construction you could also just make the signature of the method absl::optional<std::vector<BYTE>> and have the state of the optional indicate method success / failure

YuriL180821 · 2022-09-26T10:27:46Z

Why does I have lint error here regarding to make parameter as const?

The error isn't saying "make it const" it's saying either it should be a const reference, or it should be passed as a pointer. This comes from the google style guide which we inherit and follow.

Basically if an argument is an "input" to a method it should be a const reference, if it is an "output" as in your case (it is modified as a result of the call) it should be a pointer.

Hello @MarshallOfSound !
Thank You a lot for refreshing C++ basics, but this is not that case
Basically if an argument is an "input" to a method it should be a const reference, if it is an "output" as in your case (it is modified as a result of the call) it should be a pointer.
Basically - does not mean every time and mandatory.

MarshallOfSound · 2022-09-26T10:30:01Z

@YuriL180821 I am not saying this is a rule of C++, I am saying this is a rule of Googles C++ styleguide. It is an opinionated decision but a decision nonetheless. We abide by the styleguide, if your code does not it will not pass CI and can not be merged

YuriL180821 · 2022-10-11T09:56:58Z

Hello @MarshallOfSound

I have strongly point of view that last changes in repo from Community which were merged have problems, please compare my few days old MR and MR after upgrade branch today

I do not agree that my MR brings something wrong, because changes into MacOS are absent.
Same I want to add and for Linux tests. Hope on reaction on my post.

Regards,
Yurii.

MarshallOfSound · 2022-10-11T10:37:16Z

@YuriL180821 Per the CI dashboard

the test that failed on mas-testing-x64-tests has been detected as flaky, when it comes to review that test failure will either be rerun or manually ignored. I wouldn't worry about it as long as the test has that Flaky label on it

YuriL180821 · 2022-10-11T11:12:45Z

Hello @MarshallOfSound !

Thank You for the update, but I do not understand what I have to do with this MR. As I told : my MR did not bring any changes on MacOS or Linux and in current configuration is pure Windows related feature. I'm expecting on consideration last changes and in case if ok expecting merge of them. After that I'm going to create separate pull request with changes on MacOS. If into few words : I'm trying to deploy feature into 2 steps: Win and than MacOS related functionality.

Regards,
Yurii.

MarshallOfSound · 2022-10-11T11:20:06Z

@YuriL180821 I don't believe either mine nor @zcbenz 's comments from the original PR have been appropriately addressed. Given this is a new PR I will transpose the comments this week to ensure you have a clear picture of what I consider still needs adjustment. Quick thing right now is the SAP-* comments that remain. I will have substantially more technical comments later this week but they will be basically what I commented on the previous PR

YuriL180821 · 2022-10-11T11:22:45Z

@MarshallOfSound, ok I will provide extra review for comments into my codebase.

YuriL180821 · 2022-10-12T10:16:48Z

Hello @MarshallOfSound I did clean up for SAP- related comments.
On given moment Win part of project is tested and stable
I would recommend to take a look on that problem #35899 because fix for it is necessary to push my feature. Of course I already have direct solution for bug above, but hope guys from Chromium or from Electron Community will take additional look on problem into ServiceWorkerSingleScriptUpdateChecker::OnReceiveResponse - we have direct access to null pointer, in my vision this is enough important thing. As far as I see this is conceptual problem, not a problem of absence check for nullptr.
Please look on the video below, previously registered ServiceWorker leads to crash, this issue was absent before commit mentioned into issue description above.

2022-10-12-13-07-27.mp4

…ound

… for it

This reverts commit 8669b67.

# Conflicts: # shell/browser/notifications/mac/notification_center_delegate.mm

This reverts commit 17a83e7. # Conflicts: # shell/browser/electron_browser_client.h

YuriL180821 · 2023-05-14T14:55:26Z

Hello Community, on given moment MR is ready

I had to update codebase three times only during this week to have consistency with current main, can I ask to share directly, timeline for merge or to receive understanding what is wrong again?
MR does not have dependency on any corporate sources, proposed codebase is pure my own implementation , all commits were signed and contains e-mail to contact with author(i.e me), al previously requested changes were considered and relevant adaptation of code was done, what else is necessary?

felixrieseberg · 2023-10-09T18:43:54Z

docs/api/app.md

@@ -601,7 +632,7 @@ Hides all application windows without minimizing them.

 ### `app.isHidden()` _macOS_

-Returns `boolean` - `true` if the application—including all of its windows—is hidden (e.g. with `Command-H`), `false` otherwise.
+Returns `boolean` - `true` if the application-including all of its windows???is hidden (e.g. with `Command-H`), `false` otherwise.


Heads-up: A lot of the special characters here seem to have been changed to ???!

Hello @felixrieseberg thank you for the remark.

Honestly due to continuous work of community into notifications module my branch becomes conflicting, and I have to resolve conflicts, sometimes in parts which are not related to notifications at all, this is a result of one of such resolving. Maybe I missed this moment, will take a look when have enough time.

VerteDinde

Hey @YuriL180821, thanks for your submission! I'm glad CI went green - however, it looks like there are still some issues from previous reviews that need to be addressed. I left those as specific comments below - let me know if you have questions and we can re-review after the comments have been addressed.

One issue in particular I wanted to call out: right now, the PR is written with the assumption that a process id only has one web contents. I realize this may not be a simple refactor, but I don't think we'll be able to merge the PR unless this is changed. Let me know if you have specific concerns and we can try to address them.

VerteDinde · 2023-10-23T18:56:32Z

shell/browser/notifications/platform_notification_service.cc

+  content::WebContents* web_ctx =
+      browser_client_->GetWebContentsFromProcessID(proc_id.value());
+  if (!web_ctx)


I believe @MarshallOfSound called this out in an early review, but I don't think this implementation is going to work - this assumes there is only one web contents per process ID, which may not be the case. Could we refactor this to account for multiple web contents per process id?

Hello @VerteDinde thank you for the remark.

It works fine. Honestly it is too time consuming for me to repeat things from previous discussions. Persistent notifications come from ServiceWorker which in it's turn has last NotificationContext. Internaly we store pointer on it and than by function call try to find with usage of pending process list. Process with pointer on this context is right place for notification. This is our huge difference with non-persistent notifications for which we have exact process. You can believe me or not, but in case if somebody told me the similar at least I would try to understand developer who made everything almost two years ago.

I would say more - we can use same approach and for non-persistent notifications and exclude Electron patch for Chromium in
void PlatformNotificationService::DisplayNotification( content::RenderFrameHost* render_frame_host, const std::string& notification_id,

As far as I remember content::RenderFrameHost* render_frame_host, was added by through chromium patch.

And in my vision this is not too good conceptual solution. We do need any patches, just use my approach above and for non-persistent part.

Be sure I had enough time into Debug mode to understand some not obvious things :)

Regards.

VerteDinde · 2023-10-23T18:56:56Z

shell/browser/api/electron_api_app.cc

@@ -1555,6 +1561,27 @@ std::string App::GetUserAgentFallback() {
  return ElectronBrowserClient::Get()->GetUserAgent();
 }

+#if BUILDFLAG(IS_WIN)
+// SAP-15762: Support COM activation registration at runtime


These SAP comments need to be removed

Time to time I have to resolve MR conflicts or need to add new functionality and sometimes potentially can miss internal comments which I did for my company. I will take a look when have enough time.

VerteDinde · 2023-10-23T18:57:11Z

shell/browser/api/electron_api_app.cc

+std::string App::GetBrowserClientNotificationsComServerCLSID() {
+  return ElectronBrowserClient::Get()->GetNotificationsComServerCLSID();
+}
+// SAP-21094: Application name displays in incorrect format on notification


I won't comment on each of these individually, but there are 5-6 places where SAP-XXXX is commented - can you remove those please?

Yes absolutely no need to comment all SAP comments, I will fix this

samuelmaddock · 2024-07-12T21:56:51Z

Hi @YuriL180821, I wanted to see if you'd be interesting in continuing to move this PR forward. I can bring it up to be discussed within the Electron API Working Group. Please ping @electron/wg-api if so.

It looks like there's some more work to be done around cleaning up code comments and possibly gaining more consensus around certain implementation details.

thomasbachem · 2024-12-23T19:34:55Z

@YuriL180821, you did amazing work here – can you help push it over the finish line?

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 26, 2022

YuriL180821 force-pushed the html5-notifications branch 2 times, most recently from a1a4358 to a3c635c Compare September 26, 2022 08:34

jkleinsc added the semver/major incompatible API changes label Sep 26, 2022

electron-cation bot added the api-review/requested 🗳 label Sep 26, 2022

YuriL180821 force-pushed the html5-notifications branch from 1182cae to 01196a6 Compare September 27, 2022 08:22

YuriL180821 requested review from a team as code owners September 27, 2022 08:22

YuriL180821 force-pushed the html5-notifications branch 4 times, most recently from 3e1f76f to 433139a Compare September 27, 2022 09:46

YuriL180821 force-pushed the html5-notifications branch 2 times, most recently from ea4f568 to 3f3af3b Compare October 7, 2022 08:16

jkleinsc added the no-backport label Oct 11, 2022

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 11, 2022

This was referenced Oct 12, 2022

test: check for electron build system #36004

Closed

[Bug]: ServiceWorker crash after restart #35899

Closed

jeanfbrito mentioned this pull request Oct 18, 2022

Persistent notifications for Windows #36068

Open

3 tasks

YuriL180821 added 21 commits May 14, 2023 11:06

fix: silencing IncommingCall sound and playing default sound by PlayS…

3e35815

…ound

feat: support image property for XML scheme and necessary refactoring…

1367c22

… for it

fix: swap for icon and image paths

9c2dfc1

feat: support notificationsComDisplayName property

9746861

fix: refine requireInterraction silent scheme

eab0882

fix: replace base::PostTask to content::GetUIThreadTaskRunner

1126ea6

fix: changes regarding new prototype of WebNotificationAllowed

05f154e

fix: signed / unsigned comparison

e7fd2f2

fix: lint check errors

a5e2de4

Revert "feat: initial commit for renotify feature support (MacOS)"

de76a80

This reverts commit 8669b67.

temporary exclusion MacOS environment

ce712f2

# Conflicts: # shell/browser/notifications/mac/notification_center_delegate.mm

fix: Requested landing changes for Windows platform

dff6937

fix: lint check errors

02eb717

revert changes for electron_browser_context.cc from 1359c9d

76e5bb8

Revert "fix: resolve problem with Notifications BrowserContext"

d4f6df3

This reverts commit 17a83e7. # Conflicts: # shell/browser/electron_browser_client.h

feat: make earlier exit in case if Notification::WebContents is nullptr

25f1478

feat: v8 data property processing

541a217

feat: full simplification for the HTML5 notifications feature

5cca79c

fix test for app_name in api-notification-dbus-spec.ts

60e8601

feat: impovement for WRL::Activate parameters packing into json

fa236b0

fix: replacement bare BrowserContext pointer on raw_ptr

d9fbdc6

YuriL180821 force-pushed the html5-notifications branch from 2f0a55c to d9fbdc6 Compare May 14, 2023 08:21

felixrieseberg reviewed Oct 9, 2023

View reviewed changes

sportfloh mentioned this pull request Oct 11, 2023

[Bug]: Notifications in notification center do not disappear after they have been read mattermost/desktop#2878

Closed

6 tasks

VerteDinde requested changes Oct 23, 2023

View reviewed changes

samuelmaddock added the wip ⚒ label Jul 17, 2024

thomasbachem mentioned this pull request Dec 23, 2024

serviceWorkerRegistration.showNotification does not work #13041

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: full support for persistent HTML5 notifications #35810

feat: full support for persistent HTML5 notifications #35810

YuriL180821 commented Sep 26, 2022 •

edited

Loading

YuriL180821 commented Sep 26, 2022

YuriL180821 commented Sep 26, 2022 •

edited

Loading

MarshallOfSound commented Sep 26, 2022 •

edited

Loading

YuriL180821 commented Sep 26, 2022 •

edited

Loading

MarshallOfSound commented Sep 26, 2022 •

edited

Loading

YuriL180821 commented Oct 11, 2022 •

edited

Loading

MarshallOfSound commented Oct 11, 2022

YuriL180821 commented Oct 11, 2022 •

edited

Loading

MarshallOfSound commented Oct 11, 2022

YuriL180821 commented Oct 11, 2022

YuriL180821 commented Oct 12, 2022 •

edited

Loading

YuriL180821 commented May 14, 2023 •

edited

Loading

felixrieseberg Oct 9, 2023 •

edited

Loading

YuriL180821 Oct 24, 2023

VerteDinde left a comment

VerteDinde Oct 23, 2023

YuriL180821 Oct 24, 2023 •

edited

Loading

VerteDinde Oct 23, 2023

YuriL180821 Oct 24, 2023 •

edited

Loading

VerteDinde Oct 23, 2023

YuriL180821 Oct 24, 2023

Cat-Raphael-scrib Oct 25, 2023

samuelmaddock commented Jul 12, 2024 •

edited

Loading

thomasbachem commented Dec 23, 2024

feat: full support for persistent HTML5 notifications #35810

Are you sure you want to change the base?

feat: full support for persistent HTML5 notifications #35810

Conversation

YuriL180821 commented Sep 26, 2022 • edited Loading

Description of Change

Checklist

Release Notes

YuriL180821 commented Sep 26, 2022

YuriL180821 commented Sep 26, 2022 • edited Loading

MarshallOfSound commented Sep 26, 2022 • edited Loading

YuriL180821 commented Sep 26, 2022 • edited Loading

MarshallOfSound commented Sep 26, 2022 • edited Loading

YuriL180821 commented Oct 11, 2022 • edited Loading

MarshallOfSound commented Oct 11, 2022

YuriL180821 commented Oct 11, 2022 • edited Loading

MarshallOfSound commented Oct 11, 2022

YuriL180821 commented Oct 11, 2022

YuriL180821 commented Oct 12, 2022 • edited Loading

YuriL180821 commented May 14, 2023 • edited Loading

felixrieseberg Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

YuriL180821 Oct 24, 2023

Choose a reason for hiding this comment

VerteDinde left a comment

Choose a reason for hiding this comment

VerteDinde Oct 23, 2023

Choose a reason for hiding this comment

YuriL180821 Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

VerteDinde Oct 23, 2023

Choose a reason for hiding this comment

YuriL180821 Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

VerteDinde Oct 23, 2023

Choose a reason for hiding this comment

YuriL180821 Oct 24, 2023

Choose a reason for hiding this comment

Cat-Raphael-scrib Oct 25, 2023

Choose a reason for hiding this comment

samuelmaddock commented Jul 12, 2024 • edited Loading

thomasbachem commented Dec 23, 2024

YuriL180821 commented Sep 26, 2022 •

edited

Loading

YuriL180821 commented Sep 26, 2022 •

edited

Loading

MarshallOfSound commented Sep 26, 2022 •

edited

Loading

YuriL180821 commented Sep 26, 2022 •

edited

Loading

MarshallOfSound commented Sep 26, 2022 •

edited

Loading

YuriL180821 commented Oct 11, 2022 •

edited

Loading

YuriL180821 commented Oct 11, 2022 •

edited

Loading

YuriL180821 commented Oct 12, 2022 •

edited

Loading

YuriL180821 commented May 14, 2023 •

edited

Loading

felixrieseberg Oct 9, 2023 •

edited

Loading

YuriL180821 Oct 24, 2023 •

edited

Loading

YuriL180821 Oct 24, 2023 •

edited

Loading

samuelmaddock commented Jul 12, 2024 •

edited

Loading