[webkit-reviews] review denied: [Bug 48858] Web Inspector: native breakpoints aren't hit on navigation : [Attachment 72829] Patch.

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Pavel Podivilov
<podivilov at chromium.org>'s request for review:
Bug 48858: Web Inspector: native breakpoints aren't hit on navigation
https://bugs.webkit.org/show_bug.cgi?id=48858

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72829&action=review

> WebCore/inspector/InspectorDOMAgent.cpp:761
> +void InspectorDOMAgent::setDOMBreakpoint(const String& breakpointId, long
nodeId, long type)

I like the way it used to be better. Dom agent can manage dom breakpoint
handles (i.e. return then and delete by them). And if there already is a
breakpoint of a type, we could return illegal handle / report error.

> WebCore/inspector/InspectorDebuggerAgent.cpp:160
> +    if (m_scheduledPauseEventType != type)

This gets complex. Could you provide an example where we hit this guard?
Setting r- until you provide explanation.

> WebCore/inspector/front-end/DOMAgent.js:382
> +	       WebInspector.breakpointManager.setCanRestoreDOMBreakpoints();

I like the previous name more.