New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inspector - Use script name for target title #8243

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@eugeneo
Contributor

eugeneo commented Aug 23, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

This only touches JSON API of the V8 Inspector integration.

Description of change

Changes inspector integration to use Node.js script file name as target
title (reported in JSON and shown in developer tools UIs). It will also
report file:// URL for the script as some tools seem to use that field
to open the script in the editor.

CC: @ofrobots

@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 25, 2016

Member

LGTM, ping @bnoordhuis

Member

jasnell commented Aug 25, 2016

LGTM, ping @bnoordhuis

@ofrobots

View changes

Show outdated Hide outdated src/node.cc
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Aug 26, 2016

Contributor

Thank you for the review. I uploaded a revised CL, please take a look.

Contributor

eugeneo commented Aug 26, 2016

Thank you for the review. I uploaded a revised CL, please take a look.

@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_agent.cc
@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Aug 27, 2016

Member

LGTM with some comments. The missing uv_fs_req_cleanup() is a bug that should be fixed, though.

Member

bnoordhuis commented Aug 27, 2016

LGTM with some comments. The missing uv_fs_req_cleanup() is a bug that should be fixed, though.

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Aug 29, 2016

Contributor

Thank you for the review. CL was updated. Please take another look.

Contributor

eugeneo commented Aug 29, 2016

Thank you for the review. CL was updated. Please take another look.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Aug 30, 2016

Member

LGTM

EDIT: If you s/inspector - Use/inspector: use/ the commit log.

Member

bnoordhuis commented Aug 30, 2016

LGTM

EDIT: If you s/inspector - Use/inspector: use/ the commit log.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Aug 30, 2016

Contributor

I can fix the commit abstract at landing time.

New CI: https://ci.nodejs.org/job/node-test-pull-request/3887/

Contributor

ofrobots commented Aug 30, 2016

I can fix the commit abstract at landing time.

New CI: https://ci.nodejs.org/job/node-test-pull-request/3887/

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Aug 31, 2016

Contributor

@eugeneo: can you fix the linter errors from the CI?

Contributor

ofrobots commented Aug 31, 2016

@eugeneo: can you fix the linter errors from the CI?

Eugene Ostroukhov
inspector - Use script name for target title
Changes inspector integration to use Node.js script file name as target
title (reported in JSON and shown in developer tools UIs). It will also
report file:// URL for the script as some tools seem to use that field
to open the script in the editor.
@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Aug 31, 2016

Contributor

Done. I'm sorry - forgot to run make cpplint...

Contributor

eugeneo commented Aug 31, 2016

Done. I'm sorry - forgot to run make cpplint...

@ofrobots

This comment has been minimized.

Show comment
Hide comment
Contributor

ofrobots commented Sep 1, 2016

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Sep 2, 2016

Contributor

CI is green (enough). Failures are unrelated / buildbot issues.

Contributor

ofrobots commented Sep 2, 2016

CI is green (enough). Failures are unrelated / buildbot issues.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Sep 2, 2016

Contributor

Landed as 609a265.

Contributor

ofrobots commented Sep 2, 2016

Landed as 609a265.

@ofrobots ofrobots closed this Sep 2, 2016

ofrobots added a commit that referenced this pull request Sep 2, 2016

inspector: use script name for target title
Changes inspector integration to use Node.js script file name as target
title (reported in JSON and shown in developer tools UIs). It will also
report file:// URL for the script as some tools seem to use that field
to open the script in the editor.

PR-URL: #8243
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

inspector: use script name for target title
Changes inspector integration to use Node.js script file name as target
title (reported in JSON and shown in developer tools UIs). It will also
report file:// URL for the script as some tools seem to use that field
to open the script in the editor.

PR-URL: nodejs#8243
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

 Conflicts:
	src/node.cc

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

inspector: use script name for target title
Changes inspector integration to use Node.js script file name as target
title (reported in JSON and shown in developer tools UIs). It will also
report file:// URL for the script as some tools seem to use that field
to open the script in the editor.

PR-URL: #8243
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

 Conflicts:
	src/node.cc

@eugeneo eugeneo deleted the eugeneo:title_url branch Sep 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment