Bug 217724

Created attachment 411360 [details] Patch

Created attachment 411361 [details] Patch

Created attachment 411362 [details] Patch

Seems like the custom paint worklet test is crashing.

Created attachment 411370 [details] Patch

Comment on attachment 411370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411370&action=review > Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.h:86 > + std::unique_ptr<WorkerInspectorController> m_inspectorController; Would it be worth it (assuming it's possible) to move each of the subclass members/methods onto the base `WorkerOrWorkletGlobalScope`? Are there any worklets that are somehow not inspectable? 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). > Source/WebCore/inspector/InspectorInstrumentation.cpp:1334 > + // FIXME: Add support for PaintWorklets. Why isn't this supported? > Source/WebCore/workers/WorkerInspectorProxy.cpp:49 > +#endif > + return downcast<WorkerGlobalScope>(context).inspectorController(); What about `PaintWorkletGlobalScope`?

(In reply to Devin Rousso from comment #6) > Comment on attachment 411370 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=411370&action=review > > > Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.h:86 > > + std::unique_ptr<WorkerInspectorController> m_inspectorController; > > Would it be worth it (assuming it's possible) to move each of the subclass > members/methods onto the base `WorkerOrWorkletGlobalScope`? Yes, this is planned for a follow-up. > Are there any > worklets that are somehow not inspectable? The main issue is that some worklets (like AudioWorklets) use threading and are thus very similar to Web Workers. Other worklets (like PaintWorklets) do not use threading and are thus very different. > 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. > > > Source/WebCore/inspector/InspectorInstrumentation.cpp:1334 > > + // FIXME: Add support for PaintWorklets. > > Why isn't this supported? PaintWorklets do not use threading and are thus very different from AudioWorklets and workers. I am working on AudioWorklets not PaintWorklets so I focused on getting AudioWorklets working (not PaintWorklets). I don't believe I made things worse for PaintWorklets. > > > Source/WebCore/workers/WorkerInspectorProxy.cpp:49 > > +#endif > > + return downcast<WorkerGlobalScope>(context).inspectorController(); > > What about `PaintWorkletGlobalScope`? PaintWorkets do not use threading. Thus PaintWorkletGlobalScope does not use WorkerInspectorProxy.

Comment on attachment 411370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411370&action=review >>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1334 >>> + // FIXME: Add support for PaintWorklets. >> >> Why isn't this supported? > > PaintWorklets do not use threading and are thus very different from AudioWorklets and workers. I am working on AudioWorklets not PaintWorklets so I focused on getting AudioWorklets working (not PaintWorklets). I don't believe I made things worse for PaintWorklets. Do PaintWorklet have a separate execution context? It's fine by me to do this as a followup/FIXME, but if you do that then please make a bug and include it's URL/title here. >> 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.

(In reply to Devin Rousso from comment #8) > Comment on attachment 411370 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=411370&action=review > > >>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1334 > >>> + // FIXME: Add support for PaintWorklets. > >> > >> Why isn't this supported? > > > > PaintWorklets do not use threading and are thus very different from AudioWorklets and workers. I am working on AudioWorklets not PaintWorklets so I focused on getting AudioWorklets working (not PaintWorklets). I don't believe I made things worse for PaintWorklets. > > Do PaintWorklet have a separate execution context? It's fine by me to do > this as a followup/FIXME, but if you do that then please make a bug and > include it's URL/title here. Yes, PaintWorklets have their own execution context. FYI, we only have a very partial implementation of PaintWorklets. It was added a while ago by an intern and never finished. The feature is not shipping. I personally don't have plans to work on paint worklets. > > >> 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.

Comment on attachment 411370 [details] Patch Ping review?

Comment on attachment 411370 [details] 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->source(), 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`?

(In reply to Devin Rousso from comment #11) > Comment on attachment 411370 [details] > 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>. Ok, then I'd like someone with Web Inspector knowledge to take over. I did what I could to save work.

Created attachment 411817 [details] Patch

(In reply to Chris Dumez from comment #12) > (In reply to Devin Rousso from comment #11) > > Comment on attachment 411370 [details] > > 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>. > > > Ok, then I'd like someone with Web Inspector knowledge to take over. I did > what I could to save work. Thank you!

<rdar://problem/70488763>

Created attachment 411986 [details] Patch

Created attachment 411990 [details] Patch

Created attachment 411993 [details] Patch

Created attachment 412001 [details] Patch

Created attachment 412032 [details] Patch

Created attachment 412140 [details] Patch

Created attachment 412180 [details] Patch

Comment on attachment 412180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412180&action=review > Source/WebCore/workers/WorkerScriptLoader.cpp:53 > + auto& workerOrWorkletGlobalScope = downcast<WorkerOrWorkletGlobalScope>(*scriptExecutionContext); Drive-by thought: Why not just call this "globalScope"? > Source/WebCore/worklets/WorkletGlobalScope.cpp:173 > +void WorkletGlobalScope::didReceiveResponse(unsigned long identifier, const ResourceResponse&) Another drive-by: Strange that this uses "unsigned long" and not "unsigned". Do you know why?

(In reply to Darin Adler from comment #23) > Comment on attachment 412180 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=412180&action=review > > > Source/WebCore/workers/WorkerScriptLoader.cpp:53 > > + auto& workerOrWorkletGlobalScope = downcast<WorkerOrWorkletGlobalScope>(*scriptExecutionContext); > > Drive-by thought: Why not just call this "globalScope"? Oh right, I updated to globalScope in the patch I landed yesterday but fail to update this patch accordingly. Will fix. > > > Source/WebCore/worklets/WorkletGlobalScope.cpp:173 > > +void WorkletGlobalScope::didReceiveResponse(unsigned long identifier, const ResourceResponse&) > > Another drive-by: Strange that this uses "unsigned long" and not "unsigned". > Do you know why? This all seems to come from the fact that CachedRawResource::identifier() returns an unsigned long. As a result, ThreadableLoader uses unsigned long for identifiers and the WorkerThreadableLoader just matches the type.

Comment on attachment 412180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412180&action=review >>> Source/WebCore/worklets/WorkletGlobalScope.cpp:173 >>> +void WorkletGlobalScope::didReceiveResponse(unsigned long identifier, const ResourceResponse&) >> >> Another drive-by: Strange that this uses "unsigned long" and not "unsigned". Do you know why? > > This all seems to come from the fact that CachedRawResource::identifier() returns an unsigned long. As a result, ThreadableLoader uses unsigned long for identifiers and the WorkerThreadableLoader just matches the type. OK. There’s no reason that I am aware of for WebKit to ever use the "unsigned long" type instead of "unsigned"—both are 32-bit unsigned integers—and there is a small downside to doing so (more overloading needed, some possible template bloat) so we should probably change CachedRawResource::identifier.

Created attachment 412204 [details] Patch