Bug 236545 – Implement ServiceWorkerRegistration.getNotifications()

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Implement ServiceWorkerRegistration.getNotifications()

Obvious and required followup to https://bugs.webkit.org/show_bug.cgi?id=22722

[Brady Eidson]

Created attachment 451965 [details]
EWS v1

[Brady Eidson]

Created attachment 451975 [details]
EWS v2

[Brady Eidson]

Created attachment 452032 [details]
EWS v3

[Alex Christensen]

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]

(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]

(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]

Created attachment 452071 [details]
PFR

[Alex Christensen]

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]

(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]

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]

<rdar://problem/88997342>