Bug 50809 - Web Inspector: introduce a pair of set/remove methods for each breakpoint type
Summary: Web Inspector: introduce a pair of set/remove methods for each breakpoint type
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: Pavel Podivilov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-10 05:16 PST by Pavel Podivilov
Modified: 2010-12-10 07:33 PST (History)
10 users (show)

See Also:


Attachments
Patch. (36.06 KB, patch)
2010-12-10 05:17 PST, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Comments addressed. (42.95 KB, patch)
2010-12-10 06:59 PST, Pavel Podivilov
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Podivilov 2010-12-10 05:16:47 PST
Web Inspector: introduce a pair of set/remove methods for each breakpoint type
Comment 1 Pavel Podivilov 2010-12-10 05:17:31 PST
Created attachment 76185 [details]
Patch.
Comment 2 Yury Semikhatsky 2010-12-10 06:08:02 PST
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
Comment 3 Pavel Podivilov 2010-12-10 06:59:59 PST
Created attachment 76190 [details]
Comments addressed.
Comment 4 Pavel Podivilov 2010-12-10 07:01:11 PST
(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 5 Yury Semikhatsky 2010-12-10 07:04:55 PST
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?
Comment 6 Pavel Podivilov 2010-12-10 07:08:09 PST
(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.
Comment 7 Pavel Podivilov 2010-12-10 07:33:56 PST
Committed r73726: <http://trac.webkit.org/changeset/73726>