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.

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

deps: update v8_inspector #8014

Merged
merged 1 commit into from
Aug 11, 2016
Merged

deps: update v8_inspector #8014

merged 1 commit into from
Aug 11, 2016

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Aug 8, 2016 •
edited
Loading

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

Update to the latest v8_inspector (pavelfeldman/v8_inspector@1352de2).

@nodejs-github-bot nodejs-github-bot added the inspector Issues and PRs related to the V8 inspector protocol label Aug 8, 2016
@ofrobots ofrobots mentioned this pull request Aug 8, 2016
Closed
4 tasks
@bnoordhuis
Copy link
Member

Changes to src/ LGTM and rubber-stamp LGTM w.r.t. everything else.

@ofrobots
Copy link
Contributor Author

ofrobots commented Aug 8, 2016

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

@ofrobots
Copy link
Contributor Author

Updated this PR to pick up a more recent version from upstream, given that it is needed for #8043. Slight changes to the code in src/, so this needs another look.

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

@ofrobots
Copy link
Contributor Author

One more roll to pick the fix needed for v6.x compatibility. New CI: https://ci.nodejs.org/job/node-test-pull-request/3615/

@ofrobots ofrobots mentioned this pull request Aug 10, 2016
Closed
@cjihrig
Copy link
Contributor

cjihrig commented Aug 10, 2016

Was the hard coded "NodeJS Main Context" part of the original changes that got signed off on? If so, LGTM and the CI is green.

@ofrobots
Copy link
Contributor Author

Was the hard coded "NodeJS Main Context" part of the original changes that got signed off on? If so, LGTM and the CI is green.

Yes, that was part of the original changes. The context name, "NodeJS Main Context", used to be hardcoded in the upstream v8_inspector code but now the embedder can provide the name.

The CI is green. I will go ahead and land this in a couple of hours.

@ofrobots
deps: v8_inspector update
e40234d 
Pick up latest from [1] corresponding to the Blink commit 62cd277.

[1]: pavelfeldman/v8_inspector@e6b8355

PR-URL: nodejs#8014
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
@ofrobots ofrobots merged commit e40234d into nodejs:master Aug 11, 2016
cjihrig pushed a commit that referenced this pull request Aug 11, 2016
@ofrobots @cjihrig
deps: v8_inspector update
a9fe85e 
Pick up latest from [1] corresponding to the Blink commit 62cd277.

[1]: pavelfeldman/v8_inspector@e6b8355

PR-URL: #8014
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
Merged
@ofrobots ofrobots deleted the update-inspector branch August 11, 2016 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ofrobots @bnoordhuis @cjihrig @MylesBorins @nodejs-github-bot