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

inspector: stop relying on magic strings #10159

Merged
merged 0 commits into from
Dec 9, 2016
Merged

inspector: stop relying on magic strings #10159

merged 0 commits into from
Dec 9, 2016

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Dec 6, 2016

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

inspector: internal datastructures were slightly altered

Description of change

Inspector uses magical strings to communicate some events between
main thread and transport thread. This change replaces those strings
with enums that are more mainatainable (and remove unnecessary
encodings/decodings)

@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 inspector Issues and PRs related to the V8 inspector protocol labels Dec 6, 2016
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 with style nits.

void ResumeStartup() {
uv_sem_post(&start_sem_);
}

private:
template<typename Action>
Copy link
Member

Choose a reason for hiding this comment

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

Space before '<' here and elsewhere.

@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 9, 2016

Landed as d8c7534

@Fishrock123
Copy link
Contributor

@eugeneo Doesn't land cleanly on v7.x fwiw

@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 13, 2016

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
Inspector uses magical strings to communicate some events between
main thread and transport thread. This change replaces those strings
with enums that are more mainatainable (and remove unnecessary
encodings/decodings)

PR-URL: nodejs#10159
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Jan 28, 2017
Inspector uses magical strings to communicate some events between
main thread and transport thread. This change replaces those strings
with enums that are more mainatainable (and remove unnecessary
encodings/decodings)

PR-URL: #10159
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
Inspector uses magical strings to communicate some events between
main thread and transport thread. This change replaces those strings
with enums that are more mainatainable (and remove unnecessary
encodings/decodings)

PR-URL: nodejs#10159
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
Inspector uses magical strings to communicate some events between
main thread and transport thread. This change replaces those strings
with enums that are more mainatainable (and remove unnecessary
encodings/decodings)

PR-URL: nodejs#10159
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

A backport PR would be required in order for this to land in v6

@eugeneo
Copy link
Contributor Author

eugeneo commented Mar 8, 2017

I don't think this needs to be backported...

@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

Awesome. Thank you

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