[webkit-reviews] review denied: [Bug 42886] Web Inspector: implement DOM breakpoints : [Attachment 64585] Proposed 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 42886: Web Inspector: implement DOM breakpoints
https://bugs.webkit.org/show_bug.cgi?id=42886

Attachment 64585: Proposed patch.
https://bugs.webkit.org/attachment.cgi?id=64585&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
WebCore/bindings/js/ScriptDebugServer.cpp:210
 +	// FIXME(43332): implement this.
You can mention webkit bugzilla here so that we could distinguish this from
radar / chromium entries.

WebCore/inspector/InspectorDOMAgent.cpp:742
 +	Node* node = nodeForId(nodeId);
Move this check above? (This should not happen).


WebCore/inspector/front-end/ElementsTreeOutline.js:767
 +		contextMenu.appendItem(WebInspector.UIString("Stop on subtree
modifications"),
You should add these to the WebCore/English.lproj/localizedStrings.js

WebCore/inspector/front-end/BreakpointManager.js:93
 +	setDOMBreakpoint: function(nodeId, type)
Nit: I think these should be declared on DOMNode's prototype.

WebCore/inspector/InspectorDOMAgent.cpp:1075
 +	Vector<Node*> stack(1, innerFirstChild(node));
You have 4 places where you traverse the tree. Could it be extracted into a
separate method? A part of r- is for this.

WebCore/inspector/InspectorDOMAgent.cpp:204
 +  enum CachedDOMBreakpointType {
I am not sure why you need this separation? r- unless this is explained.

WebCore/inspector/InspectorDOMAgent.cpp:736
 +  void InspectorDOMAgent::setDOMBreakpoint(long nodeId, long type)
So what happens when you add a breakpoint to the node, then add it to its
parent and after that remove it from node?