Bug 234370 – Add a "NotificationData" object to encompass local Notification-related parameters, instead of passing tons of them around everywhere

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Add a "NotificationData" object to encompass local Notification-related parameters, instead of passing tons of them around everywhere

[Brady Eidson]

Created attachment 447295 [details]
Patch v1

[Tim Horton]

Comment on attachment 447295 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=447295&action=review

> Source/WebCore/Modules/notifications/NotificationData.h:49
> +    encoder << title << body << iconURL << tag << language << direction << originString << notificationID;

I feel like traditionally we do one-per-line, though there's nothing wrong with this.

[Tim Horton]

Comment on attachment 447295 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=447295&action=review

> Source/WebCore/Modules/notifications/NotificationData.h:49
> +    encoder << title << body << iconURL << tag << language << direction << originString << notificationID;

I feel like traditionally we do one-per-line, though there's nothing wrong with this.

[Brady Eidson]

Created attachment 447296 [details]
PFL v1

[Brady Eidson]

Created attachment 447300 [details]
PFL v2

[Brady Eidson]

Created attachment 447304 [details]
PFL v3

[EWS]

Committed r287124 (245308@main): <https://commits.webkit.org/245308@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447304 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86555440>

[Darin Adler]

Comment on attachment 447304 [details]
PFL v3

View in context: https://bugs.webkit.org/attachment.cgi?id=447304&action=review

> Source/WebCore/Modules/notifications/NotificationData.cpp:31
> +namespace WebCore {
> +
> +} // namespace WebCore

What’s the rationale for creating an empty source file?

[Brady Eidson]

(In reply to Darin Adler from comment #9)
> Comment on attachment 447304 [details]
> PFL v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447304&action=review
> 
> > Source/WebCore/Modules/notifications/NotificationData.cpp:31
> > +namespace WebCore {
> > +
> > +} // namespace WebCore
> 
> What’s the rationale for creating an empty source file?

There was definitely a build/link error resolved by it, but I definitely forget what it was.

[Brady Eidson]

This patch had a pretty subtle bug which caused a few different types of test flakiness.

Look at this file closely and see if you can spot it.
https://trac.webkit.org/changeset/287124/webkit/trunk/Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp

Good news - That type of subtle bug within the "two different notification identifiers" system is why I embarked upon https://bugs.webkit.org/show_bug.cgi?id=234534 which should fix it.

[Darin Adler]

(In reply to Brady Eidson from comment #10)
> (In reply to Darin Adler from comment #9)
> > > Source/WebCore/Modules/notifications/NotificationData.cpp:31
> > > +namespace WebCore {
> > > +
> > > +} // namespace WebCore
> > 
> > What’s the rationale for creating an empty source file?
> 
> There was definitely a build/link error resolved by it, but I definitely
> forget what it was.

Let’s follow up on this. Doesn’t make logical sense that we need an empty source file, and I am pretty sure we can remove it.