Bug 178187 – SW "Hello world"

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

SW "Hello world"

<rdar://problem/33225187>

[Brady Eidson]

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

[Brady Eidson]

Wow, autocorrect made "changelog" into "changeling"

Nice.

[Tim Horton]

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.

[Brady Eidson]

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

[Brady Eidson]

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

[Andy Estes]

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?

[Brady Eidson]

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

[Brady Eidson]

Created attachment 323637 [details]
PFL

[Brady Eidson]

Created attachment 323639 [details]
PFL

[WebKit Commit Bot]

Comment on attachment 323639 [details]
PFL

Clearing flags on attachment: 323639

Committed r223277: <https://trac.webkit.org/changeset/223277>

[youenn fablet]

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?

[Chris Dumez]

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?