[webkit-reviews] review granted: [Bug 50017] Web Inspector: set DOM breakpoints by path : [Attachment 74748] Patch.

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has granted Pavel Podivilov
<podivilov at chromium.org>'s request for review:
Bug 50017: Web Inspector: set DOM breakpoints by path
https://bugs.webkit.org/show_bug.cgi?id=50017

Attachment 74748: Patch.
https://bugs.webkit.org/attachment.cgi?id=74748&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=74748&action=review

> WebCore/ChangeLog:5
> +	   Web Inspector: set DOM breakpoints by path

Please provide more detailed description on the purpose of this change.

> WebCore/inspector/InspectorDOMAgent.cpp:787
> +    m_idToBreakpoint.set(breakpointId, breakpoint);

I'd still rather use some compound id string instead of this two way mapping or
even better use dedicated methods for set/remove/hit breakpoint for different
breakpoint types. It may be more code but the design will be clearer than these
mappings from surrogate ids to breakpoints and back.

> WebCore/inspector/front-end/BreakpointManager.js:475
> +	   if (this._nodeId) {

Prefer early return.