[Webkit-unassigned] [Bug 47253] Web Inspector: highlight XHR breakpoint when hit

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=47253





--- Comment #3 from Pavel Feldman <pfeldman at chromium.org>  2010-10-06 13:49:07 PST ---
(From update of attachment 69918)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.