Use WorkerOrWorkletGlobalScope in WebInspector code instead of WorkerGlobalScope, in preparation for Worklets support.
Created attachment 412144 [details] Patch
Comment on attachment 412144 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412144&action=review > Source/WebCore/inspector/InspectorInstrumentation.h:102 > +class WorkerOrWorkletGlobalScope; Sure makes me wish we had a shorter name for this: Workerlikes or whatever. > Source/WebCore/inspector/InspectorInstrumentation.h:544 > + static InstrumentingAgents& instrumentingAgentsFor(Page&); > + static InstrumentingAgents& instrumentingAgentsFor(WorkerOrWorkletGlobalScope&); > + > + static InstrumentingAgents* instrumentingAgentsFor(const Frame&); > + static InstrumentingAgents* instrumentingAgentsFor(const Frame*); > + static InstrumentingAgents* instrumentingAgentsFor(ScriptExecutionContext*); > + static InstrumentingAgents* instrumentingAgentsFor(ScriptExecutionContext&); > + static InstrumentingAgents* instrumentingAgentsFor(Document&); > + static InstrumentingAgents* instrumentingAgentsFor(Document*); > + static InstrumentingAgents* instrumentingAgentsFor(RenderObject&); > + static InstrumentingAgents* instrumentingAgentsFor(WorkerOrWorkletGlobalScope*); Great to use overloading. Just wondering, why did you decide to keep the word "For"?
Comment on attachment 412144 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412144&action=review >> Source/WebCore/inspector/InspectorInstrumentation.h:544 >> + static InstrumentingAgents* instrumentingAgentsFor(WorkerOrWorkletGlobalScope*); > > Great to use overloading. Just wondering, why did you decide to keep the word "For"? 2 reasons: 1. To avoid naming conflicts with local variables. Otherwise, I would have had to use this->instrumentingAgents() in several places. I guess I could drop the "For" suffix and rename the local variables to "agents" if you prefer. 2. There is precedence in WebInspector code: m_injectedScriptManager.injectedScriptFor()
Comment on attachment 412144 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412144&action=review r=me > Source/WebCore/inspector/InspectorInstrumentation.h:534 > + static InstrumentingAgents& instrumentingAgentsFor(Page&); Can we drop the "For"? > Source/WebCore/inspector/InspectorInstrumentation.h:-550 > - if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForFrame(frame)) NIT: if you wanted, while you're at it you could make all these `InstrumentingAgents*` into `auto*` :) > Source/WebCore/workers/WorkerOrWorkletGlobalScope.cpp:43 > + , m_inspectorController(makeUnique<WorkerInspectorController>(*this)) I think this will have observable effects, as this will create a `WorkerConsoleAgent` inside worklets, meaning that `console` functions will now be able to do things. It may not have any actual effects though as the frontend connection isn't established yet (via `WorkerInspectorProxy::connectToWorkerInspectorController`). > Source/WebCore/workers/WorkerOrWorkletThread.h:51 > + virtual WorkerDebuggerProxy* workerDebuggerProxy() const = 0; Will we be able to make this into `WorkerDebuggerProxy&` in bug 217724, or do we have to leave it as a `WorkerDebuggerProxy*`?
oh lol Darin beat me to it 😅
Better with both of us reviewing. Not too many cooks.
(In reply to Devin Rousso from comment #4) > Comment on attachment 412144 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=412144&action=review > > r=me > > > Source/WebCore/inspector/InspectorInstrumentation.h:534 > > + static InstrumentingAgents& instrumentingAgentsFor(Page&); > > Can we drop the "For"? Will do. > > > Source/WebCore/inspector/InspectorInstrumentation.h:-550 > > - if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForFrame(frame)) > > NIT: if you wanted, while you're at it you could make all these > `InstrumentingAgents*` into `auto*` :) Will do. > > > Source/WebCore/workers/WorkerOrWorkletGlobalScope.cpp:43 > > + , m_inspectorController(makeUnique<WorkerInspectorController>(*this)) > > I think this will have observable effects, as this will create a > `WorkerConsoleAgent` inside worklets, meaning that `console` functions will > now be able to do things. It may not have any actual effects though as the > frontend connection isn't established yet (via > `WorkerInspectorProxy::connectToWorkerInspectorController`). > > > Source/WebCore/workers/WorkerOrWorkletThread.h:51 > > + virtual WorkerDebuggerProxy* workerDebuggerProxy() const = 0; > > Will we be able to make this into `WorkerDebuggerProxy&` in bug 217724, or > do we have to leave it as a `WorkerDebuggerProxy*`? Yes, I will turn it back into a reference in the follow-up patch. I was just trying to keep the patch size decently small.
Created attachment 412147 [details] Patch
Committed r268900: <https://trac.webkit.org/changeset/268900> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412147 [details].
<rdar://problem/70597384>