Skip to content

inspector: Move JS API code to separate file #16056

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.

Sign up for GitHub

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

Jump to bottom
Closed
eugeneo wants to merge 1 commit into from
Closed

inspector: Move JS API code to separate file #16056

eugeneo wants to merge 1 commit into from

Conversation

@eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Oct 6, 2017

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

inspector: refactored

This refactoring moves all the code supporting inspector JS APIs to a separate file. The reason for this is to make it easier to navigate the inspector_agent.js file (also this should force better C++ API for inspector agent). Debug signal handling code also does not belong to inspector_agent.cc file, but that will be refactored in a separate change.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 6, 2017
@refack refack added the inspector Issues and PRs related to the V8 inspector protocol label Oct 6, 2017
TimothyGu
TimothyGu approved these changes Oct 7, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If CI is happy, I'm happy.

src/inspector_agent.h Outdated
Copy link
Member

@TimothyGu TimothyGu Oct 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: IsWaitingForConnect?

Copy link
Contributor Author

@eugeneo eugeneo Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks!

@eugeneo
Copy link
Contributor Author

eugeneo commented Oct 9, 2017

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

@eugeneo
Copy link
Contributor Author

eugeneo commented Oct 9, 2017

CI shows some compilation issues on some platforms - I will fix them.

@eugeneo
Copy link
Contributor Author

eugeneo commented Oct 9, 2017 •
edited
Loading

Included limits.h on Posix systems (looks like it used to be included through another header).

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

mhdawson
mhdawson approved these changes Oct 11, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once the CI is happy.

jasnell
jasnell approved these changes Oct 13, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rubber stamp LGTM

@eugeneo
Copy link
Contributor Author

eugeneo commented Oct 13, 2017

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

jasnell pushed a commit that referenced this pull request Oct 13, 2017
@jasnell
inspector: Move JS API code to separate file
4faa231 
PR-URL: #16056
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 13, 2017

Landed in 4faa231

@jasnell jasnell closed this Oct 13, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
@addaleax
inspector: Move JS API code to separate file
9d1748e 
PR-URL: nodejs/node#16056
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@MylesBorins
inspector: Move JS API code to separate file
1956ae7 
PR-URL: #16056
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@eugeneo eugeneo deleted the js_api branch April 26, 2018 17:34
@MarcelRaschke MarcelRaschke mentioned this pull request Aug 19, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@mhdawson mhdawson mhdawson approved these changes

+1 more reviewer

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@eugeneo @jasnell @addaleax @TimothyGu @mhdawson @refack @MylesBorins @nodejs-github-bot