Web Inspector: introduce a pair of set/remove methods for each breakpoint type
Created attachment 76185 [details] Patch.
Comment on attachment 76185 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=76185&action=review > WebCore/inspector/InspectorController.cpp:1422 > + if (m_hasXHRBreakpointWithEmptyURL) { Why do you need to treat XHR breakpoints with empty URL differently? Can you just put the empty url into the set? > WebCore/inspector/front-end/BreakpointManager.js:52 > + var breakpointId = this._formatDOMBreakpointId(nodeId, type); _format* -> _create* ? > WebCore/inspector/front-end/BreakpointManager.js:130 > + if ("nodeId" in eventData) There should be explicit breakpoint type in the eventData. Imagine how it would look in the protocol. > WebCore/inspector/front-end/BreakpointManager.js:357 > + _persist: function() _persist -> _serializeToJSON
Created attachment 76190 [details] Comments addressed.
(In reply to comment #2) > (From update of attachment 76185 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76185&action=review > > > WebCore/inspector/InspectorController.cpp:1422 > > + if (m_hasXHRBreakpointWithEmptyURL) { > > Why do you need to treat XHR breakpoints with empty URL differently? Can you just put the empty url into the set? HashTable asserts that key != emptyValue. > > > WebCore/inspector/front-end/BreakpointManager.js:52 > > + var breakpointId = this._formatDOMBreakpointId(nodeId, type); > > _format* -> _create* ? > Done. > > WebCore/inspector/front-end/BreakpointManager.js:130 > > + if ("nodeId" in eventData) > > There should be explicit breakpoint type in the eventData. Imagine how it would look in the protocol. Done. > > > WebCore/inspector/front-end/BreakpointManager.js:357 > > + _persist: function() > > _persist -> _serializeToJSON Done.
Comment on attachment 76190 [details] Comments addressed. View in context: https://bugs.webkit.org/attachment.cgi?id=76190&action=review > WebCore/inspector/InspectorInstrumentation.cpp:48 > +static const char* const domNativeBreakpointType = "DOM"; I recall that InspectorInstrumentation was supposed to be a thin interface to the IC and now it's getting more logic. Who can I figure where to look for a particular feature implementation in IC or in II?
(In reply to comment #5) > (From update of attachment 76190 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76190&action=review > > > WebCore/inspector/InspectorInstrumentation.cpp:48 > > +static const char* const domNativeBreakpointType = "DOM"; > > I recall that InspectorInstrumentation was supposed to be a thin interface to the IC and now it's getting more logic. Who can I figure where to look for a particular feature implementation in IC or in II? I think we should move this stuff to a new BreakpointManager class, and keep II thin.
Committed r73726: <http://trac.webkit.org/changeset/73726>