[webkit-reviews] review denied: [Bug 40781] Web Inspector: show actual breakpoint position in UI. : [Attachment 59770] 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 40781: Web Inspector: show actual breakpoint position in UI.
https://bugs.webkit.org/show_bug.cgi?id=40781

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
WebCore/bindings/js/ScriptDebugServer.cpp:140
 +  bool ScriptDebugServer::setBreakpoint(const String& sourceID,
ScriptBreakpoint breakpoint, int lineNumber, int* actualLineNumber)
Given that you still prefer returning bool, you don't need to encode -1, so you
should use unsigned values for line numbers.

WebCore/inspector/InspectorController.h:387
 +	HashMap<String, int> m_breakpointsMapping;
So why do you need this mapping after all? Why does sourceID:line not identify
breakpoint properly?

Btw, if you really need unique breakpoint ids, now that setBreakpoint has a
callback, you could assign them on backend (using progress counter) and send
them back to the backend.