Skip to content
This repository has been archived by the owner on Sep 2, 2021. It is now read-only.

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

Merged
merged 6 commits into from
Nov 12, 2019
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2a172ef
Disabled remote-debugging by default and appshell.app.getRemoteDebugg…
g-217 Nov 6, 2019
76fc591
Addressing review comments
g-217 Nov 11, 2019
533c965
Addressing review comments
g-217 Nov 11, 2019
436bdb2
Updating error meesage for port validation
g-217 Nov 11, 2019
9b903fa
Addressing review comments from vickramdhawal, also it is not possibl…
g-217 Nov 11, 2019
de74633
More error message correction
g-217 Nov 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions appshell/appshell_extension_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ class AppShellExtensionHandler : public CefV8Handler {
CefString& exception) {

// The only messages that are handled here is getElapsedMilliseconds(),
// GetCurrentLanguage(), GetApplicationSupportDirectory(), and GetRemoteDebuggingPort().
// GetCurrentLanguage(), and GetApplicationSupportDirectory().
// All other messages are passed to the browser process.
if (name == "GetElapsedMilliseconds") {
retval = CefV8Value::CreateDouble(GetElapsedMilliseconds());
Expand All @@ -170,8 +170,6 @@ class AppShellExtensionHandler : public CefV8Handler {
retval = CefV8Value::CreateString(AppGetSupportDirectory());
} else if (name == "GetUserDocumentsDirectory") {
retval = CefV8Value::CreateString(AppGetDocumentsDirectory());
} else if (name == "GetRemoteDebuggingPort") {
retval = CefV8Value::CreateInt(REMOTE_DEBUGGING_PORT);
} else {
// Pass all messages to the browser process. Look in appshell_extensions.cpp for implementation.
CefRefPtr<CefBrowser> browser = CefV8Context::GetCurrentContext()->GetBrowser();
Expand Down
2 changes: 2 additions & 0 deletions appshell/appshell_extensions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

responseArgs->SetInt(2, g_remote_debugging_port);
}

else {
Expand Down
4 changes: 2 additions & 2 deletions appshell/appshell_extensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,8 +640,8 @@ if (!brackets) {
* @return int. The remote debugging port used by the appshell.
*/
native function GetRemoteDebuggingPort();
appshell.app.getRemoteDebuggingPort = function () {
return GetRemoteDebuggingPort();
appshell.app.getRemoteDebuggingPort = function (callback) {
GetRemoteDebuggingPort(callback || _dummyCallback);
};


Expand Down
17 changes: 16 additions & 1 deletion appshell/cefclient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "config.h"

CefRefPtr<ClientHandler> g_handler;
int g_remote_debugging_port = 0;
Copy link
Collaborator

@vickramdhawal vickramdhawal Nov 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

please use unsigned int.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


#ifdef OS_WIN
bool g_force_enable_acc = false;
Expand Down Expand Up @@ -95,7 +96,21 @@ void AppGetSettings(CefSettings& settings, CefRefPtr<CefCommandLine> command_lin
command_line->GetSwitchValue(client::switches::kJavascriptFlags);

// Enable dev tools
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());
Copy link
Collaborator

@vickramdhawal vickramdhawal Nov 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed parsing to use strtol.

static const int max_port_num = 65535;
static const int max_reserved_port_num = 1024;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use unsigned int.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if (port > max_reserved_port_num && port < max_port_num) {
g_remote_debugging_port = port;
settings.remote_debugging_port = port;
}
else {
LOG(ERROR) << "Could not enable Remote debugging on port: "<< port
<< ". Port number must be greater than "<< max_reserved_port_num
<< " and less than " << max_port_num << ".";
}
}

std::wstring versionStr = appshell::AppGetProductVersionString();

Expand Down
2 changes: 1 addition & 1 deletion appshell/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@

#endif

#define REMOTE_DEBUGGING_PORT 9234
extern int g_remote_debugging_port;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


// Comment out this line to enable OS themed drawing
#define DARK_UI
Expand Down