[inspector] added type of break location into getPossibleBreakpoints output

[inspector] added type of break location into getPossibleBreakpoints output

This CL provide type with each break location, type could be: call, return or debugger statement.

BUG=chromium:432469
R=yangguo@chromium.org,dgozman@chromium.org

Review-Url: https://codereview.chromium.org/2728563002
Cr-Commit-Position: refs/heads/master@{#43619}
Committed: https://chromium.googlesource.com/v8/v8/+/562da356141da650d0597ddf2fb075ee997a3622

[kozy, 2017-03-01 03:07:34]

Dmitry and Yang, please take a look.

https://codereview.chromium.org/2728563002/diff/1/test/inspector/debugger/get...
File test/inspector/debugger/get-possible-breakpoints-master-expected.txt
(right):

https://codereview.chromium.org/2728563002/diff/1/test/inspector/debugger/get...
test/inspector/debugger/get-possible-breakpoints-master-expected.txt:98: for
(var k |C|of []) {}
surprise! will debug and fix it in followup.

[dgozman, 2017-03-01 22:10:47]

https://codereview.chromium.org/2728563002/diff/20001/src/debug/interface-typ...
File src/debug/interface-types.h (right):

https://codereview.chromium.org/2728563002/diff/20001/src/debug/interface-typ...
src/debug/interface-types.h:75: class V8_EXPORT_PRIVATE BreakLocation {
Let's inherit from Location?

https://codereview.chromium.org/2728563002/diff/20001/src/inspector/js_protoc...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2728563002/diff/20001/src/inspector/js_protoc...
src/inspector/js_protocol.json:479: { "name": "location", "$ref": "Location",
"description": "Possible break location." },
Let's "inherit" here as well.

[kozy, 2017-03-01 23:14:13]

thanks, all done!

https://codereview.chromium.org/2728563002/diff/20001/src/debug/interface-typ...
File src/debug/interface-types.h (right):

https://codereview.chromium.org/2728563002/diff/20001/src/debug/interface-typ...
src/debug/interface-types.h:75: class V8_EXPORT_PRIVATE BreakLocation {
On 2017/03/01 22:10:47, dgozman wrote:
> Let's inherit from Location?

Done.

https://codereview.chromium.org/2728563002/diff/20001/src/inspector/js_protoc...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2728563002/diff/20001/src/inspector/js_protoc...
src/inspector/js_protocol.json:479: { "name": "location", "$ref": "Location",
"description": "Possible break location." },
On 2017/03/01 22:10:47, dgozman wrote:
> Let's "inherit" here as well.

Done.

[kozy, 2017-03-01 23:14:46]

kozyatinskiy@chromium.org changed reviewers:
+ clemensh@chromium.org

[kozy, 2017-03-01 23:14:46]

+Clemens, I need owner lgtm for two wasm related files.

[kozy, 2017-03-01 23:14:57]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 23:15:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 23:19:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 23:19:51]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/23440)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[kozy, 2017-03-01 23:27:28]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 23:27:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 23:55:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 23:55:47]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-03-02 10:09:07]

On 2017/03/01 at 23:14:46, kozyatinskiy wrote:
> +Clemens, I need owner lgtm for two wasm related files.

src/wasm and test/cctest/wasm LGTM

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

https://codereview.chromium.org/2728563002/diff/60001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2728563002/diff/60001/src/api.cc#newcode9274
src/api.cc:9274: for (const auto& location : v8_locations) {
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.

https://codereview.chromium.org/2728563002/diff/60001/src/api.cc#newcode9283
src/api.cc:9283: } else {
Can we DCHECK that this has to be a call location?

https://codereview.chromium.org/2728563002/diff/60001/src/debug/interface-typ...
File src/debug/interface-types.h (right):

https://codereview.chromium.org/2728563002/diff/60001/src/debug/interface-typ...
src/debug/interface-types.h:89: bool is_call_;
This could also be an enum. Just saying :)

[kozy, 2017-03-02 22:38:10]

please take another look.

https://codereview.chromium.org/2728563002/diff/60001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2728563002/diff/60001/src/api.cc#newcode9274
src/api.cc:9274: for (const auto& location : v8_locations) {
On 2017/03/02 20:06:57, Yang wrote:
> 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.

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.

https://codereview.chromium.org/2728563002/diff/60001/src/api.cc#newcode9283
src/api.cc:9283: } else {
On 2017/03/02 20:06:57, Yang wrote:
> Can we DCHECK that this has to be a call location?

Done.

https://codereview.chromium.org/2728563002/diff/60001/src/debug/interface-typ...
File src/debug/interface-types.h (right):

https://codereview.chromium.org/2728563002/diff/60001/src/debug/interface-typ...
src/debug/interface-types.h:89: bool is_call_;
On 2017/03/02 20:06:57, Yang wrote:
> This could also be an enum. Just saying :)

Done.

[dgozman, 2017-03-03 18:55:56]

lgtm

https://codereview.chromium.org/2728563002/diff/80001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2728563002/diff/80001/src/api.cc#newcode9249
src/api.cc:9249: for (const auto& location : v8_locations) {
I agree with Yang that we should move this to inspector code and let
debug-interface expose all break locations available.

[kozy, 2017-03-03 22:44:15]

all done, Yang please take another look.

https://codereview.chromium.org/2728563002/diff/80001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2728563002/diff/80001/src/api.cc#newcode9249
src/api.cc:9249: for (const auto& location : v8_locations) {
On 2017/03/03 18:55:56, dgozman wrote:
> I agree with Yang that we should move this to inspector code and let
> debug-interface expose all break locations available.

Done.

[kozy, 2017-03-03 22:44:21]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 22:44:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 23:11:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-03 23:11:39]

Dry run: This issue passed the CQ dry run.

[Yang, 2017-03-06 14:41:11]

LGTM. nice.

https://codereview.chromium.org/2728563002/diff/100001/src/inspector/v8-debug...
File src/inspector/v8-debugger-script.cc (right):

https://codereview.chromium.org/2728563002/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger-script.cc:176: current = allLocations[i];
Can we DCHECK that we only overwrite a common break location with a call
location? I don't think any other case can occur, and if it does, we probably
are doing something wrong.

[kozy, 2017-03-06 18:25:10]

Patchset #7 (id:120001) has been deleted

[kozy, 2017-03-06 18:25:25]

https://codereview.chromium.org/2728563002/diff/100001/src/inspector/v8-debug...
File src/inspector/v8-debugger-script.cc (right):

https://codereview.chromium.org/2728563002/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger-script.cc:176: current = allLocations[i];
On 2017/03/06 14:41:11, Yang wrote:
> Can we DCHECK that we only overwrite a common break location with a call
> location? I don't think any other case can occur, and if it does, we probably
> are doing something wrong.

I added DCHECK and it was failed on generator function like:
function* idMaker() {
  yield 1;
  yield 2;
  yield 3;
}
For some reasons this code generates a lot of return locations, maybe we need to
have separate location per each generator call or it's side effect of generators
implementation.

[kozy, 2017-03-06 18:25:29]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-03-06 18:25:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
clemensh@chromium.org, dgozman@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2728563002/#ps140001
(title: "added DCHECK")

[commit-bot: I haz the power, 2017-03-06 18:25:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 20:47:56]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488824729334230,
"parent_rev": "a3923ce210c305fda4ec3f4d97f5a55f99e47b89", "commit_rev":
"562da356141da650d0597ddf2fb075ee997a3622"}

[commit-bot: I haz the power, 2017-03-06 20:48:01]

Description was changed from

==========
[inspector] added type of break location into getPossibleBreakpoints output

This CL provide type with each break location, type could be: call, return or
debugger statement.

BUG=chromium:432469
R=yangguo@chromium.org,dgozman@chromium.org
==========

to

==========
[inspector] added type of break location into getPossibleBreakpoints output

This CL provide type with each break location, type could be: call, return or
debugger statement.

BUG=chromium:432469
R=yangguo@chromium.org,dgozman@chromium.org

Review-Url: https://codereview.chromium.org/2728563002
Cr-Commit-Position: refs/heads/master@{#43619}
Committed:
https://chromium.googlesource.com/v8/v8/+/562da356141da650d0597ddf2fb075ee997...
==========

[commit-bot: I haz the power, 2017-03-06 20:48:02]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/562da356141da650d0597ddf2fb075ee997...