[webkit-reviews] review granted: [Bug 127663] Web Inspector: Include profile with FunctionCall and EvaluateScript Timeline records : [Attachment 222291] 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 127663: Web Inspector: Include profile with FunctionCall and EvaluateScript
Timeline records
https://bugs.webkit.org/show_bug.cgi?id=127663

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

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


r=me, looks fine I'd be worried about a lot of extra / duplicated data being
sent to the frontend.

Also it would be really great to include a test for this.
LayoutTests/inspector-protocol/timeline/profiler-data-in-script-execution.html
or something like that.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:338
> +	   RefPtr<ScriptProfile> profile =
ScriptProfiler::stop(toJSDOMWindow(frame, debuggerWorld())->globalExec(),
ASCIILiteral("Timeline EvaluateScript"));
> +	   TimelineRecordFactory::appendProfile(entry.data.get(),
profile.release());

Seems like we could be duplicating a bunch of data sent to the frontend?

It seems nested ScriptProfiler::start/stops are fine, but each time it stops it
sends an entire profiler, much of which could be data already captured and sent
in another profile?

I know you measured, what was the performance impact of this?

> Source/WebCore/inspector/TimelineRecordFactory.cpp:257
> +    data->setValue("profile", profile->buildInspectorObjectForHead());

Nit: ASCIILiteral("profile")