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

Improve network monitor integration #11773

Closed
jdm opened this issue Jun 17, 2016 · 4 comments
Closed

Improve network monitor integration #11773

jdm opened this issue Jun 17, 2016 · 4 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Jun 17, 2016

#11593 made the network monitor appear when using the Firefox developer tools with Servo. There are some known problems with the output that it shows, however:

  • we do not report requests that are redirected (like visiting https://reddit.com and getting redirected to https://www.reddit.com)
  • the response panel is empty
  • we do not report timing data for "waiting" and "receiving"

I'm not sure how to solve the problem with the response panel yet, but the others will require modifying the code in http_loader.rs to send more notifications to the developer tools server. For the missing requests, we should be creating a new request ID each time and sending the information about the request and response sooner. For the missing timing data, we will need to time how long receiving the full HTTP response body takes and send a message to the developer tools that reports it.

Code: components/net/http_loader.rs, components/devtools_traits/lib.rs, components/devtools/lib.rs, components/devtools/actors/network_event.rs

cc @mrmiywj This might be interesting to you.

@highfive
Copy link

@highfive highfive commented Jun 17, 2016

@mrmiywj
Copy link
Contributor

@mrmiywj mrmiywj commented Jun 17, 2016

Thanks!
It looks attractive to me.

@mrmiywj
Copy link
Contributor

@mrmiywj mrmiywj commented Jun 27, 2016

@jdm
Recently I'm working on this issue. But I still have some questions.
You said that

For the missing requests, we should be creating a new request ID each time and sending the information about the request and response sooner.

But actually, I found that components/net/http_loader.rs L967 every time the browser sends out a request, it will also generate a (msg, response). What's the purpose of creating a new request ID you said? Just send them to the devtools is not enough?

@jdm
Copy link
Member Author

@jdm jdm commented Jun 27, 2016

@mrmiywj Note that we don't send the message to the devtools until https://github.com/servo/servo/blob/master/components/net/http_loader.rs#L1061 . In between there are several places where we restart the loop and never send the message. The purpose of creating a new request ID would be to ensure that each message the devtools receives for a separate request gets a separate ID.

@mrmiywj mrmiywj mentioned this issue Jun 28, 2016
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Jul 27, 2016
send requests that are redirected to devtools

<!-- 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 #11773  (github issue number if applicable).

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

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11889)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.