[webkit-reviews] review denied: [Bug 42886] Web Inspector: implement DOM breakpoints : [Attachment 64945] Binary diff for localizedStrings.js

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys 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 64945: Binary diff for localizedStrings.js
https://bugs.webkit.org/attachment.cgi?id=64945&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
WebCore/inspector/InspectorDOMAgent.cpp:1066
 +	if (!node)
You don't seem to need this check.

WebCore/inspector/InspectorDOMAgent.cpp:1064
 +  void InspectorDOMAgent::updateSubtreeBreakpoints(Node* node, long mask,
bool set)
I'd rename mask to rootMask or something that would reflect the fact that it's
not shifted.

WebCore/inspector/InspectorDOMAgent.h:218
 +	    HashMap<Node*, long> m_breakpoints;
Why not have int as value type? We use only 32 bits in the code anyway.

WebCore/inspector/InspectorDOMAgent.cpp:1060
 +	return s_domAgentOnBreakpoint != 0;
Please reset s_domAgentOnBreakpoint to 0 before leaving this method.

WebCore/bindings/v8/ScriptDebugServer.cpp:243
 +	v8::Handle<v8::Context> context = v8::Context::GetCurrent();
Isn't it Debugger context?

Pleas address the comments, otherwise looks good.