[webkit-reviews] review granted: [Bug 131673] Web Inspector: Move InspectorProfilerAgent to JavaScriptCore : [Attachment 229372] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Timothy Hatcher
<timothy at apple.com>'s request for review:
Bug 131673: Web Inspector: Move InspectorProfilerAgent to JavaScriptCore
https://bugs.webkit.org/show_bug.cgi?id=131673

Attachment 229372: Patch
https://bugs.webkit.org/attachment.cgi?id=229372&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=229372&action=review


Looks good to me! r=me

> Source/JavaScriptCore/inspector/agents/InspectorProfilerAgent.cpp:224
> +void
InspectorProfilerAgent::didCreateFrontendAndBackend(InspectorFrontendChannel*
frontendChannel, InspectorBackendDispatcher* backendDispatcher)
> +{
> +    m_frontendDispatcher =
std::make_unique<InspectorProfilerFrontendDispatcher>(frontendChannel);
> +    m_backendDispatcher =
InspectorProfilerBackendDispatcher::create(backendDispatcher, this);
> +}
> +
> +void
InspectorProfilerAgent::willDestroyFrontendAndBackend(InspectorDisconnectReason
reason)
> +{
> +    m_frontendDispatcher = nullptr;
> +    m_backendDispatcher.clear();
> +
> +    reset();
> +
> +    disable(reason == InspectorDisconnectReason::InspectedTargetDestroyed ?
SkipRecompile : Recompile);
> +}

Nit: I like to have these methods up near the constructor so they are easy to
find.

> Source/JavaScriptCore/inspector/agents/JSGlobalObjectProfilerAgent.cpp:37
>
+JSGlobalObjectProfilerAgent::JSGlobalObjectProfilerAgent(JSC::JSGlobalObject&
globalObject)

Nit: You can drop the "JSC::" here.

> Source/JavaScriptCore/inspector/agents/JSGlobalObjectProfilerAgent.h:32
> +#include <wtf/PassOwnPtr.h>

Nit: I do not think this include is needed.

> Source/JavaScriptCore/inspector/agents/JSGlobalObjectProfilerAgent.h:45
> +    virtual JSC::ExecState* profilingGlobalExecState() const override final;


Nit: Final is already on the class, we can remove it from here.

> Source/WebCore/inspector/InspectorController.cpp:76
> +#include <inspector/agents/InspectorProfilerAgent.h>

Nit: unnecessary include.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:962
> +    return nullptr;

Maybe this should return "String()" instead of nullptr.

> Source/WebCore/inspector/PageProfilerAgent.h:45
> +    virtual JSC::ExecState* profilingGlobalExecState() const override final;


Nit: Lets put final on the class instead of this method.

> Source/WebCore/inspector/PageProfilerAgent.h:47
> +    Page* m_inspectedPage;

Nit: Page&?

> Source/WebCore/inspector/WebProfilerAgent.h:36
> +typedef String ErrorString;

Nit: Unnecessary forward declaration.

> Source/WebCore/inspector/WorkerInspectorController.cpp:99
> +   
profilerAgent->setScriptDebugServer(&debuggerAgent->scriptDebugServer());

Oops. Though I'm not sure if we actually need this in a worker agent. Better to
have it then not.

> Source/WebCore/inspector/WorkerProfilerAgent.h:45
> +    virtual JSC::ExecState* profilingGlobalExecState() const override final;


Nit: final on class instead of method.

> Source/WebCore/page/PageConsole.cpp:170
>      if (!InspectorInstrumentation::profilerEnabled(&m_page))
>	   return;
>  

This seems unnecessary. Since the InspectorInstrumentation::startProfiling
won't do anything unless the profiler is enabled.

> Source/WebCore/page/PageConsole.cpp:179
> +    RefPtr<JSC::Profile> profile =
InspectorInstrumentation::stopProfiling(&m_page, exec, title);

Likewise.