[inspector] Change ScriptBreakpoint to include scriptId

src/inspector/v8-debugger.cc

 // Copyright 2016 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/inspector/v8-debugger.h"
 
 #include "src/inspector/debugger-script.h"
 #include "src/inspector/protocol/Protocol.h"
 #include "src/inspector/script-breakpoint.h"
 #include "src/inspector/string-util.h"
@@ skipping 117 matching lines @@
     if (origin.IsEmbedderDebugScript()) continue;
     v8::Local<v8::String> v8ContextData;
     if (!script->ContextData().ToLocal(&v8ContextData)) continue;
     String16 contextData = toProtocolString(v8ContextData);
     if (contextData.find(contextPrefix) != 0) continue;
     result.push_back(
         wrapUnique(new V8DebuggerScript(m_isolate, script, false)));
   }
 }
 
-String16 V8Debugger::setBreakpoint(const String16& sourceID,
-                                   const ScriptBreakpoint& scriptBreakpoint,
+String16 V8Debugger::setBreakpoint(const ScriptBreakpoint& breakpoint,
                                    int* actualLineNumber,
                                    int* actualColumnNumber) {
   v8::HandleScope scope(m_isolate);
   v8::Local<v8::Context> context = debuggerContext();
   v8::Context::Scope contextScope(context);
 
   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);
 
   v8::Local<v8::Function> setBreakpointFunction = v8::Local<v8::Function>::Cast(
       m_debuggerScript.Get(m_isolate)
           ->Get(context, toV8StringInternalized(m_isolate, "setBreakpoint"))
           .ToLocalChecked());
   v8::Local<v8::Value> breakpointId =
       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>());
 }
 
 void V8Debugger::removeBreakpoint(const String16& breakpointId) {
   v8::HandleScope scope(m_isolate);
   v8::Local<v8::Context> context = debuggerContext();
   v8::Context::Scope contextScope(context);
 
   v8::Local<v8::Object> info = v8::Object::New(m_isolate);
   bool success = false;
@@ skipping 784 matching lines @@
 
   size_t stackSize =
       fullStack ? V8StackTraceImpl::maxCallStackSizeToCapture : 1;
   if (m_inspector->enabledRuntimeAgentForGroup(contextGroupId))
     stackSize = V8StackTraceImpl::maxCallStackSizeToCapture;
 
   return V8StackTraceImpl::capture(this, contextGroupId, stackSize);
 }
 
 }  // namespace v8_inspector