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

@@ -27,20 +27,36 @@ using v8_inspector::StringView;
 template<typename Transport>
 using TransportAndIo = std::pair<Transport*, InspectorIo*>;

-std::string GetProcessTitle() {
-  // uv_get_process_title will trim the title if it is too long.
+static std::string GetProcessTitle() {


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

@@ -39,6 +39,7 @@ class InspectorSessionDelegate {
 public:
  virtual ~InspectorSessionDelegate() = default;
  virtual bool WaitForFrontendMessage() = 0;
+  // XXX SendMessageToFrontend() ?


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

@@ -39,6 +39,7 @@ class InspectorSessionDelegate {
 public:
  virtual ~InspectorSessionDelegate() = default;
  virtual bool WaitForFrontendMessage() = 0;
+  // XXX SendMessageToFrontend() ?


I think this rename would make meaning more clear.

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

src/inspector_socket.cc

@@ -559,21 +559,22 @@ static void init_handshake(InspectorSocket* inspector) {
  settings->on_url = path_cb;
 }

-int inspector_accept(uv_stream_t* server, InspectorSocket* inspector,
+// XXX(sam) shouldn't all these snake_case functions be CamelCase?


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

@@ -58,7 +59,7 @@ class InspectorSocket {
  struct http_parsing_state_s* http_parsing_state;
  struct ws_state_s* ws_state;
  std::vector<char> buffer;
-  uv_tcp_t client;
+  uv_tcp_t client;  // XXX(sam) this is a server, why is it called "client"?


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

@@ -230,6 +242,7 @@ class SocketSession {

 private:
  enum class State { kHttp, kWebSocket, kClosing, kEOF, kDeclined };
+  // XXX(sam) why do these two methods have a trailing _? that's for properties


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

 class InspectorSocketServer {
 public:
  using ServerCallback = void (*)(InspectorSocketServer*);
  InspectorSocketServer(SocketServerDelegate* delegate,
                        const std::string& host,
                        int port,
                        FILE* out = stderr);
+  // XXX(sam) why is loop not passed in ctor? Its fixed and known at that time.


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

@@ -4010,8 +4010,8 @@ static void ParseArgs(int* argc,
 }


-static void StartDebug(Environment* env, const char* path,
-                       DebugOptions debug_options) {
+static void StartInspector(Environment* env, const char* path,


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

 class InspectorSocketServer {
 public:
  using ServerCallback = void (*)(InspectorSocketServer*);
  InspectorSocketServer(SocketServerDelegate* delegate,
                        const std::string& host,
                        int port,
                        FILE* out = stderr);
+  // XXX(sam) why is loop not passed in ctor? Its fixed and known at that time.


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

@@ -230,6 +242,7 @@ class SocketSession {

 private:
  enum class State { kHttp, kWebSocket, kClosing, kEOF, kDeclined };
+  // XXX(sam) why do these two methods have a trailing _? that's for properties


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

@@ -58,7 +59,7 @@ class InspectorSocket {
  struct http_parsing_state_s* http_parsing_state;
  struct ws_state_s* ws_state;
  std::vector<char> buffer;
-  uv_tcp_t client;
+  uv_tcp_t client;  // XXX(sam) this is a server, why is it called "client"?


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

@@ -559,21 +559,22 @@ static void init_handshake(InspectorSocket* inspector) {
  settings->on_url = path_cb;
 }

-int inspector_accept(uv_stream_t* server, InspectorSocket* inspector,
+// XXX(sam) shouldn't all these snake_case functions be CamelCase?


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

  void WaitForDisconnect();
  void FatalException(v8::Local<v8::Value> error,
                      v8::Local<v8::Message> message);
+
+  // These three APIs called by the WS protocol and JS binding to create


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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inspector: refactor to rename and comment methods #13321

inspector: refactor to rename and comment methods #13321

sam-github commented May 30, 2017

addaleax May 30, 2017

sam-github May 31, 2017

eugeneo left a comment

eugeneo May 30, 2017

sam-github May 31, 2017

eugeneo May 31, 2017

sam-github May 31, 2017

sam-github left a comment

sam-github May 30, 2017

sam-github May 30, 2017

eugeneo May 31, 2017

sam-github May 30, 2017

eugeneo May 31, 2017

sam-github May 30, 2017

eugeneo May 31, 2017

sam-github May 30, 2017

eugeneo May 31, 2017

sam-github May 30, 2017

eugeneo May 31, 2017

eugeneo May 31, 2017

eugeneo May 31, 2017

eugeneo May 31, 2017

sam-github commented May 31, 2017

addaleax May 31, 2017

eugeneo left a comment

sam-github commented May 31, 2017

jasnell commented Jun 1, 2017

jasnell commented Jun 1, 2017

sam-github commented Jun 2, 2017

inspector: refactor to rename and comment methods #13321

inspector: refactor to rename and comment methods #13321

Conversation

sam-github commented May 30, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented May 31, 2017

Choose a reason for hiding this comment

eugeneo left a comment

Choose a reason for hiding this comment

sam-github commented May 31, 2017

jasnell commented Jun 1, 2017

jasnell commented Jun 1, 2017

sam-github commented Jun 2, 2017