Bug 178187

Created attachment 323605 [details] Patch (without changeling but otherwise review ready?)

Wow, autocorrect made "changelog" into "changeling" Nice.

Comment on attachment 323605 [details] Patch (without changeling but otherwise review ready?) View in context: https://bugs.webkit.org/attachment.cgi?id=323605&action=review > Source/WebCore/workers/service/ServiceWorkerContextData.h:70 > + if (!decoder.decode(scriptURL)) > + return std::nullopt; Shouldn't we have a MESSAGE_CHECK of some sort here? > Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:35 > +ServiceWorkerGlobalScope::ServiceWorkerGlobalScope(uint64_t serverConnectionIdentifier, const ServiceWorkerContextData& data, const URL& url, const String& identifier, const String& userAgent, ServiceWorkerThread& thread, bool shouldBypassMainWorldContentSecurityPolicy, Ref<SecurityOrigin>&& topOrigin, MonotonicTime timeOrigin, IDBClient::IDBConnectionProxy* connectionProxy, SocketProvider* socketProvider, PAL::SessionID sessionID) That is a lot of arguments. Will there be more? Seems ripe for a Parameters object or something.

(In reply to Tim Horton from comment #3) > Comment on attachment 323605 [details] > Patch (without changeling but otherwise review ready?) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323605&action=review > > > Source/WebCore/workers/service/ServiceWorkerContextData.h:70 > > + if (!decoder.decode(scriptURL)) > > + return std::nullopt; > > Shouldn't we have a MESSAGE_CHECK of some sort here? WebCore has no concept of MESSAGE_CHECK, so therefore non of the WebCore encoder/decoders do. > > > Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:35 > > +ServiceWorkerGlobalScope::ServiceWorkerGlobalScope(uint64_t serverConnectionIdentifier, const ServiceWorkerContextData& data, const URL& url, const String& identifier, const String& userAgent, ServiceWorkerThread& thread, bool shouldBypassMainWorldContentSecurityPolicy, Ref<SecurityOrigin>&& topOrigin, MonotonicTime timeOrigin, IDBClient::IDBConnectionProxy* connectionProxy, SocketProvider* socketProvider, PAL::SessionID sessionID) > > That is a lot of arguments. Will there be more? Seems ripe for a Parameters > object or something. This is exactly the same as WorkerGlobalScope, DedicatedWorkerGlobalScope, WorkerThread, etc etc etc. I don't know that a parameters object makes a lot of sense. For these families of objects there's only ever going to be one caller, and making changes to the parameters is easily handled with the ::create() method being variadic. Also a parameters object would allow for default arguments, and I think it's good for none of these arguments to be things you can ignore! Part of it is made nicer with a variadic ::create() method.

Created attachment 323625 [details] Patch (Changelog to come, still reviewable now)

Comment on attachment 323625 [details] Patch (Changelog to come, still reviewable now) View in context: https://bugs.webkit.org/attachment.cgi?id=323625&action=review > Source/WebCore/bindings/js/WorkerScriptController.cpp:112 > + Structure* contextPrototypeStructure = JSServiceWorkerGlobalScopePrototype::createStructure(*m_vm, nullptr, jsNull()); > + Strong<JSServiceWorkerGlobalScopePrototype> contextPrototype(*m_vm, JSServiceWorkerGlobalScopePrototype::create(*m_vm, nullptr, contextPrototypeStructure)); > + Structure* structure = JSServiceWorkerGlobalScope::createStructure(*m_vm, nullptr, contextPrototype.get()); > + auto* proxyStructure = JSProxy::createStructure(*m_vm, nullptr, jsNull(), PureForwardingProxyType); > + auto* proxy = JSProxy::create(*m_vm, proxyStructure); > + > + m_workerGlobalScopeWrapper.set(*m_vm, JSServiceWorkerGlobalScope::create(*m_vm, structure, static_cast<ServiceWorkerGlobalScope&>(*m_workerGlobalScope), proxy)); > + contextPrototypeStructure->setGlobalObject(*m_vm, m_workerGlobalScopeWrapper.get()); > + ASSERT(structure->globalObject() == m_workerGlobalScopeWrapper); > + ASSERT(m_workerGlobalScopeWrapper->structure()->globalObject() == m_workerGlobalScopeWrapper); > + contextPrototype->structure()->setGlobalObject(*m_vm, m_workerGlobalScopeWrapper.get()); > + contextPrototype->structure()->setPrototypeWithoutTransition(*m_vm, JSWorkerGlobalScope::prototype(*m_vm, *m_workerGlobalScopeWrapper.get())); > + > + proxy->setTarget(*m_vm, m_workerGlobalScopeWrapper.get()); > + proxy->structure()->setGlobalObject(*m_vm, m_workerGlobalScopeWrapper.get()); Since there's very little difference between this and the the code for dedicated workers above, I'd consider factoring this out into a function template that takes the prototype and global scope class names as parameters. > Source/WebCore/workers/service/ServiceWorkerGlobalScope.h:38 > + whitespace > Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:76 > +static uint64_t nextUniqueContextIdentifier() > +{ > + ASSERT(isMainThread()); > + static uint64_t currentUniqueContextIdentifier; > + return ++currentUniqueContextIdentifier; > +} Can ServiceWorkerThread use Identified<> instead?

(In reply to Andy Estes from comment #6) > Comment on attachment 323625 [details] > Patch (Changelog to come, still reviewable now) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323625&action=review > > > Source/WebCore/bindings/js/WorkerScriptController.cpp:112 > > + Structure* contextPrototypeStructure = JSServiceWorkerGlobalScopePrototype::createStructure(*m_vm, nullptr, jsNull()); > > + Strong<JSServiceWorkerGlobalScopePrototype> contextPrototype(*m_vm, JSServiceWorkerGlobalScopePrototype::create(*m_vm, nullptr, contextPrototypeStructure)); > > + Structure* structure = JSServiceWorkerGlobalScope::createStructure(*m_vm, nullptr, contextPrototype.get()); > > + auto* proxyStructure = JSProxy::createStructure(*m_vm, nullptr, jsNull(), PureForwardingProxyType); > > + auto* proxy = JSProxy::create(*m_vm, proxyStructure); > > + > > + m_workerGlobalScopeWrapper.set(*m_vm, JSServiceWorkerGlobalScope::create(*m_vm, structure, static_cast<ServiceWorkerGlobalScope&>(*m_workerGlobalScope), proxy)); > > + contextPrototypeStructure->setGlobalObject(*m_vm, m_workerGlobalScopeWrapper.get()); > > + ASSERT(structure->globalObject() == m_workerGlobalScopeWrapper); > > + ASSERT(m_workerGlobalScopeWrapper->structure()->globalObject() == m_workerGlobalScopeWrapper); > > + contextPrototype->structure()->setGlobalObject(*m_vm, m_workerGlobalScopeWrapper.get()); > > + contextPrototype->structure()->setPrototypeWithoutTransition(*m_vm, JSWorkerGlobalScope::prototype(*m_vm, *m_workerGlobalScopeWrapper.get())); > > + > > + proxy->setTarget(*m_vm, m_workerGlobalScopeWrapper.get()); > > + proxy->structure()->setGlobalObject(*m_vm, m_workerGlobalScopeWrapper.get()); > > Since there's very little difference between this and the the code for > dedicated workers above, I'd consider factoring this out into a function > template that takes the prototype and global scope class names as parameters. > Not as much of a slam dunk as it seems as there's SW specific bits coming very very soon. I'll take a quick look at it. > > Source/WebCore/workers/service/ServiceWorkerGlobalScope.h:38 > > + > > whitespace > > > Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:76 > > +static uint64_t nextUniqueContextIdentifier() > > +{ > > + ASSERT(isMainThread()); > > + static uint64_t currentUniqueContextIdentifier; > > + return ++currentUniqueContextIdentifier; > > +} > > Can ServiceWorkerThread use Identified<> instead? (Puts on the hat of the person who added Identified<> in the first place) Yes.

Created attachment 323637 [details] PFL

Created attachment 323639 [details] PFL

Comment on attachment 323639 [details] PFL Clearing flags on attachment: 323639 Committed r223277: <https://trac.webkit.org/changeset/223277>

Is it possible with this patch landed to do some HTTP/POST to a server so as to enable communication between a window and a service worker through this server?

Comment on attachment 323639 [details] PFL View in context: https://bugs.webkit.org/attachment.cgi?id=323639&action=review > Source/WebCore/workers/service/context/SWContextManager.cpp:40 > + static SWContextManager* sharedManager = new SWContextManager; Any reason we are not using NeverDestroyed here?