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

inspector: code cleanup #21070

Merged
merged 1 commit into from
Jun 5, 2018
Merged

inspector: code cleanup #21070

merged 1 commit into from
Jun 5, 2018

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented May 31, 2018

Remove some unused code from the WS server implementation and switch to
smart pointers where possible.

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

@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 May 31, 2018
@refack refack added the inspector Issues and PRs related to the V8 inspector protocol label May 31, 2018
bnoordhuis
bnoordhuis approved these changes Jun 1, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM but this really should have been multiple commits for easier review and bisecting.

@eugeneo
Copy link
Contributor Author

eugeneo commented Jun 1, 2018

@refack What verification do you mean? Is there something I need to setup?

@jasnell
Copy link
Member

jasnell commented Jun 1, 2018

You can disregard that last comment from refack... It was caused by a misconfigured Jenkins bot

@eugeneo
Copy link
Contributor Author

eugeneo commented Jun 1, 2018

Thanks for the reviews. I uploaded a new revision of the test case, cannot figure out why it does not compile on Windows.

Note that this PR is PR 1 of 3 (planned currently) that is laying the groundwork for the workers support.

@eugeneo
Copy link
Contributor Author

eugeneo commented Jun 3, 2018

Windows issue should be resolved now (it was a test problem).

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

@eugeneo
Copy link
Contributor Author

eugeneo commented Jun 4, 2018

There seems to be intermittance in one test on Windows. I tried a stress test (100 runs): https://ci.nodejs.org/job/node-stress-single-test/1907/nodes=win2012r2-mp-vs2017/console - it said that 100 runs out of 100 were ok (for some reason it still said the test was flaky, I suppose it is test harness bug).
I also rerun CI for this PR: https://ci.nodejs.org/job/node-test-pull-request/15252/ and it came out green.

I do not know if that is flakiness and if it is, whether this change introduced it. I would like to proceed landing this change to unblock next changes that build on top of this one.

@eugeneo
inspector: code cleanup
327ce2d 
Remove some unused code from the WS server implementation and switch to
smart pointers where possible.

PR-URL: #21070
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@eugeneo
Copy link
Contributor Author

eugeneo commented Jun 5, 2018 •
edited
Loading

Pre-merge CI - https://ci.nodejs.org/job/node-test-pull-request/15262/

Fails on Windows bot on test-async-hooks, seems to be unlikely to be caused by this PR.
Failure on Linux bot is the result of "Starting inspector on 127.0.0.1:40558 failed: address already in use" - seems to be a port conflict

@eugeneo eugeneo merged commit 327ce2d into nodejs:master Jun 5, 2018
@eugeneo eugeneo deleted the smart-pointers branch June 5, 2018 17:37
MylesBorins pushed a commit that referenced this pull request Jun 6, 2018
@eugeneo @MylesBorins
inspector: code cleanup
1c211ec 
Remove some unused code from the WS server implementation and switch to
smart pointers where possible.

PR-URL: #21070
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Jun 6, 2018
Merged
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

@bnoordhuis bnoordhuis bnoordhuis approved these changes

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.
5 participants
@eugeneo @jasnell @bnoordhuis @refack @nodejs-github-bot