inspector: refactor to rename and comment methods #13321

sam-github · 2017-05-30T23:46:16Z

@eugeneo What do you think? Its "cosmetic" in some ways, but the renaming helped me understand the code better. I've a couple open questions, I'll comment on my own PR to point them out.

Pure refactor, makes no functional changes but the renaming helped me
see more clearly what the relationship was between methods and
variables.

Renamed methods to reduce number of slightly different names for the
same thing ("thread" vs "io thread", etc.).
Annotated many local functions as static.
Added comments where it was useful to me.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

addaleax · 2017-05-30T23:49:55Z

src/inspector_io.cc

This is inside an anonymous namespace, adding static doesn’t do anything here

Thanks, I didn't notice that, I'll review all the statics for this (so you don't have to comment on each one).

eugeneo

Removing statics in anonymous namespaces will reduce the size of this patch.

eugeneo · 2017-05-30T23:51:25Z

src/inspector_agent.h

I don't think there's ever be a need. And if we manage to make WS transport talk to inspector through the JS API then there will be only one InspectorSessionDelegate implementation, which will remove the need for this interface.

I mean that I find "OnMessage()" to not clearly indicate the purpose, from the name it could be used to send messages either way, whereas SendMessageToFrontend() clearly indicates the direction of the message flow.

I agree. Still hope to get rid of this interface soon :)

Got it. I'll rename, only takes a minute, and I promise I will feel no sadness if you delete any of the renamed functions.

sam-github

Commented on a couple more renamings/tiny refactorings I think would be worthwhile, but would like confirmation on. I could comment on every rename, but I hope they are mostly self-explanatory.

sam-github · 2017-05-30T23:47:59Z

src/inspector_agent.h

I think this rename would make meaning more clear.

sam-github · 2017-05-30T23:48:58Z

src/inspector_socket.cc

Perhaps I misunderstand node naming conventions, but I thought C++ functions were supposed to be CamelCase, and only variables are supposed to be snake_case.

This class was a plain C as it was mimicking libuv conventions. Need to become proper C++ at some point...

sam-github · 2017-05-30T23:51:25Z

src/inspector_socket.h

I found this confusing, it is a TCP connection, and it is connected to a "client" on the other side, but this side is the "server" side of the HTTP/Websocket connection. I'd suggest just calling it "tcp" (because it is a uv_tcp_t) or "socket" (because it wraps a socket).

tcp/socket - both sound good. Ultimately, I believe this field name came from libuv API uv_accept that calls the connection socket "client" as opposed to "server" that is a listening socket.

sam-github · 2017-05-30T23:51:50Z

src/inspector_socket_server.cc

Typo... AFAIR, they might've had _Io suffix at some point to signify they are called on IO thread.

sam-github · 2017-05-30T23:53:04Z

src/inspector_socket_server.h

Am I missing something? It seems like most of the state is passed in the ctor, and then some is passed in Start(), and I don't see why, is it just historical? Is there some future use-case this is designed around?

This is the part I want to cleanup the most. Did a couple of half-hearted attempts but never achieved the desired result. It is historical (this code started as a fork of old Debug thread) and also because creation and start were separate at some point (this code was a part of the inpsector Agent).

sam-github · 2017-05-30T23:53:45Z

src/node.cc

Legacy... when all the debug code was deleted this function was left with its old pre-inspector name.

eugeneo · 2017-05-31T00:03:45Z

src/inspector_socket_server.h

This is the part I want to cleanup the most. Did a couple of half-hearted attempts but never achieved the desired result. It is historical (this code started as a fork of old Debug thread) and also because creation and start were separate at some point (this code was a part of the inpsector Agent).

eugeneo · 2017-05-31T00:04:57Z

src/inspector_socket_server.cc

Typo... AFAIR, they might've had _Io suffix at some point to signify they are called on IO thread.

eugeneo · 2017-05-31T00:07:56Z

src/inspector_socket.h

tcp/socket - both sound good. Ultimately, I believe this field name came from libuv API uv_accept that calls the connection socket "client" as opposed to "server" that is a listening socket.

eugeneo · 2017-05-31T00:08:54Z

src/inspector_socket.cc

This class was a plain C as it was mimicking libuv conventions. Need to become proper C++ at some point...

sam-github · 2017-05-31T17:41:19Z

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

@eugeneo @addaleax want to take another look? I backed out the statics.

addaleax · 2017-05-31T17:44:20Z

src/inspector_agent.h

Is this a typo/supposed to be are called by? (Also, I’d just say methods instead of APIs – at least to me a single API is usually a set of methods or similar things)

eugeneo

Thanks!

Pure refactor, makes no functional changes but the renaming helped me see more clearly what the relationship was between methods and variables. - Renamed methods to reduce number of slightly different names for the same thing ("thread" vs "io thread", etc.). - Added comments where it was useful to me.

sam-github · 2017-05-31T18:42:54Z

ci was all green, I rebased and fixed the typo @addaleax commented on

jasnell · 2017-06-01T22:14:56Z

New CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/8417/

Pure refactor, makes no functional changes but the renaming helped me see more clearly what the relationship was between methods and variables. * Renamed methods to reduce number of slightly different names for the same thing ("thread" vs "io thread", etc.). * Added comments where it was useful to me. PR-URL: #13321 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2017-06-01T23:05:27Z

Landed in 26ab769

sam-github · 2017-06-02T01:33:48Z

Thank you.

Pure refactor, makes no functional changes but the renaming helped me see more clearly what the relationship was between methods and variables. * Renamed methods to reduce number of slightly different names for the same thing ("thread" vs "io thread", etc.). * Added comments where it was useful to me. PR-URL: #13321 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

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 May 30, 2017

sam-github force-pushed the inspector-name-refactor branch from 76825ed to 76d70ed Compare May 30, 2017 23:46

addaleax reviewed May 30, 2017

View reviewed changes

eugeneo approved these changes May 30, 2017

View reviewed changes

sam-github commented May 30, 2017

View reviewed changes

sam-github added the dont-land-on-v6.x label May 30, 2017

sam-github force-pushed the inspector-name-refactor branch from 76d70ed to 10c93cf Compare May 30, 2017 23:58

eugeneo approved these changes May 31, 2017

View reviewed changes

sam-github force-pushed the inspector-name-refactor branch from 10c93cf to e36367d Compare May 31, 2017 17:16

addaleax approved these changes May 31, 2017

View reviewed changes

eugeneo approved these changes May 31, 2017

View reviewed changes

sam-github force-pushed the inspector-name-refactor branch from e36367d to 8acf9db Compare May 31, 2017 18:43

jasnell approved these changes Jun 1, 2017

View reviewed changes

jasnell closed this Jun 1, 2017

sam-github deleted the inspector-name-refactor branch June 2, 2017 01:33

jasnell mentioned this pull request Jun 5, 2017

8.1.0 Proposal #13483

Closed

Uh oh!

inspector: refactor to rename and comment methods #13321

inspector: refactor to rename and comment methods #13321

Uh oh!

Conversation

sam-github commented May 30, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eugeneo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eugeneo left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github commented May 31, 2017

Uh oh!

jasnell commented Jun 1, 2017

Uh oh!

jasnell commented Jun 1, 2017

Uh oh!

sam-github commented Jun 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants