Bug 126668 - Web Inspector: capture probe samples on the backend
Summary: Web Inspector: capture probe samples on the backend
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-01-08 15:36 PST by Brian Burg
Modified: 2014-01-15 15:18 PST (History)
6 users (show)

See Also:


Attachments
v1 (46.22 KB, patch)
2014-01-13 12:08 PST, Brian Burg
no flags Details | Formatted Diff | Diff
patch with fixes (46.20 KB, patch)
2014-01-15 14:42 PST, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian 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
<rdar://problem/15777026>
Comment 2 Brian Burg 2014-01-13 12:08:39 PST
Created attachment 221071 [details]
v1
Comment 3 Joseph Pecoraro 2014-01-14 16:28:27 PST
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.
Comment 4 Brian 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.