Bug 218108

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>