Skip to content
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

Default context #8502

Closed
wants to merge 1 commit into from
Closed

Default context #8502

wants to merge 1 commit into from

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Sep 12, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

This change touches only the inspector integration.

Description of change

Inspector now returns a proper context when the default context is requested. This makes it possible to omit context ID from some requests.

Note that this PR is built on top of another PR (#8429) and adds a new test. I will roll the test change into that PR if this one is greenlighted first.

CC: @ofrobots

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. tools Issues and PRs related to the tools directory. inspector Issues and PRs related to the V8 inspector protocol labels Sep 12, 2016
@mscdex mscdex removed build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Sep 12, 2016
function checkHttpResponse(port, path, callback) {
http.get({port, path}, function(r) {
let response = '';
r.
Copy link
Member

Choose a reason for hiding this comment

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

Call r.setEncoding('utf8') first if you want string data in the data event listener. (And maybe rename the variable to res for clarity.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change belongs to #8429 - I updated that PR

@bnoordhuis
Copy link
Member

LGTM with a comment. Can you lowercase the status lines of the commit logs?

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 13, 2016

Updated the status lines. Thanks for the review!

@ofrobots
Copy link
Contributor

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 14, 2016

I updated the CL, should now pass the test on Windows.

@ofrobots
Copy link
Contributor

@ofrobots
Copy link
Contributor

Landed as efe4d19.

@ofrobots ofrobots closed this Sep 20, 2016
ofrobots pushed a commit that referenced this pull request Sep 20, 2016
Fixes: #8426
PR-URL: #8502
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2016
Fixes: nodejs#8426
PR-URL: nodejs#8502
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
Fixes: #8426
PR-URL: #8502
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@eugeneo eugeneo deleted the default_context branch October 13, 2016 23:33
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++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants