Skip to content

inspector: Move JS API code to separate file - #16056

Closed
eugeneo wants to merge 1 commit into
nodejs:masterfrom
eugeneo:js_api
Closed

inspector: Move JS API code to separate file#16056
eugeneo wants to merge 1 commit into
nodejs:masterfrom
eugeneo:js_api

Conversation

@eugeneo

@eugeneo eugeneo commented Oct 6, 2017

Copy link
Copy Markdown
Contributor
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 involving general changes in the lib/ or src/ directories. 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

@TimothyGu TimothyGu left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/inspector_agent.h Outdated

@TimothyGu TimothyGu Oct 7, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: IsWaitingForConnect?

@eugeneo eugeneo Oct 9, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks!

@eugeneo

eugeneo commented Oct 9, 2017

Copy link
Copy Markdown
Contributor Author

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

@eugeneo

eugeneo commented Oct 9, 2017

Copy link
Copy Markdown
Contributor Author

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

@eugeneo

eugeneo commented Oct 9, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

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

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

rubber stamp LGTM

@eugeneo

eugeneo commented Oct 13, 2017

Copy link
Copy Markdown
Contributor Author

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

jasnell pushed a commit that referenced this pull request Oct 13, 2017
Eugene Ostroukhov @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

jasnell commented Oct 13, 2017

Copy link
Copy Markdown
Member

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
Eugene Ostroukhov @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
Eugene Ostroukhov @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
+2 more reviewers
@TimothyGu TimothyGu TimothyGu approved these changes
@mhdawson mhdawson mhdawson 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 involving general changes in the lib/ or src/ directories.

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