Disabled remote-debugging by default and appshell.app.getRemoteDebuggingPort() needs a callback argument to work #668

g-217 · 2019-11-06T13:38:04Z

Disabling remote debugging by default, and it can be enabled by passing a command line arg as --remote-debugging-port=xxxx
Changed appshell.app.getRemoteDebuggingPort() implementation to require a callback
Added validation for port range, and also without remote-debugging-port we will not be able to debug brackets using a browser, and if browser debugging is disabled, appshell.app.getRemoteDebuggingPort(callback) will print 0

…ingPort() needs a callback argument to work

shubhsnov · 2019-11-07T20:37:41Z

appshell/cefclient.cpp

+  CefString debugger_port = command_line->GetSwitchValue("remote-debugging-port");
+  if (!debugger_port.empty()) {
+    int port = atoi(debugger_port.ToString().c_str());
+    static const int max_port_num = static_cast<int>(std::numeric_limits<uint16_t>::max());


What happens if the port is already in use?

I checked with python -m SimpleHTTPServer 9999 and Brackets snatches the port from Python HTTP Server.

shubhsnov · 2019-11-07T20:39:53Z

appshell/appshell_extensions.cpp

@@ -842,6 +842,8 @@ class ProcessMessageDelegate : public ClientHandler::ProcessMessageDelegate {
            uberDict->SetList(0, dirContents);
            uberDict->SetList(1, allStats);
            responseArgs->SetList(2, uberDict);
+        } else if (message_name == "GetRemoteDebuggingPort") {


We're changing the normal function to one that takes callback? Does this have any impact on existing usages? This is a breaking change, though I guess no extensions will ideally use this.

Yes, it does impact one use case, and fixed that with commit: adobe/brackets@8677756

shubhsnov · 2019-11-08T11:09:21Z

appshell/cefclient.cpp

+  CefString debugger_port = command_line->GetSwitchValue("remote-debugging-port");
+  if (!debugger_port.empty()) {
+    int port = atoi(debugger_port.ToString().c_str());
+    static const int max_port_num = static_cast<int>(std::numeric_limits<uint16_t>::max());


Can we get away without needing limits header?
I am not comfortable with the need for unsetting standard functions.
You can compute uint16 max as such, which will work for all platforms

uint16_t max_limit = 0; max_limit = ~max_limit;

std::numeric_limits<> is C++ equivalent of C macros denoting MAX and MIN of a default numeric data types. So, instead of std::numeric_limits<uint16_t>::max(), I could use UINT16_MAX defined in stdint.h

uint16 max_limit = 0; max_limit = ~max_limit; max_limit = max_limit >> 1

Bitwise gymnastics suggested above is unnecessary.

Don't bother about header, it's C++, not JS.

g-217 · 2019-11-11T06:16:55Z

Addressed all the review comments.

swmitra · 2019-11-11T09:36:09Z

@vickramdhawal Can you please take a quick look?

vickramdhawal · 2019-11-11T09:56:15Z

appshell/config.h

@@ -82,7 +82,7 @@

 #endif

-#define REMOTE_DEBUGGING_PORT 9234
+extern int g_remote_debugging_port;


why is this needed here ? use it in appshell_extensions.cpp

vickramdhawal · 2019-11-11T09:57:15Z

appshell/cefclient.cpp

@@ -19,6 +19,7 @@
 #include "config.h"

 CefRefPtr<ClientHandler> g_handler;
+int g_remote_debugging_port = 0;


~~please use unsigned int.~~

Since, we are going to use atom or strtol further, as suggested by you using int is better for comparisons.

vickramdhawal · 2019-11-11T09:58:27Z

appshell/cefclient.cpp

+    static const int max_port_num = 65535;
+    static const int max_reserved_port_num = 1024;


Use unsigned int.

Using strtol for integer parsing, so moved to signed long.

vickramdhawal · 2019-11-11T10:02:17Z

appshell/cefclient.cpp

-  settings.remote_debugging_port = REMOTE_DEBUGGING_PORT;
+  CefString debugger_port = command_line->GetSwitchValue("remote-debugging-port");
+  if (!debugger_port.empty()) {
+    int port = atoi(debugger_port.ToString().c_str());


using atoi seems acceptable in this case since the string is coming from the CommandLine handler. Better would be to use strtol.

Changed parsing to use strtol.

…e to use g_handler to hold remote-debugging-port as g_handler does not get initialized while we are parsing command line

g-217 · 2019-11-11T17:33:27Z

@vickramdhawal Addressed review comments. It is not possible to use g_handler to hold remote-debugging-port as g_handler does not get initialized while we are parsing command line.

vickramdhawal · 2019-11-12T04:39:13Z

appshell/cefclient.cpp

+        LOG(ERROR) << "Error while parsing remote-debugging-port arg: "<< debugger_port.ToString();
+        errno = 0;
+    }
+    static const long max_port_num = 65535;


all of this should then be in the else case if errno is not ERANGE, since we know the port was not parsed properly and comparisons further may not give desired results.

vickramdhawal

LGTM :+1

swmitra

LGTM.

…ingPort() needs a callback argument to work (adobe#668) * Disabled remote-debugging by default and appshell.app.getRemoteDebuggingPort() needs a callback argument to work * Addressing review comments * Addressing review comments * Updating error meesage for port validation * Addressing review comments from vickramdhawal, also it is not possible to use g_handler to hold remote-debugging-port as g_handler does not get initialized while we are parsing command line * More error message correction

Disabled remote-debugging by default and appshell.app.getRemoteDebugg…

2a172ef

…ingPort() needs a callback argument to work

g-217 mentioned this pull request Nov 7, 2019

Adding infobar for remote debugging enabled adobe/brackets#14956

Merged

shubhsnov reviewed Nov 7, 2019

View reviewed changes

narayani28 requested a review from swmitra November 8, 2019 09:00

shubhsnov reviewed Nov 8, 2019

View reviewed changes

g-217 added 2 commits November 11, 2019 11:29

Addressing review comments

76fc591

Addressing review comments

533c965

shubhsnov approved these changes Nov 11, 2019

View reviewed changes

Updating error meesage for port validation

436bdb2

swmitra requested a review from vickramdhawal November 11, 2019 09:35

vickramdhawal suggested changes Nov 11, 2019

View reviewed changes

Addressing review comments from vickramdhawal, also it is not possibl…

9b903fa

…e to use g_handler to hold remote-debugging-port as g_handler does not get initialized while we are parsing command line

vickramdhawal suggested changes Nov 12, 2019

View reviewed changes

More error message correction

de74633

vickramdhawal approved these changes Nov 12, 2019

View reviewed changes

swmitra approved these changes Nov 12, 2019

View reviewed changes

swmitra merged commit fd8fce7 into adobe:master Nov 12, 2019

g-217 mentioned this pull request Nov 18, 2019

Improved console error messages #669

Merged

This was referenced Nov 18, 2019

Merging changes to release branch for 1.14.1 patch release #671

Open

Cherry picking changes from master for patch release #674

Merged

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Adding infobar for remote debugging enabled brackets-archive/bracketsIssues#12059

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disabled remote-debugging by default and appshell.app.getRemoteDebuggingPort() needs a callback argument to work #668

Disabled remote-debugging by default and appshell.app.getRemoteDebuggingPort() needs a callback argument to work #668

g-217 commented Nov 6, 2019

shubhsnov Nov 7, 2019

g-217 Nov 8, 2019

shubhsnov Nov 7, 2019

g-217 Nov 10, 2019

shubhsnov Nov 8, 2019 •

edited

Loading

g-217 Nov 10, 2019 •

edited

Loading

g-217 Nov 10, 2019

g-217 commented Nov 11, 2019

swmitra commented Nov 11, 2019

vickramdhawal Nov 11, 2019

g-217 Nov 11, 2019

vickramdhawal Nov 11, 2019 •

edited

Loading

vickramdhawal Nov 11, 2019

vickramdhawal Nov 11, 2019

g-217 Nov 11, 2019

vickramdhawal Nov 11, 2019 •

edited

Loading

g-217 Nov 11, 2019

g-217 commented Nov 11, 2019 •

edited

Loading

vickramdhawal Nov 12, 2019

g-217 Nov 12, 2019

vickramdhawal left a comment

swmitra left a comment

		static const int max_port_num = 65535;
		static const int max_reserved_port_num = 1024;

Disabled remote-debugging by default and appshell.app.getRemoteDebuggingPort() needs a callback argument to work #668

Disabled remote-debugging by default and appshell.app.getRemoteDebuggingPort() needs a callback argument to work #668

Conversation

g-217 commented Nov 6, 2019

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

shubhsnov Nov 8, 2019 • edited Loading

Choose a reason for hiding this comment

g-217 Nov 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g-217 commented Nov 11, 2019

swmitra commented Nov 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vickramdhawal Nov 11, 2019 • edited Loading

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

vickramdhawal Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g-217 commented Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vickramdhawal left a comment

Choose a reason for hiding this comment

swmitra left a comment

Choose a reason for hiding this comment

shubhsnov Nov 8, 2019 •

edited

Loading

g-217 Nov 10, 2019 •

edited

Loading

vickramdhawal Nov 11, 2019 •

edited

Loading

vickramdhawal Nov 11, 2019 •

edited

Loading

g-217 commented Nov 11, 2019 •

edited

Loading