[inspector] added type of break location into getPossibleBreakpoints output

src/api.cc

Index: src/api.cc
diff --git a/src/api.cc b/src/api.cc
index cfd51a30d220ee4f670f0ffd477fa44ea3abddab..ed0b02dfa94d0503e74685e3375579912d424152 100644
--- a/src/api.cc
+++ b/src/api.cc
@@ -9240,7 +9240,8 @@ int GetSmiValue(i::Handle<i::FixedArray> array, int index) {
 
 bool debug::Script::GetPossibleBreakpoints(
     const debug::Location& start, const debug::Location& end,
-    bool restrict_to_function, std::vector<debug::Location>* locations) const {
+    bool restrict_to_function,
+    std::vector<debug::BreakLocation>* locations) const {
   CHECK(!start.IsEmpty());
   i::Handle<i::Script> script = Utils::OpenHandle(this);
   if (script->type() == i::Script::TYPE_WASM) {
@@ -9262,15 +9263,30 @@ bool debug::Script::GetPossibleBreakpoints(
                        : GetSourceOffset(end);
   if (start_offset >= end_offset) return true;
 
-  std::set<int> offsets;
+  std::vector<i::BreakLocation> v8_locations;
   if (!isolate->debug()->GetPossibleBreakpoints(
-          script, start_offset, end_offset, restrict_to_function, &offsets)) {
+          script, start_offset, end_offset, restrict_to_function,
+          &v8_locations)) {
     return false;
   }
 
+  std::map<int, i::BreakLocation> unique_locations;
+  for (const auto& location : v8_locations) {

[Yang, 2017/03/02 20:06:57]

Some comments would be great. What issue is this supposed to work around? From
what I can tell, we are deduplicating breaks with the same source location, but
prefer call/return/debugger locations over normal statement positions? I don't
think we need to do this for debugger or return statements. Because these are
already statements too.

Also, I'd prefer if this kind of logic is either put in the debugger or in the
inspector, not in api.cc where we should mostly only have glue code.

[kozy, 2017/03/02 22:38:09]

Added a comment and DCHECK.
From my perspective of view it's a glue code:
- on one side we have v8::Debug with all possible locations and with a way to
set breakpoint by each of them, there are no reason to merge locations there,
- on another side we have inspector and currently we can put breakpoint only by
first break location at current source location - it means there are no reason
to report all break locations to inspector.

+    auto it = unique_locations.find(location.position());
+    if (it != unique_locations.end() &&
+        (it->second.IsCall() || it->second.IsDebuggerStatement() ||
+         it->second.IsReturn())) {
+      continue;
+    }
+    if (it == unique_locations.end()) {
+      unique_locations.insert(std::make_pair(location.position(), location));
+    } else {

[Yang, 2017/03/02 20:06:57]

Can we DCHECK that this has to be a call location?

[kozy, 2017/03/02 22:38:09]

Done.

+      it->second = location;
+    }
+  }
   int current_line_end_index = 0;
-  for (const auto& it : offsets) {
-    int offset = it;
+  for (const auto& it : unique_locations) {
+    int offset = it.first;
     while (offset > GetSmiValue(line_ends, current_line_end_index)) {
       ++current_line_end_index;
       CHECK(current_line_end_index < line_ends->length());
@@ -9280,10 +9296,12 @@ bool debug::Script::GetPossibleBreakpoints(
     if (current_line_end_index > 0) {
       line_offset = GetSmiValue(line_ends, current_line_end_index - 1) + 1;
     }
-    locations->push_back(debug::Location(
+    locations->emplace_back(
         current_line_end_index + script->line_offset(),
         offset - line_offset +
-            (current_line_end_index == 0 ? script->column_offset() : 0)));
+            (current_line_end_index == 0 ? script->column_offset() : 0),
+        it.second.IsCall(), it.second.IsReturn(),
+        it.second.IsDebuggerStatement());
   }
   return true;
 }