Skip to content

/ servo

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

Add support for launching devtools server on random port #25941

merged 1 commit into from Mar 16, 2020
+73 −9

Conversation

@kunalmohan
Copy link
Collaborator

kunalmohan commented Mar 10, 2020

In case the default port(6000) or the port specified by user for
devtools server is already taken, random port will be assigned to
it which is reported to the embedding layer for display to user.

r?@jdm @paulrouget

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #25907 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___
@jdm
Copy link
Member

jdm commented Mar 10, 2020

r? @paulrouget
@highfive highfive assigned paulrouget and unassigned jdm Mar 10, 2020
@paulrouget
paulrouget reviewed Mar 11, 2020
View changes
Copy link
Contributor

paulrouget left a comment

Thanks a lot! Almost there. Let me know if you need help with testing the Windows implementation.
components/devtools/lib.rs Outdated Show resolved Hide resolved
components/devtools/lib.rs Outdated Show resolved Hide resolved
components/embedder_traits/lib.rs Outdated Show resolved Hide resolved
support/hololens/ServoApp/ServoControl/Servo.cpp Outdated Show resolved Hide resolved
@kunalmohan kunalmohan requested a review from paulrouget Mar 11, 2020
@paulrouget
paulrouget reviewed Mar 13, 2020
View changes
components/config/opts.rs Show resolved Hide resolved
@paulrouget
paulrouget reviewed Mar 13, 2020
View changes
Copy link
Contributor

paulrouget left a comment

For now, let's leave out the Windows/Hololens code. I will implement a proper UI notification in another PR.
components/devtools/lib.rs Show resolved Hide resolved
ports/glutin/browser.rs Show resolved Hide resolved
ports/libmlservo/src/lib.rs Show resolved Hide resolved
ports/libsimpleservo/capi/src/lib.rs Outdated
debug!("Devtools Server running at {}", p);
(self.0.on_devtools_started)(p.into())
},
Err(()) => eprintln!("Error running devtools server"),

This comment has been minimized.

Sign in to view
Copy link
@paulrouget

paulrouget Mar 13, 2020

Contributor

Error can't be lost here, we need to let the embedder know that something didn't work. Maybe the callback on_devtools_started can take 2 arguments. A result, and a port.

Look at CPromptResult for example.

  #[repr(C)]
  pub enum CDevtoolsServerState {
    Started,
    Error,
  }
support/hololens/ServoApp/ServoControl/Servo.h Outdated Show resolved Hide resolved
support/hololens/ServoApp/ServoControl/Servo.cpp Outdated Show resolved Hide resolved
support/hololens/ServoApp/ServoControl/Servo.cpp Outdated Show resolved Hide resolved
ports/libsimpleservo/jniapi/src/lib.rs Show resolved Hide resolved
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented Mar 13, 2020

I hope this is what you expected to see with CDevtoolsServerState.
@paulrouget
paulrouget reviewed Mar 14, 2020
View changes
Copy link
Contributor

paulrouget left a comment

Thank you! It looks good. Please address the 2 minor comments, and then I think it's good to go.
ports/glutin/browser.rs Outdated
@@ -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 at {}", p),

This comment has been minimized.

Sign in to view
Copy link
@paulrouget

paulrouget Mar 14, 2020

Contributor

"running on port {}"
ports/libsimpleservo/capi/src/lib.rs Outdated
@@ -229,6 +229,7 @@ pub struct CHostCallbacks {
default: *const c_char,
trusted: bool,
) -> *const c_char,
pub on_devtools_started: extern "C" fn(port: c_uint, result: CDevtoolsServerState),

This comment has been minimized.

Sign in to view
Copy link
@paulrouget

paulrouget Mar 14, 2020

Contributor

Nit: result then port :)
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented Mar 14, 2020

Done!
@paulrouget
paulrouget reviewed Mar 16, 2020
View changes
ports/libmlservo/src/lib.rs Outdated

fn on_devtools_started(&self, port: Result<u16, ()>) {
match port {
Ok(p) => info!("Devtools Server running at {}", p),

This comment has been minimized.

Sign in to view
Copy link
@paulrouget

paulrouget Mar 16, 2020

Contributor

"running on port {}"
@paulrouget
paulrouget reviewed Mar 16, 2020
View changes
ports/libsimpleservo/capi/src/lib.rs Outdated
fn on_devtools_started(&self, port: Result<u16, ()>) {
match port {
Ok(p) => {
info!("Devtools Server running at {}", p);

This comment has been minimized.

Sign in to view
Copy link
@paulrouget

paulrouget Mar 16, 2020

Contributor

"running on port {}"
@paulrouget
paulrouget reviewed Mar 16, 2020
View changes
ports/libsimpleservo/jniapi/src/lib.rs Outdated

fn on_devtools_started(&self, port: Result<u16, ()>) {
match port {
Ok(p) => info!("Devtools Server running at {}", p),

This comment has been minimized.

Sign in to view
Copy link
@paulrouget

paulrouget Mar 16, 2020

Contributor

"running on port {}"
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented Mar 16, 2020

Sorry, I should have made those changes with the previous one.
@paulrouget
Copy link
Contributor

paulrouget commented Mar 16, 2020

@bors-servo r+

Once this lands, I will implement the UWP part.
@bors-servo
Copy link
Contributor

bors-servo commented Mar 16, 2020

📌 Commit b10dbc9 has been approved by paulrouget
@bors-servo
Copy link
Contributor

bors-servo commented Mar 16, 2020

⌛ Testing commit b10dbc9 with merge 10dd0ce...
bors-servo added a commit that referenced this pull request Mar 16, 2020
@bors-servo
Auto merge of #25941 - kunalmohan:25907-DevtoolsServer, r=paulrouget
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
10dd0ce 
Add support for launching devtools server on random port

In case the default port(6000) or the port specified by user for
devtools server is already taken, random port will be assigned to
it which is reported to the embedding layer for display to user.

r?@jdm @paulrouget

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25907  (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 16, 2020

💔 Test failed - status-taskcluster
@paulrouget
Copy link
Contributor

paulrouget commented Mar 16, 2020

Formatting issues:

git apply this:

diff --git a/support/hololens/ServoApp/ServoControl/Servo.cpp b/support/hololens/ServoApp/ServoControl/Servo.cpp
index ab48351248..d2d04c6fa3 100644
--- a/support/hololens/ServoApp/ServoControl/Servo.cpp
+++ b/support/hololens/ServoApp/ServoControl/Servo.cpp
@@ -87,7 +87,8 @@ const char *prompt_input(const char *message, const char *default,
   }
 }

-void on_devtools_started(Servo::DevtoolsServerState result, const unsigned int port) {
+void on_devtools_started(Servo::DevtoolsServerState result,
+                         const unsigned int port) {
   // FIXME
 }
@kunalmohan
Add support for launching devtools server on random port
Verified
This commit was signed with a verified signature.
kunalmohan Kunal Mohan
GPG key ID: 2B475A4524237BAC Learn about signing commits
Loading status checks…
94db0d6 
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.
@paulrouget
Copy link
Contributor

paulrouget commented Mar 16, 2020

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Mar 16, 2020

📌 Commit 94db0d6 has been approved by paulrouget
@bors-servo
Copy link
Contributor

bors-servo commented Mar 16, 2020

⌛ Testing commit 94db0d6 with merge 9d729db...
bors-servo added a commit that referenced this pull request Mar 16, 2020
@bors-servo
Auto merge of #25941 - kunalmohan:25907-DevtoolsServer, r=paulrouget
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
9d729db 
Add support for launching devtools server on random port

In case the default port(6000) or the port specified by user for
devtools server is already taken, random port will be assigned to
it which is reported to the embedding layer for display to user.

r?@jdm @paulrouget

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25907  (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 16, 2020

💔 Test failed - status-taskcluster
@paulrouget
Copy link
Contributor

paulrouget commented Mar 16, 2020

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Mar 16, 2020

⌛ Testing commit 94db0d6 with merge 58ff35f...
@paulrouget paulrouget mentioned this pull request Mar 16, 2020
Merged
@bors-servo
Copy link
Contributor

bors-servo commented Mar 16, 2020

☀️ Test successful - status-taskcluster
Approved by: paulrouget
Pushing 58ff35f to master...
@bors-servo bors-servo merged commit 58ff35f into servo:master Mar 16, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
@kunalmohan kunalmohan deleted the kunalmohan:25907-DevtoolsServer branch Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulrouget paulrouget

Assignees

@paulrouget paulrouget

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Support launching the devtools server on a random port
5 participants
@kunalmohan @jdm @paulrouget @bors-servo @highfive
You can’t perform that action at this time.