Bug 47253 - Web Inspector: highlight XHR breakpoint when hit
Summary: Web Inspector: highlight XHR breakpoint when hit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-06 03:42 PDT by Pavel Podivilov
Modified: 2010-10-07 05:43 PDT (History)
10 users (show)

See Also:

Attachments
Screenshot. (167.57 KB, image/png)
2010-10-06 03:42 PDT, Pavel Podivilov 		no flags Details
Patch. (18.88 KB, patch)
2010-10-06 03:51 PDT, Pavel Podivilov 		pfeldman: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Podivilov 2010-10-06 03:42:28 PDT 
Web Inspector: highlight XHR breakpoint when hit
Comment 1 Pavel Podivilov 2010-10-06 03:42:50 PDT 
Created attachment 69916 [details]
Screenshot.
Comment 2 Pavel Podivilov 2010-10-06 03:51:59 PDT 
Created attachment 69918 [details]
Patch.
Comment 3 Pavel Feldman 2010-10-06 13:49:07 PDT 
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
Comment 4 Pavel Feldman 2010-10-06 13:50:19 PDT 
Comment on attachment 69918 [details]
Patch.

r+ given that comments are addressed.
Comment 5 Joseph Pecoraro 2010-10-06 13:57:09 PDT 
> > 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.
Comment 6 Pavel Feldman 2010-10-06 14:04:51 PDT 
(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
Comment 7 Pavel Podivilov 2010-10-07 05:43:33 PDT 
Committed r69300: <http://trac.webkit.org/changeset/69300>