RESOLVED FIXED 236545
Implement ServiceWorkerRegistration.getNotifications() 
https://bugs.webkit.org/show_bug.cgi?id=236545
Summary Implement ServiceWorkerRegistration.getNotifications()
Brady Eidson
Reported 2022-02-12 17:13:32 PST
Implement ServiceWorkerRegistration.getNotifications() Obvious and required followup to https://bugs.webkit.org/show_bug.cgi?id=22722
Attachments
EWS v1 (15.42 KB, patch)
2022-02-14 17:08 PST, Brady Eidson 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
EWS v2 (15.46 KB, patch)
2022-02-14 18:30 PST, Brady Eidson 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
EWS v3 (15.65 KB, patch)
2022-02-15 09:15 PST, Brady Eidson 		no flags
DetailsFormatted DiffDiff
PFR (16.30 KB, patch)
2022-02-15 11:55 PST, Brady Eidson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Brady Eidson
Comment 1 2022-02-14 17:08:07 PST
Created attachment 451965 [details] EWS v1
Brady Eidson
Comment 2 2022-02-14 18:30:53 PST
Created attachment 451975 [details] EWS v2
Brady Eidson
Comment 3 2022-02-15 09:15:33 PST
Created attachment 452032 [details] EWS v3
Alex Christensen
Comment 4 2022-02-15 09:32:25 PST
Comment on attachment 452032 [details] EWS v3 View in context: https://bugs.webkit.org/attachment.cgi?id=452032&action=review > Source/WebCore/workers/service/ServiceWorkerRegistration.h:132 > + ListHashSet<Notification*> m_notificationList; Can this use WeakPtr?
Brady Eidson
Comment 5 2022-02-15 09:51:46 PST
(In reply to Alex Christensen from comment #4) > Comment on attachment 452032 [details] > EWS v3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=452032&action=review > > > Source/WebCore/workers/service/ServiceWorkerRegistration.h:132 > > + ListHashSet<Notification*> m_notificationList; > > Can this use WeakPtr? Yah that's probably better.
Brady Eidson
Comment 6 2022-02-15 10:34:39 PST
(In reply to Brady Eidson from comment #5) > (In reply to Alex Christensen from comment #4) > > Comment on attachment 452032 [details] > > EWS v3 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=452032&action=review > > > > > Source/WebCore/workers/service/ServiceWorkerRegistration.h:132 > > > + ListHashSet<Notification*> m_notificationList; > > > > Can this use WeakPtr? > > Yah that's probably better. But also can't work - Can't use WeakPtr<> as a HashKey for obvious reasons.
Brady Eidson
Comment 7 2022-02-15 11:55:05 PST
Created attachment 452071 [details] PFR
Alex Christensen
Comment 8 2022-02-15 12:04:53 PST
Comment on attachment 452071 [details] PFR View in context: https://bugs.webkit.org/attachment.cgi?id=452071&action=review > Source/WebCore/Modules/notifications/Notification.cpp:88 > +Notification::Notification(const Notification& other) After calling the copy constructor, do we not need to call addNotificationToList?
Brady Eidson
Comment 9 2022-02-15 12:08:05 PST
(In reply to Alex Christensen from comment #8) > Comment on attachment 452071 [details] > PFR > > View in context: > https://bugs.webkit.org/attachment.cgi?id=452071&action=review > > > Source/WebCore/Modules/notifications/Notification.cpp:88 > > +Notification::Notification(const Notification& other) > > After calling the copy constructor, do we not need to call > addNotificationToList? We definitely *DON'T* want the copies to be in the list. They don't represent a live notification object the same way the ones in the list do.
EWS
Comment 10 2022-02-15 17:26:37 PST
Committed r289866 (247306@main): <https://commits.webkit.org/247306@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 452071 [details].
Radar WebKit Bug Importer
Comment 11 2022-02-15 17:27:18 PST
<rdar://problem/88997342>
Note You need to log in before you can comment on or make changes to this bug.