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

Add support for launching devtools server on random port #25941

Merged
merged 1 commit into from Mar 16, 2020
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Add support for launching devtools server on random port

Assign random port to devtools server in case user does not specify a
port explicitly and report it to the embedding layer for display to user.
  • Loading branch information
kunalmohan committed Mar 16, 2020
commit 94db0d61cb6a92e36b2047ec6c643b3fa4734789
@@ -653,12 +653,7 @@ pub fn from_cmdline_args(mut opts: Options, args: &[String]) -> ArgumentParsingR
"Start remote debugger server on port",
"2794",
);
opts.optflagopt(
"",
"devtools",
"Start remote devtools server on port",
"6000",
);
opts.optflagopt("", "devtools", "Start remote devtools server on port", "0");
opts.optflagopt(
"",
"webdriver",
@@ -886,7 +881,8 @@ pub fn from_cmdline_args(mut opts: Options, args: &[String]) -> ArgumentParsingR
})
});

let devtools_port = opt_match.opt_default("devtools", "6000").map(|port| {
// Set default port 0 for a random port to be selected.
let devtools_port = opt_match.opt_default("devtools", "0").map(|port| {
This conversation was marked as resolved by kunalmohan

This comment has been minimized.

Copy link
@paulrouget

paulrouget Mar 13, 2020

Contributor

Please add a comment mentioning that 0 mean random port.

This comment has been minimized.

Copy link
@kunalmohan

kunalmohan Mar 13, 2020

Author Collaborator

Done!

port.parse()
.unwrap_or_else(|err| args_fail(&format!("Error parsing option: --devtools ({})", err)))
});
@@ -154,7 +154,20 @@ fn run_server(
port: u16,
embedder: EmbedderProxy,
) {
let listener = TcpListener::bind(&("0.0.0.0", port)).unwrap();
let bound = TcpListener::bind(&("0.0.0.0", port)).ok().and_then(|l| {
l.local_addr()
.map(|addr| addr.port())
.ok()
.map(|port| (l, port))
});

let port = bound.as_ref().map(|(_, port)| *port).ok_or(());
embedder.send((None, EmbedderMsg::OnDevtoolsStarted(port)));

let listener = match bound {
Some((l, _)) => l,
None => return,
};
This conversation was marked as resolved by kunalmohan

This comment has been minimized.

Copy link
@paulrouget

paulrouget Mar 13, 2020

Contributor

This works, but maybe we could do something a bit more "rusty", like so:

let bound = TcpListener::bind(&("0.0.0.0", port)).ok().and_then(|l|
    l.local_addr().map(|addr| addr.port()).ok().map(|port| (l, port))
);

let port = bound.as_ref().map(|(_, port)| *port).ok_or(());
embedder.send((None, EmbedderMsg::OnDevtoolsStarted(port)));

let listener = match bound {
    Some((l, _)) => l,
    None => return,
};

let mut registry = ActorRegistry::new();

@@ -198,6 +198,8 @@ pub enum EmbedderMsg {
/// Notifies the embedder about media session events
/// (i.e. when there is metadata for the active media session, playback state changes...).
MediaSessionEvent(MediaSessionEvent),
/// Report the status of Devtools Server
OnDevtoolsStarted(Result<u16, ()>),
}

impl Debug for EmbedderMsg {
@@ -232,6 +234,7 @@ impl Debug for EmbedderMsg {
EmbedderMsg::BrowserCreated(..) => write!(f, "BrowserCreated"),
EmbedderMsg::ReportProfile(..) => write!(f, "ReportProfile"),
EmbedderMsg::MediaSessionEvent(..) => write!(f, "MediaSessionEvent"),
EmbedderMsg::OnDevtoolsStarted(..) => write!(f, "OnDevtoolsStarted"),
}
}
}
@@ -515,6 +515,12 @@ where
debug!("MediaSessionEvent received");
// TODO(ferjm): MediaSession support for Glutin based browsers.
},
EmbedderMsg::OnDevtoolsStarted(port) => {
match port {
Ok(p) => info!("Devtools Server running on port {}", p),
Err(()) => error!("Error running devtools server"),
}
},
This conversation was marked as resolved by kunalmohan

This comment has been minimized.

Copy link
@paulrouget

paulrouget Mar 13, 2020

Contributor

Use info! and error! instead of debug!.

}
}
}
@@ -423,6 +423,13 @@ impl HostTrait for HostCallbacks {
}

fn set_clipboard_contents(&self, _contents: String) {}

fn on_devtools_started(&self, port: Result<u16, ()>) {
match port {
Ok(p) => info!("Devtools Server running on port {}", p),
Err(()) => error!("Error running Devtools server"),
}
}
This conversation was marked as resolved by kunalmohan

This comment has been minimized.

Copy link
@paulrouget

paulrouget Mar 13, 2020

Contributor

Same.

}

pub struct ServoInstance {
@@ -146,6 +146,8 @@ pub trait HostTrait {
fn on_media_session_playback_state_change(&self, state: MediaSessionPlaybackState);
/// Called when the media session position state is set.
fn on_media_session_set_position_state(&self, duration: f64, position: f64, playback_rate: f64);
/// Called when devtools server is started
fn on_devtools_started(&self, port: Result<u16, ()>);
}

pub struct ServoGlue {
@@ -670,6 +672,9 @@ impl ServoGlue {
),
};
},
EmbedderMsg::OnDevtoolsStarted(port) => {
self.callbacks.host_callbacks.on_devtools_started(port);
},
EmbedderMsg::Status(..) |
EmbedderMsg::SelectFiles(..) |
EmbedderMsg::MoveTo(..) |
@@ -23,7 +23,7 @@ use simpleservo::{
use std::ffi::{CStr, CString};
#[cfg(target_os = "windows")]
use std::mem;
use std::os::raw::{c_char, c_void};
use std::os::raw::{c_char, c_uint, c_void};
use std::panic::{self, UnwindSafe};
use std::slice;
use std::str::FromStr;
@@ -229,6 +229,7 @@ pub struct CHostCallbacks {
default: *const c_char,
trusted: bool,
) -> *const c_char,
pub on_devtools_started: extern "C" fn(result: CDevtoolsServerState, port: c_uint),
}

/// Servo options
@@ -286,6 +287,12 @@ pub enum CMediaSessionPlaybackState {
Paused,
}

#[repr(C)]
pub enum CDevtoolsServerState {
Started,
Error,
}

impl From<MediaSessionPlaybackState> for CMediaSessionPlaybackState {
fn from(state: MediaSessionPlaybackState) -> Self {
match state {
@@ -854,4 +861,17 @@ impl HostTrait for HostCallbacks {
let contents_str = c_str.to_str().expect("Can't create str");
Some(contents_str.to_owned())
}

fn on_devtools_started(&self, port: Result<u16, ()>) {
match port {
Ok(p) => {
info!("Devtools Server running on port {}", p);
(self.0.on_devtools_started)(CDevtoolsServerState::Started, p.into());
},
Err(()) => {
error!("Error running devtools server");
(self.0.on_devtools_started)(CDevtoolsServerState::Error, 0);
},
}
}
}
@@ -603,6 +603,13 @@ impl HostTrait for HostCallbacks {
)
.unwrap();
}

fn on_devtools_started(&self, port: Result<u16, ()>) {
match port {
Ok(p) => info!("Devtools Server running on port {}", p),
Err(()) => error!("Error running devtools server"),
}
}
This conversation was marked as resolved by kunalmohan

This comment has been minimized.

Copy link
@paulrouget

paulrouget Mar 13, 2020

Contributor

info/error.

}

fn initialize_android_glue(env: &JNIEnv, activity: JObject) {
@@ -87,6 +87,11 @@ const char *prompt_input(const char *message, const char *default,
}
}

void on_devtools_started(Servo::DevtoolsServerState result,
const unsigned int port) {
// FIXME
}

Servo::Servo(hstring url, hstring args, GLsizei width, GLsizei height,
float dpi, ServoDelegate &aDelegate)
: mWindowHeight(height), mWindowWidth(width), mDelegate(aDelegate) {
@@ -147,6 +152,7 @@ Servo::Servo(hstring url, hstring args, GLsizei width, GLsizei height,
c.prompt_ok_cancel = &prompt_ok_cancel;
c.prompt_yes_no = &prompt_yes_no;
c.prompt_input = &prompt_input;
c.on_devtools_started = &on_devtools_started;

capi::register_panic_handler(&on_panic);

@@ -31,6 +31,7 @@ class Servo {
typedef capi::CPromptResult PromptResult;
typedef capi::CMediaSessionActionType MediaSessionActionType;
typedef capi::CMediaSessionPlaybackState MediaSessionPlaybackState;
typedef capi::CDevtoolsServerState DevtoolsServerState;

void PerformUpdates() { capi::perform_updates(); }
void DeInit() { capi::deinit(); }
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.