Bug 47253 – Web Inspector: highlight XHR breakpoint when hit

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

[Pavel Podivilov]

Web Inspector: highlight XHR breakpoint when hit

[Pavel Podivilov]

Created attachment 69916 [details]
Screenshot.

[Pavel Podivilov]

Created attachment 69918 [details]
Patch.

[Pavel Feldman]

Comment on attachment 69918 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=69918&action=review

> WebCore/ChangeLog:5
> +        Web Inspector: highlight XHR breakpoint when hit

Nit: We usually end titles with "." punctuation.

> WebCore/inspector/InspectorController.cpp:1716
> +unsigned int InspectorController::findEventListenerBreakpoint(const String& eventName)

I understand that it will linear wrt number of breakpoints set, but still linear search from within instrumentation does not sound right. It might be worth reversing the map.

> WebCore/inspector/front-end/BreakpointManager.js:204
> +        breakpoint._locked = true;

_locked -> _beingAdded or _editing

> WebCore/inspector/front-end/BreakpointManager.js:207
> +        function didSetNativeBreakpoint(backendBreakpointId)

We usually put callback function above the call.

> WebCore/inspector/front-end/BreakpointManager.js:232
> +            breakpoint.dispatchEventToListeners("hit");

Should we combine the two events into "hit-state-changed"?

> WebCore/inspector/front-end/BreakpointManager.js:337
> +        return this._manager.nativeBreakpointEnabled(this);

This sounds strange: breakpoint asks manager, passes itself -> then manager tests breakpoint against one of the properties. Given that both are in the same file and can access private memeber of each other, I'd suggest to shortcut it as "_breakpointId" in this.

> WebCore/inspector/front-end/BreakpointManager.js:342
> +        this._manager.setNativeBreakpointEnabled(this, enabled);

could be private

> WebCore/inspector/front-end/BreakpointManager.js:347
> +        this._manager.removeNativeBreakpoint(this);

ditto

[Pavel Feldman]

Comment on attachment 69918 [details]
Patch.

r+ given that comments are addressed.

[Joseph Pecoraro]

> > WebCore/ChangeLog:5
> > +        Web Inspector: highlight XHR breakpoint when hit
> 
> Nit: We usually end titles with "." punctuation.

Wow, I never noticed that. Out of 241 "Web Inspector:" changelog entires in
WebCore/ChangeLog, 112 of them end with a period. I didn't go further to
see if that was a recent trend or not.

[Pavel Feldman]

(In reply to comment #5)
> > > WebCore/ChangeLog:5
> > > +        Web Inspector: highlight XHR breakpoint when hit
> > 
> > Nit: We usually end titles with "." punctuation.
> 
> Wow, I never noticed that. Out of 241 "Web Inspector:" changelog entires in
> WebCore/ChangeLog, 112 of them end with a period. I didn't go further to
> see if that was a recent trend or not.

Most likely these 112 are mine :P

[Pavel Podivilov]

Committed r69300: <http://trac.webkit.org/changeset/69300>