Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(600)

Issue 2700743002: [inspector] extend protocol for code coverage. (Closed)

Created:
3 years, 10 months ago by Yang
Modified:
3 years, 10 months ago
Reviewers:
kozy, pfeldman, jgruber
CC:
v8-reviews_googlegroups.com, devtools-reviews_chromium.org, fmeawad
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

Patch Set 1 #

Total comments: 2

Patch Set 2 : address comment. #

Patch Set 3 : comments #

Total comments: 31

Patch Set 4 : address comments #

Patch Set 5 : small fix #

Total comments: 3

Patch Set 6 : rebase #

Patch Set 7 : addressed comments #

Patch Set 8 : fix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+738 lines, -245 lines) Patch
M src/api.cc View 1 2 3 4 5 6 1 chunk +23 lines, -19 lines 0 comments Download
M src/d8.cc View 1 2 3 4 5 6 2 chunks +30 lines, -40 lines 0 comments Download
M src/debug/debug-coverage.h View 1 2 3 4 5 6 1 chunk +6 lines, -6 lines 0 comments Download
M src/debug/debug-coverage.cc View 1 2 3 4 5 6 4 chunks +21 lines, -32 lines 0 comments Download
M src/debug/debug-interface.h View 1 2 3 4 5 6 2 chunks +24 lines, -11 lines 0 comments Download
M src/inspector/js_protocol.json View 1 2 3 4 5 6 2 chunks +58 lines, -0 lines 0 comments Download
M src/inspector/v8-runtime-agent-impl.h View 1 2 3 4 5 6 1 chunk +8 lines, -0 lines 0 comments Download
M src/inspector/v8-runtime-agent-impl.cc View 1 2 3 4 5 6 5 chunks +88 lines, -0 lines 0 comments Download
M src/runtime/runtime-debug.cc View 1 2 3 4 5 6 7 2 chunks +21 lines, -45 lines 0 comments Download
M test/cctest/test-debug.cc View 1 2 3 4 5 6 1 chunk +18 lines, -17 lines 0 comments Download
A test/inspector/runtime/coverage.js View 1 2 3 4 5 6 1 chunk +89 lines, -0 lines 0 comments Download
A test/inspector/runtime/coverage-expected.txt View 1 2 3 4 5 6 1 chunk +333 lines, -0 lines 0 comments Download
M test/mjsunit/code-coverage-ad-hoc.js View 1 2 3 5 chunks +12 lines, -43 lines 0 comments Download
M test/mjsunit/code-coverage-precise.js View 1 2 3 3 chunks +7 lines, -32 lines 0 comments Download

Messages

Total messages: 40 (17 generated)
Yang
3 years, 10 months ago (2017-02-16 09:02:17 UTC) #1
jgruber
lgtm https://codereview.chromium.org/2700743002/diff/1/src/inspector/js_protocol.json File src/inspector/js_protocol.json (right): https://codereview.chromium.org/2700743002/diff/1/src/inspector/js_protocol.json#newcode382 src/inspector/js_protocol.json:382: "parameters": [], Can we remove empty parameters?
3 years, 10 months ago (2017-02-16 09:42:35 UTC) #4
Yang
https://codereview.chromium.org/2700743002/diff/1/src/inspector/js_protocol.json File src/inspector/js_protocol.json (right): https://codereview.chromium.org/2700743002/diff/1/src/inspector/js_protocol.json#newcode382 src/inspector/js_protocol.json:382: "parameters": [], On 2017/02/16 09:42:35, jgruber wrote: > Can ...
3 years, 10 months ago (2017-02-16 09:51:13 UTC) #5
Yang
On 2017/02/16 09:51:13, Yang wrote: > https://codereview.chromium.org/2700743002/diff/1/src/inspector/js_protocol.json > File src/inspector/js_protocol.json (right): > > https://codereview.chromium.org/2700743002/diff/1/src/inspector/js_protocol.json#newcode382 > ...
3 years, 10 months ago (2017-02-16 11:41:32 UTC) #10
Yang
On 2017/02/16 11:41:32, Yang wrote: > On 2017/02/16 09:51:13, Yang wrote: > > > https://codereview.chromium.org/2700743002/diff/1/src/inspector/js_protocol.json ...
3 years, 10 months ago (2017-02-16 12:39:40 UTC) #11
kozy
https://codereview.chromium.org/2700743002/diff/40001/src/inspector/js_protocol.json File src/inspector/js_protocol.json (right): https://codereview.chromium.org/2700743002/diff/40001/src/inspector/js_protocol.json#newcode377 src/inspector/js_protocol.json:377: "description": "Disable precise code coverage. Disabling releases unnecessary execution ...
3 years, 10 months ago (2017-02-16 16:47:19 UTC) #12
kozy
On 2017/02/16 12:39:40, Yang wrote: > I just figured out that you can add types ...
3 years, 10 months ago (2017-02-16 18:17:50 UTC) #13
pfeldman
https://codereview.chromium.org/2700743002/diff/40001/src/inspector/js_protocol.json File src/inspector/js_protocol.json (right): https://codereview.chromium.org/2700743002/diff/40001/src/inspector/js_protocol.json#newcode211 src/inspector/js_protocol.json:211: "properties": [ experimental: true https://codereview.chromium.org/2700743002/diff/40001/src/inspector/js_protocol.json#newcode212 src/inspector/js_protocol.json:212: { "name": "functionName", ...
3 years, 10 months ago (2017-02-16 21:23:10 UTC) #14
Yang
https://codereview.chromium.org/2700743002/diff/40001/src/inspector/js_protocol.json File src/inspector/js_protocol.json (right): https://codereview.chromium.org/2700743002/diff/40001/src/inspector/js_protocol.json#newcode228 src/inspector/js_protocol.json:228: { "name": "toplevel", "$ref": "CoverageRange", "description": "Coverage ranges for ...
3 years, 10 months ago (2017-02-16 21:30:51 UTC) #15
pfeldman
https://codereview.chromium.org/2700743002/diff/40001/src/inspector/js_protocol.json File src/inspector/js_protocol.json (right): https://codereview.chromium.org/2700743002/diff/40001/src/inspector/js_protocol.json#newcode228 src/inspector/js_protocol.json:228: { "name": "toplevel", "$ref": "CoverageRange", "description": "Coverage ranges for ...
3 years, 10 months ago (2017-02-16 23:04:51 UTC) #16
pfeldman
Briefly talked to Fadi - he is suggesting the data is pretty good, so it ...
3 years, 10 months ago (2017-02-16 23:13:42 UTC) #17
Yang
Will address these issues by Monday. Adding Fadi on CC regarding whether to reset counters. ...
3 years, 10 months ago (2017-02-17 05:35:54 UTC) #18
pfeldman
https://codereview.chromium.org/2700743002/diff/40001/src/inspector/js_protocol.json File src/inspector/js_protocol.json (right): https://codereview.chromium.org/2700743002/diff/40001/src/inspector/js_protocol.json#newcode228 src/inspector/js_protocol.json:228: { "name": "toplevel", "$ref": "CoverageRange", "description": "Coverage ranges for ...
3 years, 10 months ago (2017-02-17 17:59:39 UTC) #19
Yang
Addressed comments. Changed data structure. https://codereview.chromium.org/2700743002/diff/40001/src/inspector/js_protocol.json File src/inspector/js_protocol.json (right): https://codereview.chromium.org/2700743002/diff/40001/src/inspector/js_protocol.json#newcode211 src/inspector/js_protocol.json:211: "properties": [ On 2017/02/16 ...
3 years, 10 months ago (2017-02-20 11:24:42 UTC) #20
kozy
lgtm https://codereview.chromium.org/2700743002/diff/80001/test/inspector/runtime/coverage.js File test/inspector/runtime/coverage.js (right): https://codereview.chromium.org/2700743002/diff/80001/test/inspector/runtime/coverage.js#newcode34 test/inspector/runtime/coverage.js:34: .then((result) => InspectorTest.logMessage(result)) .then(InspectorTest.logMessage) https://codereview.chromium.org/2700743002/diff/80001/test/inspector/runtime/coverage.js#newcode53 test/inspector/runtime/coverage.js:53: .then(() => ...
3 years, 10 months ago (2017-02-21 17:57:30 UTC) #25
pfeldman
https://codereview.chromium.org/2700743002/diff/80001/src/inspector/js_protocol.json File src/inspector/js_protocol.json (right): https://codereview.chromium.org/2700743002/diff/80001/src/inspector/js_protocol.json#newcode398 src/inspector/js_protocol.json:398: "name": "takeBestEffortCoverage", getBestEffortCoverage (it does not reset, so should ...
3 years, 10 months ago (2017-02-21 18:32:00 UTC) #26
Yang
On 2017/02/21 18:32:00, pfeldman wrote: > https://codereview.chromium.org/2700743002/diff/80001/src/inspector/js_protocol.json > File src/inspector/js_protocol.json (right): > > https://codereview.chromium.org/2700743002/diff/80001/src/inspector/js_protocol.json#newcode398 > ...
3 years, 10 months ago (2017-02-22 09:00:48 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2700743002/120001
3 years, 10 months ago (2017-02-22 09:49:40 UTC) #30
commit-bot: I haz the power
Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/builds/13533)
3 years, 10 months ago (2017-02-22 09:56:17 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2700743002/140001
3 years, 10 months ago (2017-02-22 09:58:56 UTC) #35
commit-bot: I haz the power
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/v8/v8/+/901c29eb1c5783e72b8926f0a1b0d9e13b69f3c0
3 years, 10 months ago (2017-02-22 10:22:03 UTC) #38
nojvek
On 2017/02/22 10:22:03, commit-bot: I haz the power wrote: > Committed patchset #8 (id:140001) as ...
3 years, 10 months ago (2017-02-24 05:40:32 UTC) #39
kozy
3 years, 10 months ago (2017-02-24 05:51:20 UTC) #40
Message was sent while issue was closed.
On 2017/02/24 05:40:32, nojvek wrote:
> On 2017/02/22 10:22:03, commit-bot: I haz the power wrote:
> > Committed patchset #8 (id:140001) as
> >
>
https://chromium.googlesource.com/v8/v8/+/901c29eb1c5783e72b8926f0a1b0d9e13b6...
> 
> This breaks the v8 protocol schema.
> 
>
https://chromium.googlesource.com/v8/v8.git/+/901c29eb1c5783e72b8926f0a1b0d9e...
> 
> Please use "experimental": true (boolean) and not "experimental: "true"
> (string).

I'll include this change into https://codereview.chromium.org/2715833003/. this
CL is moving coverage related methods from Runtime to Profiler.

Powered by Google App Engine
This is Rietveld 408576698