[webkit-reviews] review denied: [Bug 42886] Web Inspector: implement DOM breakpoints : [Attachment 62416] Rebase.

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Pavel Podivilov
<podivilov at chromium.org>'s request for review:
Bug 42886: Web Inspector: implement DOM breakpoints
https://bugs.webkit.org/show_bug.cgi?id=42886

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
Few nits below. Otherwise, looks very good!

WebCore/bindings/js/ScriptDebugServer.h:82
 +	void breakProgram() { }
Please file a bug against WebKit / JSC and put a FIXME referring the bug here.


WebCore/inspector/InspectorBackend.idl:109
 +	    void setDOMBreakpoint(in long nodeId, in long type);
Please rebase, this file no longer exists.

WebCore/inspector/InspectorDOMAgent.cpp:1027
 +  #if ENABLE(JAVASCRIPT_DEBUGGER)
I think you need to optimize this:
1) do not enter this code when there are no dom breakpoints
2) when setting breakpoint on node, set it on all of its parents instead. that
way you spend time on setting breakpoint, not on dom events

WebCore/inspector/InspectorDOMAgent.cpp:761
 +  void InspectorDOMAgent::setDOMBreakpoint(long nodeId, long type)
type should be enum, synchronized with corresponding js enum.