[webkit-reviews] review granted: [Bug 126668] Web Inspector: capture probe samples on the backend : [Attachment 221071] v1

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 126668: Web Inspector: capture probe samples on the backend
https://bugs.webkit.org/show_bug.cgi?id=126668

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

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


r=me. Looks good to me, nice tests.

> Source/JavaScriptCore/inspector/protocol/Debugger.json:91
> +		   { "name": "probeId", "type": "integer", "description":
"Identifier of the 'probe' breakpoint action that created the sample." },

The type of probeId should be BreakpointActionIdentifier.

> Source/JavaScriptCore/inspector/protocol/Debugger.json:300
> +	      "description": "Fires when a new probe sample is collected."

Style: Incorrect indentation. Needs a space.

Also, I'd like to put the "description" next to the "name" from now on. It
flows better, and it is easier to read this file at a glance.

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:123
> +	       Interpreter::ErrorHandlingMode mode(debuggerCallFrame->exec());
> +	       // FIXME: we should supply the exception call stack to the
frontend.
> +	       debuggerCallFrame->exec()->clearException();
> +	       debuggerCallFrame->exec()->clearSupplementaryExceptionInfo();

Could this just do reportException(debuggerCallFrame->exec(), exception);?

> Source/WebCore/bindings/js/ScriptDebugServer.h:114
> +    int m_hitCount;

unsigned?

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:280
> +    for (ScriptBreakpointAction action : breakpointActions)

If you do "ScriptBreakpointAction& action" in this range iterator that would
avoid a copy.

NOTE: Benjamin also mentions that a typical indexed for loop compiles to be
slightly more efficient then the range based loop at this time. I think range
is fine for this likely to be small array, but avoiding copies would be nice.

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:336
> +    for (ScriptBreakpointAction action : breakpointActions)

Ditto

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:360
> +	   for (auto action : breakpointActions)

Ditto

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:685
> +	   .setTimestamp(WTF::currentTimeMS())

Nit: WTF:: not needed here. You can leave it in if you want.

We may want to look into switching everything to monotonicallyIncreasingTime.
My understanding is that it is faster.

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:711
> +    for (String identifier : breakpointIdentifiers)

Ditto

> LayoutTests/ChangeLog:13
> +	   *
inspector-protocol/debugger/setProbe-multiple-actions-expected.txt: Added.
> +	   * inspector-protocol/debugger/setProbe-multiple-actions.html: Added.


I would prefer if these were named

    inspector-protocol/<agent>/<method-or-event>-<extra-info>.html

So in this case, probably:

    inspector-protocol/debugger/didSampleProbe-multiple-probes.html

> LayoutTests/inspector-protocol/debugger/setProbe-multiple-actions.html:61
> +		   breakpointIdentifier = responseObject.result.breakpointId;

Nit: remove unused variable

> LayoutTests/inspector-protocol/debugger/setProbe-multiple-actions.html:62
> +		   actionIdentifiers =
responseObject.result.breakpointActionIdentifiers;

Nit: var

> LayoutTests/inspector-protocol/debugger/setProbe-multiple-actions.html:97
> +<p>Debugger.setBreakpoint options.actions</p>

Nit: A better description here.

    Debugger.setBreakpoint with multiple probes. Test Debugger.didSampleProbe
events for the probes.

> LayoutTests/inspector-protocol/resources/probe-helper.js:12
> +    var sample = {
> +	   probeId: data.probeId,
> +	   batchId: data.batchId,
> +	   sampleId: data.sampleId,
> +	   payload: data.payload
> +    }
> +    return sample;

Style: how about just returning { .. } without the "sample" local variable.