Created attachment 317095 [details] Patch

Comment on attachment 317095 [details] 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; ?

View in context: https://bugs.webkit.org/attachment.cgi?id=317095&action=review > Source/WebCore/bindings/js/JSServiceWorkerContainerCustom.cpp:37 > +JSValue JSServiceWorkerContainer::ready(ExecState&) const There are quite a few Promise returning attributes. Might be worth trying to upgrade the binding generator to remove those custom bindings. > Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:157 > + return 0; s/0/nullptr/ here and in other places below and above. > Source/WebCore/page/Navigator.idl:36 > +Navigator implements NavigatorServiceWorker; Usually, we use partial interface Navigator instead. Why not doing the same here? > Source/WebCore/workers/ServiceWorker.h:45 > + virtual ~ServiceWorker(); = default? > Source/WebCore/workers/ServiceWorker.h:55 > + String scriptURL() const; const String&? > Source/WebCore/workers/ServiceWorker.idl:15 > + * from this software without specific prior written permission. I seem to recall we use a 2 clause license now. This 3 clause license is also used in other new files of this patch. > Source/WebCore/workers/ServiceWorker.idl:40 > + [CallWith=ScriptState, MayThrowException] void postMessage(any message, optional sequence<object> transfer = []); Is "= []" required by our binding generator? > Source/WebCore/workers/ServiceWorkerContainer.h:41 > + virtual ~ServiceWorkerContainer(); No need for virtual here? > Source/WebCore/workers/ServiceWorkerContainer.h:62 > + virtual void derefEventTarget() { deref(); } Use final here? > Source/WebCore/workers/ServiceWorkerContainer.idl:49 > + attribute EventHandler onmessage; // event.source of message events is ServiceWorker object Can we remove those two comments? > Source/WebKit/UIProcess/WebPreferences.cpp:303 > + setServiceWorkersEnabled(false); If not doing that in that patch, we should quickly activate SW on RWT. We could probably quickly do some preliminary layout test regression testing on the IDLs for instance.

(In reply to Chris Dumez from comment #5) > Comment on attachment 317095 [details] > 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. Our generator doesn't make the right thing here. The issue will definitely come up while he works on removing custom bindings. > > 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? I cribbed this from toJSDedicatedWorkerGlobalScope right above, which does no such thing. I believe the downcast handles that. > > Source/WebCore/page/NavigatorServiceWorker.idl:34 > > + [SecureContext, SameObject] readonly attribute ServiceWorkerContainer serviceWorker; > > We do not support [SameObject] AFAIK. In general I copied over the spec IDLs verbatim when possible. They have SameObject, and SameObject doesn't break our bindings generator, so I think it's worth it to leave it. Especially if we're trying to get rid of all custom bindings then this will be necessary at some point. > > 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) I directly addressed why I did not use EnabledBySetting in the ChangeLog. > > Source/WebCore/workers/ServiceWorkerContainer.idl:50 > > +}; > > The spec also has: attribute EventHandler onmessageerror; ? Some I commented/removed because they confused our bindings generator. Dunno why I removed that one! Re-added. > > Source/WebCore/workers/ServiceWorkerContainer.idl:56 > > + // WorkerType type = "classic"; > > Maybe we can add "ServiceWorkerUpdateViaCache updateViaCache = "imports";" > commented out as well? Yup, re-adding commented out. > > Source/WebCore/workers/ServiceWorkerRegistration.idl:43 > > + > > Spec also has readonly attribute ServiceWorkerUpdateViaCache updateViaCache; Yup, re-adding commented out. (In reply to youenn fablet from comment #6) > View in context: > https://bugs.webkit.org/attachment.cgi?id=317095&action=review > > > Source/WebCore/bindings/js/JSServiceWorkerContainerCustom.cpp:37 > > +JSValue JSServiceWorkerContainer::ready(ExecState&) const > > There are quite a few Promise returning attributes. > Might be worth trying to upgrade the binding generator to remove those > custom bindings. Agreed - WELL outside the scope of my expertise and not worth holding up this initial work. I think you know who can work on the bindings gen ;) > > > Source/WebCore/page/Navigator.idl:36 > > +Navigator implements NavigatorServiceWorker; > > Usually, we use partial interface Navigator instead. Why not doing the same > here? The rest of the partials in Navigator.idl don't use that verbiage. If partial is more appropriate, they should probably all be moved at once. > > Source/WebCore/workers/ServiceWorker.h:45 > > + virtual ~ServiceWorker(); > > = default? For now, sure. > > Source/WebCore/workers/ServiceWorker.h:55 > > + String scriptURL() const; > > const String&? I didn't realize emptyString() was a const String& also. Will update. > > > Source/WebCore/workers/ServiceWorker.idl:15 > > + * from this software without specific prior written permission. > > I seem to recall we use a 2 clause license now. > This 3 clause license is also used in other new files of this patch. Whenever I create a new file this year, I always grab a "(c) 2017" license, and the one I grabbed was 3-clause. Will update, but we've definitely made this mistake throughout the year. > > > Source/WebCore/workers/ServiceWorker.idl:40 > > + [CallWith=ScriptState, MayThrowException] void postMessage(any message, optional sequence<object> transfer = []); > > Is "= []" required by our binding generator? Unclear - I try to stay true to the spec's IDL when our generator supports it, though. And this is supported. > > > Source/WebCore/workers/ServiceWorkerContainer.h:41 > > + virtual ~ServiceWorkerContainer(); > > No need for virtual here? I'm going to leave it to make it clear, just like on ServiceWorker and ServiceWorkerRegistration (it definitely is a class with a vtable, I prefer to keep that clear) > > Source/WebCore/workers/ServiceWorkerContainer.h:62 > > + virtual void derefEventTarget() { deref(); } > > Use final here? Sure! > > > Source/WebCore/workers/ServiceWorkerContainer.idl:49 > > + attribute EventHandler onmessage; // event.source of message events is ServiceWorker object > > Can we remove those two comments? Yah. Unclear why the spec IDLs have them. > > > Source/WebKit/UIProcess/WebPreferences.cpp:303 > > + setServiceWorkersEnabled(false); > > If not doing that in that patch, we should quickly activate SW on RWT. > We could probably quickly do some preliminary layout test regression testing > on the IDLs for instance. Oh, I absolutely agree. That's literally my next planned task after this patch. I would just like to land the code as one task and deal with tons of layout test fallout separately (and give you a change to land the WPT rebase first)

Created attachment 317135 [details] Patch for landing

Comment on attachment 317135 [details] Patch for landing Clearing flags on attachment: 317135 Committed r220220: <http://trac.webkit.org/changeset/220220>

> > > Source/WebCore/page/Navigator.idl:36 > > > +Navigator implements NavigatorServiceWorker; > > > > Usually, we use partial interface Navigator instead. Why not doing the same > > here? > > The rest of the partials in Navigator.idl don't use that verbiage. > > If partial is more appropriate, they should probably all be moved at once. I don't know the exact differences between partial and implement, Chris might know. Service worker spec is using partials so that seems better to stick with it. Partial interfaces are compile guarded, not sure how "Navigator implements" works there.

(In reply to youenn fablet from comment #10) > > > > Source/WebCore/page/Navigator.idl:36 > > > > +Navigator implements NavigatorServiceWorker; > > > > > > Usually, we use partial interface Navigator instead. Why not doing the same > > > here? > > > > The rest of the partials in Navigator.idl don't use that verbiage. > > > > If partial is more appropriate, they should probably all be moved at once. > > I don't know the exact differences between partial and implement, Chris > might know. > Service worker spec is using partials so that seems better to stick with it. > > Partial interfaces are compile guarded, not sure how "Navigator implements" > works there. implements statements are newer and better. It will also make like easier we we want to expose serverWorker on WorkerNavigator at some point.

(In reply to Chris Dumez from comment #11) > (In reply to youenn fablet from comment #10) > > > > > Source/WebCore/page/Navigator.idl:36 > > > > > +Navigator implements NavigatorServiceWorker; > > > > > > > > Usually, we use partial interface Navigator instead. Why not doing the same > > > > here? > > > > > > The rest of the partials in Navigator.idl don't use that verbiage. > > > > > > If partial is more appropriate, they should probably all be moved at once. > > > > I don't know the exact differences between partial and implement, Chris > > might know. > > Service worker spec is using partials so that seems better to stick with it. > > > > Partial interfaces are compile guarded, not sure how "Navigator implements" > > works there. > > implements statements are newer and better. It will also make like easier we > we want to expose serverWorker on WorkerNavigator at some point. That's not quite right. Implements statetements and partial interfaces have different use cases (for now). Partials are meant really as "editorial aide, allowing the definition of an interface to be separated over more than one section of the document, and sometimes multiple documents". Whereas, implements statements are meant to indicate that there is supplemental functionality being injected into an interface. That said, I think we should stick with going with what the spec uses, as that keeps it clearer when things change in the spec. And, partials and implements statements have slightly different behaviors with respect to extended attributes and inheritance.

Is this supposed to still be open?

(In reply to Sam Weinig from comment #13) > Is this supposed to still be open? Nope, unclear why commit bot didn't close.

<rdar://problem/33714276>

Note, using RuntimeEnabledFeatures from non-Document contexts is also not safe. Since RuntimeEnabledFeatures itself is not thread safe, it is not ok to call it from a worker.

(In reply to Sam Weinig from comment #12) > (In reply to Chris Dumez from comment #11) > > (In reply to youenn fablet from comment #10) > > > > > > Source/WebCore/page/Navigator.idl:36 > > > > > > +Navigator implements NavigatorServiceWorker; > > > > > > > > > > Usually, we use partial interface Navigator instead. Why not doing the same > > > > > here? > > > > > > > > The rest of the partials in Navigator.idl don't use that verbiage. > > > > > > > > If partial is more appropriate, they should probably all be moved at once. > > > > > > I don't know the exact differences between partial and implement, Chris > > > might know. > > > Service worker spec is using partials so that seems better to stick with it. > > > > > > Partial interfaces are compile guarded, not sure how "Navigator implements" > > > works there. > > > > implements statements are newer and better. It will also make like easier we > > we want to expose serverWorker on WorkerNavigator at some point. > > That's not quite right. Implements statetements and partial interfaces have > different use cases (for now). Partials are meant really as "editorial > aide, allowing the definition of an interface to be separated over more than > one section of the document, and sometimes multiple documents". Whereas, > implements statements are meant to indicate that there is supplemental > functionality being injected into an interface. > > That said, I think we should stick with going with what the spec uses, as > that keeps it clearer when things change in the spec. And, partials and > implements statements have slightly different behaviors with respect to > extended attributes and inheritance. FYI, the spec is using 2 partial interfaces (one of Navigator and one of WorkerNavigator): https://w3c.github.io/ServiceWorker/v1/#navigator-serviceworker

(In reply to Sam Weinig from comment #16) > Note, using RuntimeEnabledFeatures from non-Document contexts is also not > safe. Since RuntimeEnabledFeatures itself is not thread safe, it is not ok > to call it from a worker. Yah, I thought about that. In theory you're absolutely correct. In practice it's always created on the main thread before any background script execution threads exist, so accessing the shared object itself is safe. Then the only members are bools, which are "safe" to read from background threads while the main thread is changing them. And, in practice, they aren't generally change at runtime after the first WebProcess is created. This was already a preexisting problem (see IDB in workers) and we haven't seen any real world impact... But we *do* need to resolve it. I agree that having both Settings and RuntimeEnabledFeatures is less than ideal. But for features exposed to Workers, R.E.F. is the closest we have ATM.

> Source/WebKit/Shared/WebPreferencesDefinitions.h:368 > + macro(ServiceWorkersEnabled, serviceWorkersEnabled, Bool, bool, DEFAULT_EXPERIMENTAL_FEATURES_ENABLED, "ServiceWorkers", "Enable ServiceWorkers") \ Service worker being not yet fully functional, shouldn't serviceWorkersEnabled be set to false and not to DEFAULT_EXPERIMENTAL_FEATURES_ENABLED?

Tech Hack Guru for nice tutorials: https://mdipa.tumblr.com/