Bug 232021 – Factor out some Notifications-specific messages from WebPageProxy messages

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Factor out some Notifications-specific messages from WebPageProxy messages

Some of these messages will - in the future - sometimes be sent to Networking and sometimes sent to UIProcess for a WebPageProxy to handle.

So let's factoring that out.

[Brady Eidson]

Created attachment 441895 [details]
Patch v1

[Alex Christensen]

Comment on attachment 441895 [details]
Patch v1

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

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerMessageHandler.h:36
> +public:
> +    WebNotificationManagerMessageHandler(WebPageProxy&);

You might consider making the constructor private and befriending WebPageProxy so that only WebPageProxy can construct it.  We usually try to avoid friend, but I think in this case it would be a reasonable choice.

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerMessageHandler.h:39
> +    void showNotification(const String& title, const String& body, const String& iconURL, const String& tag, const String& lang, WebCore::NotificationDirection, const String& originString, uint64_t notificationID) final;

nit: lang should be a full word.

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerMessageHandler.h:40
> +    void cancelNotification(uint64_t notificationID) final;

nit: this should be an ObjectIdentifier of some type.

[Brady Eidson]

(In reply to Alex Christensen from comment #2)
> Comment on attachment 441895 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441895&action=review
> 
> > Source/WebKit/UIProcess/Notifications/WebNotificationManagerMessageHandler.h:36
> > +public:
> > +    WebNotificationManagerMessageHandler(WebPageProxy&);
> 
> You might consider making the constructor private and befriending
> WebPageProxy so that only WebPageProxy can construct it.  We usually try to
> avoid friend, but I think in this case it would be a reasonable choice.
> 

Agreed.

> > Source/WebKit/UIProcess/Notifications/WebNotificationManagerMessageHandler.h:39
> > +    void showNotification(const String& title, const String& body, const String& iconURL, const String& tag, const String& lang, WebCore::NotificationDirection, const String& originString, uint64_t notificationID) final;
> 
> nit: lang should be a full word.

Lots of places to change this one...  oooooff
> 
> > Source/WebKit/UIProcess/Notifications/WebNotificationManagerMessageHandler.h:40
> > +    void cancelNotification(uint64_t notificationID) final;
> 
> nit: this should be an ObjectIdentifier of some type.

https://bugs.webkit.org/show_bug.cgi?id=232023

[Brady Eidson]

Created attachment 441905 [details]
PFL v1

[Darin Adler]

Comment on attachment 441895 [details]
Patch v1

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

>>> Source/WebKit/UIProcess/Notifications/WebNotificationManagerMessageHandler.h:36
>>> +    WebNotificationManagerMessageHandler(WebPageProxy&);
>> 
>> You might consider making the constructor private and befriending WebPageProxy so that only WebPageProxy can construct it.  We usually try to avoid friend, but I think in this case it would be a reasonable choice.
> 
> Agreed.

Also a nice touch to use explicit for a constructor like this that happens to have exactly one argument but is not intended for type conversion.

[Brady Eidson]

Created attachment 441908 [details]
PFL v2 (now with more explicit content)

[Brady Eidson]

Created attachment 441911 [details]
PFL v3 - Now with more CMakeLists

[Brady Eidson]

Created attachment 441913 [details]
PFL v4 - Now with fewer tabs

[EWS]

Committed r284564 (243306@main): <https://commits.webkit.org/243306@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/84476731>