Bug 126668

Summary: Web Inspector: capture probe samples on the backend
Product: WebKit Reporter: BJ Burg <bburg>
Component: Web InspectorAssignee: BJ Burg <bburg>
Severity: Normal CC: commit-queue, graouts, joepeck, mkwst, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
patch with fixes none

Description BJ Burg 2014-01-08 15:36:00 PST
Things this patch will include:

- add new breakpoint action to protocol
- add probe sample data type to protocol
- add event for signaling probe sample captured
- backend handling of probe breakpoint action
Comment 1 Radar WebKit Bug Importer 2014-01-08 15:37:14 PST
Comment 2 BJ Burg 2014-01-13 12:08:39 PST
Created attachment 221071 [details]
Comment 3 Joseph Pecoraro 2014-01-14 16:28:27 PST
Comment on attachment 221071 [details]

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;


> 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)


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


> 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)


> 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


So in this case, probably:


> 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.
Comment 4 BJ Burg 2014-01-15 14:42:22 PST
Created attachment 221308 [details]
patch with fixes
Comment 5 WebKit Commit Bot 2014-01-15 15:18:28 PST
Comment on attachment 221308 [details]
patch with fixes

Clearing flags on attachment: 221308

Committed r162096: <http://trac.webkit.org/changeset/162096>
Comment 6 WebKit Commit Bot 2014-01-15 15:18:30 PST
All reviewed patches have been landed.  Closing bug.