[inspector] restore provisional breakpoints smarter

src/inspector/v8-debugger-agent-impl.cc

Index: src/inspector/v8-debugger-agent-impl.cc
diff --git a/src/inspector/v8-debugger-agent-impl.cc b/src/inspector/v8-debugger-agent-impl.cc
index 664c2ed095a0545d2ee655c3b15178e21e056f3f..817984b371dd27a4964e3f402ad13a7cdd53ba92 100644
--- a/src/inspector/v8-debugger-agent-impl.cc
+++ b/src/inspector/v8-debugger-agent-impl.cc
@@ -50,6 +50,8 @@ static const char isRegex[] = "isRegex";
 static const char lineNumber[] = "lineNumber";
 static const char columnNumber[] = "columnNumber";
 static const char condition[] = "condition";
+static const char sourceLineHint[] = "sourceLineHint";
+
 static const char skipAllPauses[] = "skipAllPauses";
 
 }  // namespace DebuggerAgentState
@@ -58,6 +60,10 @@ static const char kBacktraceObjectGroup[] = "backtrace";
 static const char kDebuggerNotEnabled[] = "Debugger agent is not enabled";
 static const char kDebuggerNotPaused[] =
     "Can only perform operation while paused.";
+static const int kSourceLineHintToCheckOffsets[] = {

[pfeldman, 2017/02/07 19:13:05]

:) I'm sure alph@ would suggest to iterate to 16 and check everything for "i"
and "-i" or something 

[alph, 2017/02/07 22:22:30]

The problem is that you'd then double check the zero. ;-)

+    0, 1, -1, 2, -2, 3, -3, 4, -4, 5, -5, 6, -6, 7, -7, 8, -8, 9, -9, 10, -10,
+    11, -11, 12, -12, 13, -13, 14, -14, 15, -15, 16, -16};
+static const int kSourceLineHintMaxLength = 128;
 
 namespace {
 
@@ -241,13 +247,15 @@ Response V8DebuggerAgentImpl::setSkipAllPauses(bool skip) {
 static std::unique_ptr<protocol::DictionaryValue>
 buildObjectForBreakpointCookie(const String16& url, int lineNumber,
                                int columnNumber, const String16& condition,
-                               bool isRegex) {
+                               bool isRegex, const String16& sourceLineHint) {
   std::unique_ptr<protocol::DictionaryValue> breakpointObject =
       protocol::DictionaryValue::create();
   breakpointObject->setString(DebuggerAgentState::url, url);
   breakpointObject->setInteger(DebuggerAgentState::lineNumber, lineNumber);
   breakpointObject->setInteger(DebuggerAgentState::columnNumber, columnNumber);
   breakpointObject->setString(DebuggerAgentState::condition, condition);
+  breakpointObject->setString(DebuggerAgentState::sourceLineHint,
+                              sourceLineHint);
   breakpointObject->setBoolean(DebuggerAgentState::isRegex, isRegex);
   return breakpointObject;
 }
@@ -295,20 +303,24 @@ Response V8DebuggerAgentImpl::setBreakpointByUrl(
   if (breakpointsCookie->get(breakpointId))
     return Response::Error("Breakpoint at specified location already exists.");
 
-  breakpointsCookie->setObject(
-      breakpointId, buildObjectForBreakpointCookie(
-                        url, lineNumber, columnNumber, condition, isRegex));
-
   ScriptBreakpoint breakpoint(String16(), lineNumber, columnNumber, condition);
+  String16 sourceLineHint;
   for (const auto& script : m_scripts) {
     if (!matches(m_inspector, script.second->sourceURL(), url, isRegex))
       continue;
     breakpoint.script_id = script.first;
-    std::unique_ptr<protocol::Debugger::Location> location =
-        resolveBreakpoint(breakpointId, breakpoint, UserBreakpointSource);
+    sourceLineHint =
+        script.second->lineAt(lineNumber, kSourceLineHintMaxLength);
+    std::unique_ptr<protocol::Debugger::Location> location = resolveBreakpoint(
+        breakpointId, breakpoint, UserBreakpointSource, String16());
     if (location) (*locations)->addItem(std::move(location));
   }
 
+  breakpointsCookie->setObject(
+      breakpointId,
+      buildObjectForBreakpointCookie(url, lineNumber, columnNumber, condition,
+                                     isRegex, sourceLineHint));
+
   *outBreakpointId = breakpointId;
   return Response::OK();
 }
@@ -327,8 +339,8 @@ Response V8DebuggerAgentImpl::setBreakpoint(
       m_breakpointIdToDebuggerBreakpointIds.end()) {
     return Response::Error("Breakpoint at specified location already exists.");
   }
-  *actualLocation =
-      resolveBreakpoint(breakpointId, breakpoint, UserBreakpointSource);
+  *actualLocation = resolveBreakpoint(breakpointId, breakpoint,
+                                      UserBreakpointSource, String16());
   if (!*actualLocation) return Response::Error("Could not resolve breakpoint");
   *outBreakpointId = breakpointId;
   return Response::OK();
@@ -458,7 +470,8 @@ bool V8DebuggerAgentImpl::isFunctionBlackboxed(const String16& scriptId,
 std::unique_ptr<protocol::Debugger::Location>
 V8DebuggerAgentImpl::resolveBreakpoint(const String16& breakpointId,
                                        const ScriptBreakpoint& breakpoint,
-                                       BreakpointSource source) {
+                                       BreakpointSource source,
+                                       const String16& sourceLineHint) {
   v8::HandleScope handles(m_isolate);
   DCHECK(enabled());
   // FIXME: remove these checks once crbug.com/520702 is resolved.
@@ -466,12 +479,30 @@ V8DebuggerAgentImpl::resolveBreakpoint(const String16& breakpointId,
   CHECK(!breakpoint.script_id.isEmpty());
   ScriptsMap::iterator scriptIterator = m_scripts.find(breakpoint.script_id);
   if (scriptIterator == m_scripts.end()) return nullptr;
-  if (breakpoint.line_number < scriptIterator->second->startLine() ||
-      scriptIterator->second->endLine() < breakpoint.line_number)
+
+  V8DebuggerScript* script = scriptIterator->second.get();
+  ScriptBreakpoint translatedBreakpoint = breakpoint;
+  if (!sourceLineHint.isEmpty()) {

[pfeldman, 2017/02/07 19:13:05]

Extract into the method that adjusts ScriptBreakpoint line based on the content.
Also, what if column was not 0? Should you compare suffix only? Should you scan
script based on offsets, not lines?

[kozy, 2017/02/08 01:32:56]

I'm not sure. User puts breakpoint at line and if store from offset to next end
line then we will get more collisions but support some rare cases when only
first part of string was changed (could be useful maybe for minified source, but
we have much better solution with formatted script). If we store N symbols from
offset then changing next line will not allow us to restore breakpoint.

+    for (size_t i = 0; i < arraysize(kSourceLineHintToCheckOffsets); ++i) {

[pfeldman, 2017/02/07 19:13:05]

Yeah...

+      int lineNumber =
+          translatedBreakpoint.line_number + kSourceLineHintToCheckOffsets[i];
+      if (lineNumber < script->startLine() || lineNumber > script->endLine()) {

[alph, 2017/02/07 22:22:30]

no {}

[kozy, 2017/02/08 01:32:56]

yes {}, it's V8 code style.

+        continue;
+      }
+      if (script->lineAt(lineNumber, kSourceLineHintMaxLength) ==
+          sourceLineHint) {
+        translatedBreakpoint.line_number = lineNumber;
+        break;
+      }
+    }
+  }
+
+  if (breakpoint.line_number < script->startLine() ||
+      script->endLine() < breakpoint.line_number) {
     return nullptr;
+  }
 
   // Translate from protocol location to v8 location for the debugger.
-  ScriptBreakpoint translatedBreakpoint = breakpoint;
   m_debugger->wasmTranslation()->TranslateProtocolLocationToWasmScriptLocation(
       &translatedBreakpoint.script_id, &translatedBreakpoint.line_number,
       &translatedBreakpoint.column_number);
@@ -506,7 +537,7 @@ Response V8DebuggerAgentImpl::searchInContent(
     return Response::Error("No script for id: " + scriptId);
 
   std::vector<std::unique_ptr<protocol::Debugger::SearchMatch>> matches =
-      searchInTextByLinesImpl(m_session, it->second->source(m_isolate), query,
+      searchInTextByLinesImpl(m_session, it->second->source(), query,
                               optionalCaseSensitive.fromMaybe(false),
                               optionalIsRegex.fromMaybe(false));
   *results = protocol::Array<protocol::Debugger::SearchMatch>::create();
@@ -585,7 +616,7 @@ Response V8DebuggerAgentImpl::getScriptSource(const String16& scriptId,
   if (it == m_scripts.end())
     return Response::Error("No script for id: " + scriptId);
   v8::HandleScope handles(m_isolate);
-  *scriptSource = it->second->source(m_isolate);
+  *scriptSource = it->second->source();
   return Response::OK();
 }
 
@@ -979,7 +1010,7 @@ bool V8DebuggerAgentImpl::isPaused() const { return m_debugger->isPaused(); }
 void V8DebuggerAgentImpl::didParseSource(
     std::unique_ptr<V8DebuggerScript> script, bool success) {
   v8::HandleScope handles(m_isolate);
-  String16 scriptSource = script->source(m_isolate);
+  String16 scriptSource = script->source();
   if (!success) script->setSourceURL(findSourceURL(scriptSource, false));
   if (!success)
     script->setSourceMappingURL(findSourceMapURL(scriptSource, false));
@@ -1022,14 +1053,14 @@ void V8DebuggerAgentImpl::didParseSource(
     m_frontend.scriptParsed(
         scriptId, scriptURL, scriptRef->startLine(), scriptRef->startColumn(),
         scriptRef->endLine(), scriptRef->endColumn(), contextId,
-        scriptRef->hash(m_isolate), std::move(executionContextAuxDataParam),
+        scriptRef->hash(), std::move(executionContextAuxDataParam),
         isLiveEditParam, std::move(sourceMapURLParam), hasSourceURLParam,
         isModuleParam);
   else
     m_frontend.scriptFailedToParse(
         scriptId, scriptURL, scriptRef->startLine(), scriptRef->startColumn(),
         scriptRef->endLine(), scriptRef->endColumn(), contextId,
-        scriptRef->hash(m_isolate), std::move(executionContextAuxDataParam),
+        scriptRef->hash(), std::move(executionContextAuxDataParam),
         std::move(sourceMapURLParam), hasSourceURLParam, isModuleParam);
 
   if (scriptURL.isEmpty() || !success) return;
@@ -1045,6 +1076,7 @@ void V8DebuggerAgentImpl::didParseSource(
     bool isRegex;
     breakpointObject->getBoolean(DebuggerAgentState::isRegex, &isRegex);
     String16 url;
+    String16 sourceLineHint;
     breakpointObject->getString(DebuggerAgentState::url, &url);
     if (!matches(m_inspector, scriptURL, url, isRegex)) continue;
     ScriptBreakpoint breakpoint;
@@ -1055,8 +1087,10 @@ void V8DebuggerAgentImpl::didParseSource(
                                  &breakpoint.column_number);
     breakpointObject->getString(DebuggerAgentState::condition,
                                 &breakpoint.condition);
-    std::unique_ptr<protocol::Debugger::Location> location =
-        resolveBreakpoint(cookie.first, breakpoint, UserBreakpointSource);
+    breakpointObject->getString(DebuggerAgentState::sourceLineHint,
+                                &sourceLineHint);
+    std::unique_ptr<protocol::Debugger::Location> location = resolveBreakpoint(
+        cookie.first, breakpoint, UserBreakpointSource, sourceLineHint);
     if (location)
       m_frontend.breakpointResolved(cookie.first, std::move(location));
   }
@@ -1163,7 +1197,7 @@ void V8DebuggerAgentImpl::setBreakpointAt(const String16& scriptId,
                                           const String16& condition) {
   ScriptBreakpoint breakpoint(scriptId, lineNumber, columnNumber, condition);
   String16 breakpointId = generateBreakpointId(breakpoint, source);
-  resolveBreakpoint(breakpointId, breakpoint, source);
+  resolveBreakpoint(breakpointId, breakpoint, source, String16());
 }
 
 void V8DebuggerAgentImpl::removeBreakpointAt(const String16& scriptId,