Bug 163817 – Web Inspector: Introduce Page WorkerAgent and Worker InspectorController

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Summary:
Introduce Page WorkerAgent and Worker InspectorController

This introduces the major backend pieces for Worker inspection.

    - Worker domain
      - inform the frontend about Worker creation/termination
      - provide a channel to send/receive inspector protocol messages with a Worker backend

    - Page gets a WorkerAgent
      - inform the frontend about Worker creation/termination
      - auto connect to the Worker's WorkerInspectorController

    - Worker (WorkerGlobalScope) gets a WorkerInspectorController
      - runs on the WorkerThread
      - no agents yet, they will be added individually in follow-up patches
      - receives inspector messages from the Page's WorkerAgent (MainThread WorkerAgent -> WorkerThread WorkerInspectorController)
      - sends inspector messages back through the Page's WorkerAgent (WorkerThread WorkerInspectorController -> MainThread WorkerAgent)

The communication channels get tested as soon as we add the first agent, RuntimeAgent. But I'm including it in this patch because all the code is related.

[Radar WebKit Bug Importer]

<rdar://problem/28899063>

[Joseph Pecoraro]

Created attachment 292430 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Attachment 292430 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/WorkerInspectorController.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 292430 [details]
[PATCH] Proposed Fix

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

> LayoutTests/inspector/worker/worker-create-and-terminate.html:28
> +    ProtocolTest.debug();

Oops, I'll remove this.

[Build Bot]

Comment on attachment 292430 [details]
[PATCH] Proposed Fix

Attachment 292430 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2341414

New failing tests:
svg/wicd/test-rightsizing-b.xhtml

[Build Bot]

Created attachment 292442 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Joseph Pecoraro]

Created attachment 292846 [details]
[PATCH] Proposed Fix - Rebaselined

[WebKit Commit Bot]

Attachment 292846 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/WorkerInspectorController.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Comment on attachment 292846 [details]
[PATCH] Proposed Fix - Rebaselined

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

r+ please address comments

> LayoutTests/inspector/worker/worker-create-and-terminate.html:50
> +

Nit: extra newline

> LayoutTests/inspector/worker/worker-create-and-terminate.html:54
> +    function nextCreate(callback) {

In addition to using WrappedPromise.resolve, you could name this waitForWorkerCreatedEvent or listenForWorkerCreatedEvent to make the test easier to read.

> LayoutTests/inspector/worker/worker-create-and-terminate.html:65
> +

This code would be simpler with WebInspector.WrappedPromise, which is included in TestStub.html. Or just use .awaitEvent() if we don't need to get a callback multiple times in the same test case.

> Source/JavaScriptCore/inspector/protocol/Worker.json:16
> +            "description": "Send an Inspector Protocol message to be dispatched in the Worker.",

"Worker Controller" ?

> Source/JavaScriptCore/inspector/protocol/Worker.json:19
> +                { "name": "message", "type": "string" }

The field name or description should describe encoding or something. Joe says its a JSON string. I was unsure what the message was going to or from.

> Source/WebCore/ChangeLog:12
> +        may have its own set of Agents. This patch adds the necessary

You need to say that all communication with worker agents goes through WorkerAgent.sendMessageToWorker, which tunnels the command request to the Worker's InspectorController and agents via the main page's WorkerAgent. At the protocol level, agent command responses are sent as the event Worker.dispatchMessageFromWorker. On the frontend side, the message dispatcher code will pair up these no-reply commands+response events. So, calling worker agent methods in the frontend is no different from the existing callback/promise-based ways to get a command response from an agent.

> Source/WebCore/ChangeLog:17
> +        Pages now gets a WorkerAgent. This informs the frontend about

Grammaro

> Source/WebCore/ChangeLog:20
> +        each of the Workers. The Page side always interacts with the

I think it would be better to say: "PageInspectorController can send inspector protocol commands to WorkerInspectorController and receive responses and events from it. WorkerInspectorController dispatches messages to its own worker agents."

> Source/WebCore/ChangeLog:126
> +        Not really used until we implement DebuggerAgent.

, but required by the InspectorEnvironment interface.

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:36
> +InspectorWorkerAgent::InspectorWorkerAgent(PageAgentContext& context)

There's no need to prefix with Inspector if you instead put this code in the Inspector namespace. I think it's fine to keep the filename InspectorWorkerAgent even if it's in a namespace, as JSC does this for DFG and other namespaced classes.

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:84
> +    WorkerInspectorProxy* proxy = m_connectedProxies.get(workerId);

What if this was Inspector::WorkerProxy instead?

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:90
> +    proxy->sendMessageToWorkerInspector(message);

Then this reads workerProxy->sendMessageToWorkerInspector()

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:103
> +    connectToWorkerInspectorProxy(proxy);

connectToWorkerProxy

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:114
> +void InspectorWorkerAgent::connectToAllWorkerInspectorProxiesForPage()

ForPage sounds like it's naming the first argument. I think you could drop it.

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:119
> +        if (!is<Document>(*proxy->scriptExecutionContext()))

I don't think this requires a dereference prior to is<T>, that might cause a null pointer dereference. There are template functions for is<T*> that do the null check.

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:132
> +    for (auto& entry : m_connectedProxies)

In general I would take a copy of m_connectedProxies to avoid mutating while iterating. Thankfully in this code disconnectFromWorkerInspector doesn't indirectly trigger single disconnectFromWorkerInspectorProxy calls.

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:139
> +    proxy->connectToWorkerInspector(this);

proxy->connectToWorkerFrontend

> Source/WebCore/inspector/InspectorWorkerAgent.h:41
> +class InspectorWorkerAgent final : public InspectorAgentBase, public Inspector::WorkerBackendDispatcherHandler, public WorkerInspectorProxy::PageChannel {

Please move into namespace Inspector. Be the change you want to see. haha

> Source/WebCore/inspector/WorkerScriptDebugServer.cpp:83
> +void WorkerScriptDebugServer::interruptAndRunTask(std::unique_ptr<Task>)

Delete this, not used.

> Source/WebCore/inspector/WorkerScriptDebugServer.h:48
> +    class Task {

TODO: delete this.

> Source/WebCore/workers/WorkerInspectorProxy.cpp:59
> +void WorkerInspectorProxy::workerStarted(ScriptExecutionContext* scriptExecutionContext, WorkerThread* thread, const URL& url)

workerCreated

> Source/WebCore/workers/WorkerInspectorProxy.cpp:65
> +    m_url = url;

Note to Joe: it would be good to write a test to document if this is the pre- or post-redirect URL that we use to name the worker.

> Source/WebCore/workers/WorkerInspectorProxy.cpp:99
> +void WorkerInspectorProxy::disconnectFromWorkerInspector()

disconnectFromWorkerFrontend

> Source/WebCore/workers/WorkerInspectorProxy.h:73
> +    PageChannel* m_pageChannel { nullptr };

Reminder: file a bug for Brian to rename this. Which end is which? I cannot remember.

[Joseph Pecoraro]

Comment on attachment 292846 [details]
[PATCH] Proposed Fix - Rebaselined

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

>> LayoutTests/inspector/worker/worker-create-and-terminate.html:50
>> +
> 
> Nit: extra newline

This is intentional, it breaks up the sections of the test code.

>> Source/WebCore/inspector/InspectorWorkerAgent.cpp:84
>> +    WorkerInspectorProxy* proxy = m_connectedProxies.get(workerId);
> 
> What if this was Inspector::WorkerProxy instead?

The name "WorkerFoo" just comes from the fact that all WebCore files related to Worker have the Worker prefix. I think both names are confusing, but all of the Worker names are poor at this point =(

>> Source/WebCore/inspector/InspectorWorkerAgent.cpp:90
>> +    proxy->sendMessageToWorkerInspector(message);
> 
> Then this reads workerProxy->sendMessageToWorkerInspector()

I don't see the difference other then renaming of the local. Would you rather I rename these WorkerProxy?

>> Source/WebCore/inspector/InspectorWorkerAgent.cpp:132
>> +    for (auto& entry : m_connectedProxies)
> 
> In general I would take a copy of m_connectedProxies to avoid mutating while iterating. Thankfully in this code disconnectFromWorkerInspector doesn't indirectly trigger single disconnectFromWorkerInspectorProxy calls.

Good call!

>> Source/WebCore/inspector/InspectorWorkerAgent.cpp:139
>> +    proxy->connectToWorkerInspector(this);
> 
> proxy->connectToWorkerFrontend

This would actually be WorkerBackend, I renamed these to:

    connectToWorkerInspectorController
    disconnectFromWorkerInspectorController
    sendMessageToWorkerInspectorController
    sendMessageFromWorkerToFrontend

>> Source/WebCore/inspector/InspectorWorkerAgent.h:41
>> +class InspectorWorkerAgent final : public InspectorAgentBase, public Inspector::WorkerBackendDispatcherHandler, public WorkerInspectorProxy::PageChannel {
> 
> Please move into namespace Inspector. Be the change you want to see. haha

I'm going to defer this until follow-up. I think it adds a bunch of noise to these patches otherwise.

>> Source/WebCore/workers/WorkerInspectorProxy.cpp:65
>> +    m_url = url;
> 
> Note to Joe: it would be good to write a test to document if this is the pre- or post-redirect URL that we use to name the worker.

https://bugs.webkit.org/show_bug.cgi?id=164036

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/208008>