[webkit-reviews] review granted: [Bug 175115] Add SW IDLs and stub out. : [Attachment 317095] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 175115: Add SW IDLs and stub out.
https://bugs.webkit.org/show_bug.cgi?id=175115

Attachment 317095: Patch

https://bugs.webkit.org/attachment.cgi?id=317095&action=review




--- Comment #5 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 317095
  --> https://bugs.webkit.org/attachment.cgi?id=317095
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317095&action=review

r=me with comments

> Source/WebCore/bindings/js/JSServiceWorkerContainerCustom.cpp:37
> +JSValue JSServiceWorkerContainer::ready(ExecState&) const

Why are we already using custom bindings here? Sam is in the process of
removing custom bindings.

> Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:157
> +	   return 0;

nullptr

> Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:158
> +    const ClassInfo* classInfo = asObject(value)->classInfo(vm);

auto*

> Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:168
> +	   return jsDynamicDowncast<JSWorkerGlobalScope*>(vm,
jsCast<JSProxy*>(asObject(value))->target());

Shouldn't this call toJSWorkerGlobalScope() again in case we have a proxy to a
JSServiceWorkerGlobalScope?

> Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:170
> +    return 0;

nullptr

> Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:177
> +	   return 0;

nullptr

> Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:178
> +    const ClassInfo* classInfo = asObject(value)->classInfo(vm);

auto*

> Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:183
> +    return 0;

nullptr

> Source/WebCore/page/NavigatorServiceWorker.idl:34
> +    [SecureContext, SameObject] readonly attribute ServiceWorkerContainer
serviceWorker;

We do not support [SameObject] AFAIK.

> Source/WebCore/page/RuntimeEnabledFeatures.h:201
> +    bool serviceWorkerEnabled() const { return m_serviceWorkerEnabled; }

Sam asked me a couple of days ago to use a setting instead of
RuntimeEnabledFeatures. (using EnabledBySetting in the IDL)

> Source/WebCore/workers/ServiceWorkerContainer.idl:50
> +};

The spec also has: attribute EventHandler onmessageerror; ?

> Source/WebCore/workers/ServiceWorkerContainer.idl:56
> +    // WorkerType type = "classic";

Maybe we can add "ServiceWorkerUpdateViaCache updateViaCache = "imports";"
commented out as well?

> Source/WebCore/workers/ServiceWorkerGlobalScope.idl:38
> +    [SameObject] readonly attribute ServiceWorkerRegistration registration;

I do not think we support [SameObject]

> Source/WebCore/workers/ServiceWorkerGlobalScope.idl:40
> +    [NewObject] Promise<void> skipWaiting();

I do not think we support [SameObject]

> Source/WebCore/workers/ServiceWorkerRegistration.idl:43
> +

Spec also has readonly attribute ServiceWorkerUpdateViaCache updateViaCache; ?