Bug 232021 - Factor out some Notifications-specific messages from WebPageProxy messages
Summary: Factor out some Notifications-specific messages from WebPageProxy messages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-20 10:35 PDT by Brady Eidson
Modified: 2021-10-20 13:33 PDT (History)
8 users (show)

See Also:

Attachments
Patch v1 (37.11 KB, patch)
2021-10-20 10:38 PDT, Brady Eidson 		achristensen: review+
achristensen: commit-queue-
Details | Formatted Diff | Diff
PFL v1 (37.70 KB, patch)
2021-10-20 11:21 PDT, Brady Eidson 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
PFL v2 (now with more explicit content) (37.70 KB, patch)
2021-10-20 11:31 PDT, Brady Eidson 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
PFL v3 - Now with more CMakeLists (37.94 KB, patch)
2021-10-20 11:46 PDT, Brady Eidson 		no flags Details | Formatted Diff | Diff
PFL v4 - Now with fewer tabs (37.94 KB, patch)
2021-10-20 11:57 PDT, Brady Eidson 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2021-10-20 10:35:44 PDT 
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.
Comment 1 Brady Eidson 2021-10-20 10:38:31 PDT 
Created attachment 441895 [details]
Patch v1
Comment 2 Alex Christensen 2021-10-20 10:43:24 PDT 
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.
Comment 3 Brady Eidson 2021-10-20 11:05:15 PDT 
(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
Comment 4 Brady Eidson 2021-10-20 11:21:35 PDT 
Created attachment 441905 [details]
PFL v1
Comment 5 Darin Adler 2021-10-20 11:26:59 PDT 
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.
Comment 6 Brady Eidson 2021-10-20 11:31:43 PDT 
Created attachment 441908 [details]
PFL v2 (now with more explicit content)
Comment 7 Brady Eidson 2021-10-20 11:46:28 PDT 
Created attachment 441911 [details]
PFL v3 - Now with more CMakeLists
Comment 8 Brady Eidson 2021-10-20 11:57:52 PDT 
Created attachment 441913 [details]
PFL v4 - Now with fewer tabs
Comment 9 EWS 2021-10-20 13:32:44 PDT 
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].
Comment 10 Radar WebKit Bug Importer 2021-10-20 13:33:18 PDT 
<rdar://problem/84476731>