[webkit-reviews] review denied: [Bug 217724] Hook AudioWorklets up to WebInspector : [Attachment 411370] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 217724: Hook AudioWorklets up to WebInspector
https://bugs.webkit.org/show_bug.cgi?id=217724

Attachment 411370: Patch

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




--- Comment #11 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 411370
  --> https://bugs.webkit.org/attachment.cgi?id=411370
Patch

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

r-, as this definitely needs tests.  We need to make sure that basic JavaScript
debugging works as expected.  I'd suggest copying most of tests in
`LayoutTests/inspector/worker` (some of them might need to be changed for
`setTimeout`/`setInterval`/etc. not existing) but using a `AudioWorklet` (and
`PaintWorklet` if you have the time/willpower) instead.  I'd also check to make
sure that the execution context for the workout appears in the execution
context picker to the right of the Console Prompt
<https://webkit.org/web-inspector/web-inspector-interface/#console-prompt>.

> > > > Also, do worklets have
`setTimeout`/`setInterval`/`requestAnimationFrame`?  I ask because the
`DOMDebugger` has different functionality for event/DOM breakpoints depending
on whether the target is a page or a worker (e.g. DOM breakpoints and animation
frame breakpoints are not supported for worker targets).
> > > No, worklets do not have those AFAIK.
> > In that case, we ideally should introduce a new target type `"worklet"`
(similar to the existing `"worker"`) so that we can properly annotate any
protocol commands/events and so that the frontend can differentiate between
workers and worklets (see r262302 for an example of how this has been done). I
don't think this is required in order to make inspection work, but we should do
it for correctness.  I'm OK with it being a followup bug.
> I'd appreciate if this could be a follow-up. I am doing basic hook up here
but I am not WebInspector expert and I will likely need help to do better.

Followup is fine :)

All I ask is that you create a bug for that change and add it's URL/title in
this patch as a FIXME comment, just so that there's a record of it.

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:42
> +class InspectorController;

So this isn't needed anymore because PaintWorklet doesn't use a separate
thread, meaning that all activity should eventually go through
`Page::inspectorController` right?

> Source/WebCore/worklets/WorkletGlobalScope.cpp:-166
> -    if (!m_document || isJSExecutionForbidden() || !message)

What happened to `isJSExecutionForbidden()`?

> Source/WebCore/worklets/WorkletGlobalScope.cpp:172
> +	  
m_document->addConsoleMessage(makeUnique<Inspector::ConsoleMessage>(message->so
urce(), message->type(), message->level(), message->message(), 0));

Odd that we're creating a new message with the same data.  I wonder if this is
really necessary ��

> Source/WebCore/worklets/WorkletGlobalScope.cpp:192
> +    if (m_document) {

Should this also check `isContextThread()`?

> Source/WebCore/worklets/WorkletGlobalScope.cpp:193
> +	   m_document->addMessage(source, level, messageText, sourceURL,
lineNumber, columnNumber, WTFMove(callStack), nullptr, requestIdentifier);

Should this also pass `state`?