Make ServiceWorker a Remote Inspector debuggable target We should have the ability to directly inspect a Service Worker. A Service Worker's capabilities lies in between a JavaScript Context debuggable and a Web Page debuggable. • JavaScript Context has: Script capabilities • ServiceWorker has: Script capabilities, Network capabilities • Web page has: Script capabilities, Network capabilities, DOM capabilities Also JavaScript is expected to evaluating inside of a Service Worker on the specific Worker Context Thread. Evaluating on arbitrary threads (JSContext's normal remote inspection semantics) don't gel with worker's semantics.
<rdar://problem/34126008>
Created attachment 325416 [details] [IMAGE] ServiceWorker Inspector window
Created attachment 325417 [details] [PATCH] Proposed Fix - Part 1 (Backend)
Created attachment 325418 [details] [PATCH] Proposed Fix - Part 2 (Frontend)
Attachment 325417 [details] did not pass style-queue: ERROR: Source/WebCore/workers/service/context/ServiceWorkerDebuggable.cpp:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 325417 [details] [PATCH] Proposed Fix - Part 1 (Backend) View in context: https://bugs.webkit.org/attachment.cgi?id=325417&action=review > Source/JavaScriptCore/inspector/remote/RemoteControllableTarget.h:54 > + enum class Type { JavaScript, ServiceWorker, Web, Automation }; I didn't wrap any of this code in ENABLE(SERVICE_WORKER) but I suppose I could. It would cramp the style up in a few places but otherwise is straightforward. > Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:189 > + // FIXME: Support remote debugging of a ServiceWorker. I can file a bugzilla bug on GTK before landing.
Comment on attachment 325417 [details] [PATCH] Proposed Fix - Part 1 (Backend) View in context: https://bugs.webkit.org/attachment.cgi?id=325417&action=review > Source/WebCore/workers/service/context/ServiceWorkerDebuggable.cpp:45 > +void ServiceWorkerDebuggable::connect(Inspector::FrontendChannel* channel, bool, bool) FrontendChannel&... in the future probably. > Source/WebCore/workers/service/context/ServiceWorkerDebuggable.cpp:50 > +void ServiceWorkerDebuggable::disconnect(Inspector::FrontendChannel* channel) Ditto. > Source/WebCore/workers/service/context/ServiceWorkerDebuggable.h:46 > + Inspector::RemoteControllableTarget::Type type() const override { return Inspector::RemoteControllableTarget::Type::ServiceWorker; } override -> final and probably could be made private. > Source/WebCore/workers/service/context/ServiceWorkerDebuggable.h:49 > + String url() const override { return m_scriptURL; } const String& > Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.cpp:49 > + ASSERT(!m_channel); Should we also assert it is deleted on the main thread? > Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.h:47 > + explicit ServiceWorkerInspectorProxy(); Why explicit? > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:65 > + m_remoteDebuggable = std::make_unique<ServiceWorkerDebuggable>(*this, data); Do we need to create it now? Can we do that lazily? If so, maybe use makeUniqueRef. > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:72 > + m_inspectorProxy.serviceWorkerStarted(m_serviceWorkerThread); Same for m_inspectorProxy, can it be lazily created as an optional? Also, why does it need to keep a ref to m_serviceWorkerThread?
Comment on attachment 325417 [details] [PATCH] Proposed Fix - Part 1 (Backend) View in context: https://bugs.webkit.org/attachment.cgi?id=325417&action=review >> Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.cpp:49 >> + ASSERT(!m_channel); > > Should we also assert it is deleted on the main thread? Sure. This will be deleted with the ServiceWorkerThreadProxy, which right now I don't think it ever gets deleted. >> Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.h:47 >> + explicit ServiceWorkerInspectorProxy(); > > Why explicit? Oops, I'll remove. >> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:65 >> + m_remoteDebuggable = std::make_unique<ServiceWorkerDebuggable>(*this, data); > > Do we need to create it now? Can we do that lazily? > If so, maybe use makeUniqueRef. The debuggable should not be lazy, this is what makes the ServiceWorker appear to remote inspector debuggers. >> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:72 >> + m_inspectorProxy.serviceWorkerStarted(m_serviceWorkerThread); > > Same for m_inspectorProxy, can it be lazily created as an optional? > Also, why does it need to keep a ref to m_serviceWorkerThread? After talking with Youenn I'll make this take a ServiceWorkerThreadProxy& and only access the ServiceWorkerThread through the proxy. Technically the ServiceWorkerThreadProxy ServiceWorkerInspectorProxy can be the same object, I just split it out (like we do in DedicatedWorkers) to keep the inspector hooks / logic bundled together.
Comment on attachment 325417 [details] [PATCH] Proposed Fix - Part 1 (Backend) View in context: https://bugs.webkit.org/attachment.cgi?id=325417&action=review LGTM, but let's see another patch if you are going to adopt ServiceWorkerThreadProxy. >> Source/JavaScriptCore/inspector/remote/RemoteControllableTarget.h:54 >> + enum class Type { JavaScript, ServiceWorker, Web, Automation }; > > I didn't wrap any of this code in ENABLE(SERVICE_WORKER) but I suppose I could. It would cramp the style up in a few places but otherwise is straightforward. I wouldn't bother. We aren't paying a tax to add a new enum value. The only thing I would be careful of is relying on symbols that are wrapped in the ENABLE macro. >> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:189 >> + // FIXME: Support remote debugging of a ServiceWorker. > > I can file a bugzilla bug on GTK before landing. Please. > Source/WebCore/Sources.txt:2169 > +workers/service/context/ServiceWorkerDebuggable.cpp How do these files work? Are we supposed to add cpp or .h files to both Xcode and Sources.txt? > Source/WebCore/WebCore.xcodeproj/project.pbxproj:10706 > + A52B348C1FA3BD79008B6246 /* ServiceWorkerDebuggable.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ServiceWorkerDebuggable.h; sourceTree = "<group>"; }; Ditto. Or is this just for the navigator groups? >> Source/WebCore/workers/service/context/ServiceWorkerDebuggable.cpp:45 >> +void ServiceWorkerDebuggable::connect(Inspector::FrontendChannel* channel, bool, bool) > > FrontendChannel&... in the future probably. Yeah, this is a cleanup that we can do. > Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.h:33 > +// Used to send messages to the WorkerInspector on the WorkerThread. Nit: ServiceWorkerThread and ServiceWorkerInspector[Proxy]. > Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.h:54 > + void disconnectFromWorkerInspectorController(Inspector::FrontendChannel*); Could these drop the InspectorController suffix? Doesn't seem to add anything. > Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:73 > + : WorkerThread(data.scriptURL, data.workerID, ASCIILiteral("WorkerUserAgent"), data.script, loaderProxy, debuggerProxy, DummyServiceWorkerThreadProxy::shared(), WorkerThreadStartMode::Normal, ContentSecurityPolicyResponseHeaders { }, false, SecurityOrigin::create(data.scriptURL).get(), MonotonicTime::now(), nullptr, nullptr, JSC::RuntimeFlags::createAllEnabled(), SessionID::defaultSessionID()) Holy cow, how so many arguments!? > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:92 > + // FIXME: Handle terminated case. File a bug!
Comment on attachment 325418 [details] [PATCH] Proposed Fix - Part 2 (Frontend) View in context: https://bugs.webkit.org/attachment.cgi?id=325418&action=review Nice cleanup. r=me > Source/WebInspectorUI/ChangeLog:9 > + Customize a the Web Inspector window for a ServiceWorker target. Nit: -a > Source/WebInspectorUI/ChangeLog:11 > + we haven't yet added support for Networking capabitilites. Nit: capabilities
Created attachment 325791 [details] [PATCH] For Landing
<https://trac.webkit.org/r224368>