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
<rdar://problem/15777026>
Created attachment 221071 [details] v1
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.
Created attachment 221308 [details] patch with fixes
Comment on attachment 221308 [details] patch with fixes Clearing flags on attachment: 221308 Committed r162096: <http://trac.webkit.org/changeset/162096>
All reviewed patches have been landed. Closing bug.