Summary: | Add support for ServiceWorkerGlobalScope push event handler | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||
Component: | Service Workers | Assignee: | youenn fablet <youennf> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, adieulot, annulen, augus.dupin, beidson, benjamin, bhomoki, bill, brian, brunow, calvaris, cdumez, collimarco91, deminetix, dvpdiner2, esprehn+autocc, ews-watchlist, fink.christoph, fweber, gyuyoung.kim, ik, jacek.wozniak, kangil.han, kondapallykalyan, kostas.eleftheriou, nham, nicolas, ryuan.choi, sergio, tomac, webkit-bug-importer, yg, yuriy.chaikovsky | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 231007 | ||||||||||
Bug Blocks: | 182566, 231070 | ||||||||||
Attachments: |
|
Description
youenn fablet
2021-09-30 00:46:26 PDT
*** Bug 182566 has been marked as a duplicate of this bug. *** Created attachment 439712 [details]
Patch
Comment on attachment 439712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439712&action=review > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:305 > + if (m_ongoingPushTasks.isEmpty()) It seems like we might be able to get away with just storing the number of ongoing tasks in a size_t instead of a HashMap of the CompletionHandlers. Created attachment 439833 [details]
Patch
Comment on attachment 439833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439833&action=review r=me with fixes. > Source/WebCore/testing/ServiceWorkerInternals.cpp:60 > +void ServiceWorkerInternals::schedulePushEvent(const std::optional<String>& message, RefPtr<DeferredPromise>&& promise) We never use std::optional<String> because String already has a null state (different than empty string). The generated bindings code will pass you a null string if the parameter was omitted in JS, not std::nullopt, so this is wrong. > Source/WebCore/testing/ServiceWorkerInternals.cpp:66 > + if (message) { if (!message.isNull()) > Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:220 > + RELEASE_LOG(ServiceWorker, "ServiceWorkerThread::queueTaskToFirePushEvent firing event for worker %llu", serviceWorkerGlobalScope->thread().identifier().toUInt64()); PRIu64 not %llu > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:311 > + callOnMainRunLoop([this, protectedThis = WTFMove(protectedThis), identifier, result]() mutable { We're in WebCore so we should use callOnMainThread() or postTaskToLoader(). ServiceWorkers are WebKit2 only but I remember we used to get tasks out of order by mixing callOnMainThread and callOnMainRunLoop (not sure if this is still the case nowadays). Created attachment 439961 [details]
Patch for landing
Committed r283438 (242425@main): <https://commits.webkit.org/242425@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 439961 [details]. "Status: RESOLVED FIXED" ... Does this mean that Web Push Notifications will be finally available in Safari? |