Track silent push counts
Created attachment 451797 [details] Patch
Created attachment 451835 [details] Patch
Created attachment 451857 [details] Patch
<rdar://problem/88886050>
Comment on attachment 451857 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451857&action=review > Source/WebCore/ChangeLog:19 > + e.g. making push subscriptions associated with this oirgin non-waking or removing the push s/oirgin/origin. It seems like we should add some callback API to inform the application each time the counter got increased. That would be useful for API tests as well. > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:424 > + Vector<SecurityOriginData> removedPushMetadata; Maybe we could simplify and consider that an updatedPushMetadata with a counter of 0 is a removed push metadata? > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:761 > + callOnMainThread([protectedThis = Ref { *this }, data = data.isolatedCopy()]() mutable { WTFMove(data).isolatedCopy(). > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:762 > + protectedThis->addPushMetadataToStore(WTFMove(data)); Maybe we could build the vector/map, and do just one call to add the metadata to store. > Source/WebCore/workers/service/server/RegistrationStore.cpp:127 > + ASSERT(!data.origin.protocol.isEmpty() && !data.origin.host.isEmpty()); Preexisting pattern, but I wonder whether this is needed here. Ditto for removePushMetadata > Source/WebCore/workers/service/server/RegistrationStore.cpp:142 > + m_updatedPushMetadata.set(origin, std::nullopt); Why using std::nullopt, and not a counter of 0? > Source/WebCore/workers/service/server/SWServer.cpp:193 > +void SWServer::addPushMetadataFromStore(PushMetadataData &&data) && data. > Source/WebCore/workers/service/server/SWServer.cpp:195 > + m_originToPushMetadata.set(data.origin, WTFMove(data)); ASSERT(!m_originToPushMetadata.contains(data.origin)); m_originToPushMetadata.add(...); But we could also just have just one big init by setting m_originToPushMetadata directly. > Source/WebCore/workers/service/server/SWServer.cpp:598 > +void SWServer::flushPendingRegistrationsForTesting(CompletionHandler<void()>&& handler) Could tests reuse storeRegistrationsOnDisk instead? > Source/WebCore/workers/service/server/SWServer.cpp:1264 > + m_originToPushMetadata.set(origin, WTFMove(data)); We can probably simplify with something like: m_originToPushMetadata.add(origin, { origin, 0 }).iterator->value++; m_registrationStore->updatePushMetadata(data); > Source/WebCore/workers/service/server/SWServer.cpp:1289 > + if (auto it = m_originToPushMetadata.find(origin); it != m_originToPushMetadata.end()) { s/it/iterator. Also, what is the difference between std::nullopt and 0? Maybe we can simply do: completionHandler(m_originToPushMetadata.get(origin).silentPushCount). > Source/WebCore/workers/service/server/SWServer.cpp:1338 > + weakThis->incrementSilentPushCount(origin); This is probably wrong in case of third party service worker. Do we have tests covering this area for third-party service workers? > Source/WebCore/workers/service/server/SWServer.h:309 > + HashMap<SecurityOriginData, PushMetadataData> m_originToPushMetadata; Should we simplify to HashMap<SecurityOriginData, unsigned>? > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:198 > + server().resetSilentPushCount(registration->key().topOrigin()); I am not sure we want this. There could be no controlling registration for the given URL even though there might be a related service worker. For instance, a web site might have a unique push service worker but this service worker is only used for push and not for controlling any page. Why not simply calling server().resetSileintPushCount for any top level navigation load?
(In reply to youenn fablet from comment #5) > Comment on attachment 451857 [details] > Patch > > > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:424 > > + Vector<SecurityOriginData> removedPushMetadata; > > Maybe we could simplify and consider that an updatedPushMetadata with a > counter of 0 is a removed push metadata? The reason for this is because I am thinking we may eventually need to store other stuff in this PushMetadata object. So the value of the updatedPushMetadata isn't an integer but rather the whole PushMetadata object. Perhaps a premature optimization. > WTFMove(data).isolatedCopy(). > > > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:762 > > + protectedThis->addPushMetadataToStore(WTFMove(data)); > > Maybe we could build the vector/map, and do just one call to add the > metadata to store. I originally did that, but then decided to follow the existing pattern. I can change it to just build up one vector and hop to the main thread just once. I think would have to be passed in a vector since I don't think HashMap implements a cross-thread copier. > > Source/WebCore/workers/service/server/RegistrationStore.cpp:142 > > + m_updatedPushMetadata.set(origin, std::nullopt); > > Why using std::nullopt, and not a counter of 0? Same reason as above, it's so that we can add more columns to PushMetadata in the future without having to rewrite this. > > Source/WebCore/workers/service/server/SWServer.cpp:1338 > > + weakThis->incrementSilentPushCount(origin); > > This is probably wrong in case of third party service worker. Do we have > tests covering this area for third-party service workers? I need to investigate this. > > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:198 > > + server().resetSilentPushCount(registration->key().topOrigin()); > > I am not sure we want this. > There could be no controlling registration for the given URL even though > there might be a related service worker. > For instance, a web site might have a unique push service worker but this > service worker is only used for push and not for controlling any page. > Why not simply calling server().resetSileintPushCount for any top level > navigation load? I'm going to remove the call to resetSilentPushCount for now since Brady wants a further discussion on when or if we should be doing that.
HashMap appears to have a cross thread copier: https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/CrossThreadCopier.h#L160
Went with another approach. Duping to the newer patch. *** This bug has been marked as a duplicate of bug 236863 ***