[webkit-reviews] review granted: [Bug 179043] Make ServiceWorker a Remote Inspector debuggable target : [Attachment 325417] [PATCH] Proposed Fix - Part 1 (Backend)

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Joseph Pecoraro <joepeck at webkit.org>'s
request for review:
Bug 179043: Make ServiceWorker a Remote Inspector debuggable target
https://bugs.webkit.org/show_bug.cgi?id=179043

Attachment 325417: [PATCH] Proposed Fix - Part 1 (Backend)

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




--- Comment #9 from Brian Burg <bburg at apple.com> ---
Comment on attachment 325417
  --> https://bugs.webkit.org/attachment.cgi?id=325417
[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!