[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 0e455cb9b90f4575daa605a92959cc2353755af6..a7d7688da25475695c1d9e6c4489278ceeb9e324 100644
--- a/src/inspector/v8-debugger-agent-impl.cc
+++ b/src/inspector/v8-debugger-agent-impl.cc
@@ -43,6 +43,7 @@ static const char pauseOnExceptionsState[] = "pauseOnExceptionsState";
 static const char asyncCallStackDepth[] = "asyncCallStackDepth";
 static const char blackboxPattern[] = "blackboxPattern";
 static const char debuggerEnabled[] = "debuggerEnabled";
+static const char skipAllPauses[] = "skipAllPauses";
 
 // Breakpoint properties.
 static const char url[] = "url";
@@ -50,7 +51,7 @@ static const char isRegex[] = "isRegex";
 static const char lineNumber[] = "lineNumber";
 static const char columnNumber[] = "columnNumber";
 static const char condition[] = "condition";
-static const char skipAllPauses[] = "skipAllPauses";
+static const char hint[] = "hint";
 
 }  // namespace DebuggerAgentState
 
@@ -59,6 +60,9 @@ static const char kDebuggerNotEnabled[] = "Debugger agent is not enabled";
 static const char kDebuggerNotPaused[] =
     "Can only perform operation while paused.";
 
+static const size_t kBreakpointHintMaxLength = 128;
+static const size_t kBreakpointHintMaxSearchOffset = 80 * 10;
+
 namespace {
 
 void TranslateWasmStackTraceLocations(Array<CallFrame>* stackTrace,
@@ -119,6 +123,59 @@ std::unique_ptr<protocol::Debugger::Location> buildProtocolLocation(
       .build();
 }
 
+String16 breakpointHint(const V8DebuggerScript& script,
+                        const ScriptBreakpoint& breakpoint) {
+  int offset = script.offset(breakpoint.line_number, breakpoint.column_number);
+  if (offset == V8DebuggerScript::kNoOffset) return String16();
+  const String16& source = script.source();
+  String16 hint = source.substring(offset, offset + kBreakpointHintMaxLength)

[alph, 2017/02/27 22:01:29]

the second arg of substring is length

[kozy, 2017/02/27 23:16:35]

ooops, fixed.

+                      .stripWhiteSpace();
+  for (size_t i = 0; i < hint.length(); ++i) {
+    if (hint[i] == '\r' || hint[i] == '\n' || hint[i] == ';') {
+      return hint.substring(0, i);
+    }
+  }
+  return hint;
+}
+
+void adjustBreakpointLocation(const V8DebuggerScript& script,
+                              const String16& hint,
+                              ScriptBreakpoint* breakpoint) {
+  if (hint.isEmpty()) return;
+  size_t offset = static_cast<size_t>(
+      script.offset(breakpoint->line_number, breakpoint->column_number));
+  if (offset == static_cast<size_t>(V8DebuggerScript::kNoOffset)) return;
+  size_t searchAreaOffset = 0;
+  if (offset > kBreakpointHintMaxSearchOffset)
+    searchAreaOffset = offset - kBreakpointHintMaxSearchOffset;
+  String16 searchArea =
+      script.source()
+          .substring(searchAreaOffset, offset + kBreakpointHintMaxSearchOffset)

[alph, 2017/02/27 22:01:29]

The second arg of substring is length.
Also you may want to use 2 * kBreakpointHintMaxSearchOffset

[kozy, 2017/02/27 23:16:35]

Done.

+          .stripWhiteSpace();

[alph, 2017/02/27 22:01:29]

As long as it strips whitespaces from the beginning, you can't map the offsets
to original script anymore.

[kozy, 2017/02/27 23:16:35]

Done.

+  size_t hintOffset = searchArea.find(hint);
+  if (hintOffset == String16::kNotFound) return;
+  size_t bestMatch = hintOffset + searchAreaOffset;
+  size_t bestDistance =
+      bestMatch > offset ? bestMatch - offset : offset - bestMatch;

[alph, 2017/02/27 22:01:29]

I wonder if you can use intptr_t instead of size_t to simplify arithmetic with
negative numbers.

[kozy, 2017/02/27 23:16:35]

Done.

+  while (hintOffset != String16::kNotFound) {

[alph, 2017/02/27 22:01:29]

the condition always holds

[kozy, 2017/02/27 23:16:35]

Done.

+    hintOffset = searchArea.find(hint, hintOffset + 1);
+    if (hintOffset == String16::kNotFound) break;
+    size_t hintDistance = hintOffset + searchAreaOffset > offset

[alph, 2017/02/27 22:01:29]

You don't need to do it in a loop. Just two checks with find and reverseFind
should be enough. right?

[kozy, 2017/02/27 23:16:35]

Done.

+                              ? hintOffset + searchAreaOffset - offset
+                              : offset - (hintOffset + searchAreaOffset);
+    if (hintDistance < bestDistance) {
+      bestMatch = hintOffset + searchAreaOffset;
+      bestDistance = hintDistance;
+    } else {
+      break;
+    }
+  }
+  if (bestMatch == String16::kNotFound) return;

[alph, 2017/02/27 22:01:29]

always false

[kozy, 2017/02/27 23:16:35]

Done.

+  v8::debug::Location hintPosition = script.location(bestMatch);
+  if (hintPosition.IsEmpty()) return;
+  breakpoint->line_number = hintPosition.GetLineNumber();
+  breakpoint->column_number = hintPosition.GetColumnNumber();
+}
 }  // namespace
 
 V8DebuggerAgentImpl::V8DebuggerAgentImpl(
@@ -239,7 +296,7 @@ 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& hint) {
   std::unique_ptr<protocol::DictionaryValue> breakpointObject =
       protocol::DictionaryValue::create();
   breakpointObject->setString(DebuggerAgentState::url, url);
@@ -247,6 +304,9 @@ buildObjectForBreakpointCookie(const String16& url, int lineNumber,
   breakpointObject->setInteger(DebuggerAgentState::columnNumber, columnNumber);
   breakpointObject->setString(DebuggerAgentState::condition, condition);
   breakpointObject->setBoolean(DebuggerAgentState::isRegex, isRegex);
+  if (!hint.isEmpty()) {
+    breakpointObject->setString(DebuggerAgentState::hint, hint);
+  }
   return breakpointObject;
 }
 
@@ -293,20 +353,23 @@ 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));
-
+  String16 hint;
   ScriptBreakpoint breakpoint(String16(), lineNumber, columnNumber, condition);
   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);
+        resolveBreakpoint(breakpointId, breakpoint, UserBreakpointSource, hint);
+    if (!isRegex) hint = breakpointHint(*script.second, breakpoint);
     if (location) (*locations)->addItem(std::move(location));
   }
 
+  breakpointsCookie->setObject(
+      breakpointId,
+      buildObjectForBreakpointCookie(url, lineNumber, columnNumber, condition,
+                                     isRegex, hint));
+
   *outBreakpointId = breakpointId;
   return Response::OK();
 }
@@ -325,8 +388,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, /* hint */ String16());
   if (!*actualLocation) return Response::Error("Could not resolve breakpoint");
   *outBreakpointId = breakpointId;
   return Response::OK();
@@ -457,7 +520,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& hint) {
   v8::HandleScope handles(m_isolate);
   DCHECK(enabled());
   // FIXME: remove these checks once crbug.com/520702 is resolved.
@@ -471,6 +535,8 @@ V8DebuggerAgentImpl::resolveBreakpoint(const String16& breakpointId,
 
   // Translate from protocol location to v8 location for the debugger.
   ScriptBreakpoint translatedBreakpoint = breakpoint;
+  adjustBreakpointLocation(*scriptIterator->second, hint,
+                           &translatedBreakpoint);
   m_debugger->wasmTranslation()->TranslateProtocolLocationToWasmScriptLocation(
       &translatedBreakpoint.script_id, &translatedBreakpoint.line_number,
       &translatedBreakpoint.column_number);
@@ -505,7 +571,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();
@@ -538,7 +604,7 @@ Response V8DebuggerAgentImpl::setScriptSource(
       &m_pausedCallFrames, stackChanged, &compileError);
   if (!response.isSuccess() || compileError) return response;
 
-  it->second->setSource(newSource);
+  it->second->setSource(newContent);
   std::unique_ptr<Array<CallFrame>> callFrames;
   response = currentCallFrames(&callFrames);
   if (!response.isSuccess()) return response;
@@ -583,8 +649,7 @@ Response V8DebuggerAgentImpl::getScriptSource(const String16& scriptId,
   ScriptsMap::iterator it = m_scripts.find(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();
 }
 
@@ -993,7 +1058,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));
@@ -1036,14 +1101,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;
@@ -1069,8 +1134,15 @@ void V8DebuggerAgentImpl::didParseSource(
                                  &breakpoint.column_number);
     breakpointObject->getString(DebuggerAgentState::condition,
                                 &breakpoint.condition);
+    String16 hint;
+    bool hasHint = breakpointObject->getString(DebuggerAgentState::hint, &hint);
     std::unique_ptr<protocol::Debugger::Location> location =
-        resolveBreakpoint(cookie.first, breakpoint, UserBreakpointSource);
+        resolveBreakpoint(cookie.first, breakpoint, UserBreakpointSource, hint);
+    if (!hasHint) {
+      hint = breakpointHint(*scriptRef, breakpoint);
+      if (!hint.isEmpty())
+        breakpointObject->setString(DebuggerAgentState::hint, hint);
+    }
     if (location)
       m_frontend.breakpointResolved(cookie.first, std::move(location));
   }
@@ -1209,7 +1281,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, /* hint */ String16());
 }
 
 void V8DebuggerAgentImpl::removeBreakpointAt(const String16& scriptId,