[inspector] Change ScriptBreakpoint to include scriptId

src/inspector/v8-debugger.cc

Index: src/inspector/v8-debugger.cc
diff --git a/src/inspector/v8-debugger.cc b/src/inspector/v8-debugger.cc
index b3657e577cfc66cf0917f05e57f9243ac6d17b7c..1b4679569acb5988e65906526738d8764e6ca9a7 100644
--- a/src/inspector/v8-debugger.cc
+++ b/src/inspector/v8-debugger.cc
@@ -135,8 +135,7 @@ void V8Debugger::getCompiledScripts(
   }
 }
 
-String16 V8Debugger::setBreakpoint(const String16& sourceID,
-                                   const ScriptBreakpoint& scriptBreakpoint,
+String16 V8Debugger::setBreakpoint(const ScriptBreakpoint& breakpoint,
                                    int* actualLineNumber,
                                    int* actualColumnNumber) {
   v8::HandleScope scope(m_isolate);
@@ -146,20 +145,20 @@ String16 V8Debugger::setBreakpoint(const String16& sourceID,
   v8::Local<v8::Object> info = v8::Object::New(m_isolate);
   bool success = false;
   success = info->Set(context, toV8StringInternalized(m_isolate, "sourceID"),
-                      toV8String(m_isolate, sourceID))
+                      toV8String(m_isolate, breakpoint.script_id))
                 .FromMaybe(false);
   DCHECK(success);
   success = info->Set(context, toV8StringInternalized(m_isolate, "lineNumber"),
-                      v8::Integer::New(m_isolate, scriptBreakpoint.lineNumber))
+                      v8::Integer::New(m_isolate, breakpoint.line_number))
                 .FromMaybe(false);
   DCHECK(success);
   success =
       info->Set(context, toV8StringInternalized(m_isolate, "columnNumber"),
-                v8::Integer::New(m_isolate, scriptBreakpoint.columnNumber))
+                v8::Integer::New(m_isolate, breakpoint.column_number))
           .FromMaybe(false);
   DCHECK(success);
   success = info->Set(context, toV8StringInternalized(m_isolate, "condition"),
-                      toV8String(m_isolate, scriptBreakpoint.condition))
+                      toV8String(m_isolate, breakpoint.condition))
                 .FromMaybe(false);
   DCHECK(success);
 
@@ -171,16 +170,18 @@ String16 V8Debugger::setBreakpoint(const String16& sourceID,
       v8::DebugInterface::Call(debuggerContext(), setBreakpointFunction, info)
           .ToLocalChecked();
   if (!breakpointId->IsString()) return "";
-  *actualLineNumber =
-      info->Get(context, toV8StringInternalized(m_isolate, "lineNumber"))
-          .ToLocalChecked()
-          ->Int32Value(context)
-          .FromJust();
-  *actualColumnNumber =
-      info->Get(context, toV8StringInternalized(m_isolate, "columnNumber"))
-          .ToLocalChecked()
-          ->Int32Value(context)
-          .FromJust();
+  if (actualLineNumber)

[Yang, 2016/11/15 07:49:18]

why this change? are these going to be optional arguments?

[Clemens Hammacher, 2016/11/15 08:46:01]

It is not always needed, e.g. here:
https://codereview.chromium.org/2491133003/diff/40001/src/inspector/v8-debugg...
Making the arguments optional avoids the need to define local variables which
are never used afterwards in the caller. And it avoids the cost for looking up
the two properties here.

[kozy, 2016/11/15 23:29:47]

I think that we rather prefer to return actual location in continueToLocation
method then make it optional here. Could we left it not optional for now?

[Clemens Hammacher, 2016/11/16 12:06:24]

Done.

+    *actualLineNumber =
+        info->Get(context, toV8StringInternalized(m_isolate, "lineNumber"))
+            .ToLocalChecked()
+            ->Int32Value(context)
+            .FromJust();
+  if (actualColumnNumber)
+    *actualColumnNumber =
+        info->Get(context, toV8StringInternalized(m_isolate, "columnNumber"))
+            .ToLocalChecked()
+            ->Int32Value(context)
+            .FromJust();
   return toProtocolString(breakpointId.As<v8::String>());
 }