Bug 50809 – Web Inspector: introduce a pair of set/remove methods for each breakpoint type

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Podivilov]

Web Inspector: introduce a pair of set/remove methods for each breakpoint type

[Pavel Podivilov]

Created attachment 76185 [details]
Patch.

[Yury Semikhatsky]

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

[Pavel Podivilov]

Created attachment 76190 [details]
Comments addressed.

[Pavel Podivilov]

(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.

[Yury Semikhatsky]

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?

[Pavel Podivilov]

(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.

[Pavel Podivilov]

Committed r73726: <http://trac.webkit.org/changeset/73726>