Bug 126668 – Web Inspector: capture probe samples on the backend

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

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

[Radar WebKit Bug Importer]

<rdar://problem/15777026>

[Blaze Burg]

Created attachment 221071 [details]
v1

[Joseph Pecoraro]

Comment on attachment 221071 [details]
v1

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.

[Blaze Burg]

Created attachment 221308 [details]
patch with fixes

[WebKit Commit Bot]

Comment on attachment 221308 [details]
patch with fixes

Clearing flags on attachment: 221308

Committed r162096: <http://trac.webkit.org/changeset/162096>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.