Skip to content

inspector: use inspector API for "break on start"#12076

Merged
eugeneo merged 0 commit into
nodejs:masterfrom
eugeneo:continue-to-location
Mar 30, 2017
Merged

inspector: use inspector API for "break on start"#12076
eugeneo merged 0 commit into
nodejs:masterfrom
eugeneo:continue-to-location

Conversation

@eugeneo

@eugeneo eugeneo commented Mar 27, 2017

Copy link
Copy Markdown
Contributor

This change removes a need for using deprecated debug context for
breaking at the start of the main module.

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

inspector

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x lib / src Issues and PRs related to general changes in the lib or src directory. module Issues and PRs related to the module subsystem. labels Mar 27, 2017
@eugeneo

eugeneo commented Mar 27, 2017

Copy link
Copy Markdown
Contributor Author

cc @ofrobots

Comment thread lib/internal/bootstrap_node.js Outdated

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.

===

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.

Done

Comment thread lib/module.js Outdated

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.

Ditto.

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.

Done

Comment thread src/inspector_agent.cc Outdated

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.

The HandleScope isn't necessary, API functions get one implicitly.

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.

Done.

Comment thread src/inspector_agent.cc Outdated

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.

Just CHECK(args.Length() > 1) and CHECK(args[0]->IsFunction())? This is only called by built-in code, right?

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.

For now, it is not supposed to be a public API. This might change in the future.

Comment thread src/inspector_agent.cc Outdated

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.

This effectively breaks on the next call into JS land? If this function were to return instead of calling the function, it would break at the next line in lib/module.js?

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.

It breaks on the next JS instruction. E.g. I just noticed that it is not breaking inside the wrapper - it seems to break on the wrapper call (e.g. one step is required to reach the same point it was breaking before). It does not make any difference for the tests as the line number and variables are the same.

@eugeneo

eugeneo commented Mar 29, 2017

Copy link
Copy Markdown
Contributor Author

Thank you for the review. I addressed the comments, please take another look.

Comment thread src/inspector_agent.cc Outdated

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.

Can you check that session_ != nullptr?

Comment thread src/inspector_agent.cc Outdated

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.

env->context() might be simpler

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.

Done.

@jasnell

jasnell commented Mar 30, 2017

Copy link
Copy Markdown
Member

CI has some challenges but they appear unrelated.

@eugeneo eugeneo modified the milestone: ake -j16 test Mar 30, 2017
@eugeneo eugeneo closed this Mar 30, 2017
@eugeneo eugeneo deleted the continue-to-location branch March 30, 2017 20:27
@eugeneo

eugeneo commented Mar 30, 2017

Copy link
Copy Markdown
Contributor Author

Landed as 7954d2a

@eugeneo eugeneo merged commit 7954d2a into nodejs:master Mar 30, 2017
@jasnell jasnell mentioned this pull request Apr 4, 2017
@italoacasas

Copy link
Copy Markdown

cc @eugeneo

@eugeneo

eugeneo commented Apr 10, 2017

Copy link
Copy Markdown
Contributor Author

@italoacasas - sorry, I forgot to add labels. This code does not need to be ported to pre-8x as those versions still rely on debug context. Thanks!

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

Labels

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. module Issues and PRs related to the module subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants