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

Jump to bottom

PerformanceResourceTiming: connectStart #21261

Closed
avadacatavra opened this issue Jul 27, 2018 · 9 comments
Closed

PerformanceResourceTiming: connectStart #21261

avadacatavra opened this issue Jul 27, 2018 · 9 comments
Labels
A-perf-measurement C-assigned E-easy

Comments

@avadacatavra
Copy link
Contributor

@avadacatavra avadacatavra commented Jul 27, 2018

If a persistent connection is used, connect_start will be set in components/net/http_loader.rs::http_fetch, otherwise, it will be in components/net/http_loader.rs::obtain_response

On getting, the connectStart attribute MUST return as follows:

  1. The same value as fetchStart, if a persistent connection [RFC7230] is used or the resource is retrieved from relevant application caches or local resources.
  2. The time immediately before the user agent start establishing the connection to the server to retrieve the resource, if the last non-redirected fetch of the resource passes the timing allow check algorithm. If the transport connection fails and the user agent reopens a connection, connectStart SHOULD return the corresponding value of the new connection.
  3. zero, otherwise.

https://w3c.github.io/resource-timing/#dom-performanceresourcetiming-connectstart
@avadacatavra avadacatavra mentioned this issue Jul 27, 2018
Open
12 of 19 tasks complete
@highfive
Copy link

@highfive highfive commented Nov 29, 2018

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄
@VishalCR7
Copy link

@VishalCR7 VishalCR7 commented Jan 27, 2019

@highfive: assign me
@highfive highfive added the C-assigned label Jan 27, 2019
@highfive
Copy link

@highfive highfive commented Jan 27, 2019

Hey @VishalCR7! Thanks for your interest in working on this issue. It's now assigned to you!
@VishalCR7
Copy link

@VishalCR7 VishalCR7 commented Jan 27, 2019

@jdm : Hey! I m just getting started. Can you please explain what needs to be done and which files to look at for solving this bug. Thanks in advance :)
@jdm
Copy link
Member

@jdm jdm commented Jan 31, 2019

Since I don't believe we can determine if a persistent connection is used or not right now, we should set connectStart in the obtain_response function in components/net/http_loader.rs, right before calling client.request.
@jdm jdm removed the C-assigned label Mar 10, 2019
@nehalem501
Copy link
Contributor

@nehalem501 nehalem501 commented May 3, 2019

@highfive: assign me
@highfive highfive added the C-assigned label May 3, 2019
@highfive
Copy link

@highfive highfive commented May 3, 2019

Hey @nehalem501! Thanks for your interest in working on this issue. It's now assigned to you!
@nehalem501 nehalem501 mentioned this issue May 5, 2019
Merged
4 of 5 tasks complete
bors-servo added a commit that referenced this issue May 6, 2019
@bors-servo
Auto merge of #23323 - nehalem501:master, r=jdm
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…
b0d00a8 
Implement connectStart in PerformanceResourceTiming

<!-- Please describe your changes on the following line: -->
- Added connectStart where needed (```ResourceFetchTiming```, ```ResourceAttribute```) in ```components/net_traits/lib.rs ```
- Before calling ```client.request``` in ```obtain_response```, we now add the ```connectStart``` attribute (```components/net/http_loader.rs```)
- Updated tests to reflect those changes

---
<!-- 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 #21261 (at least partially)

<!-- Either: -->
- [X] 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23323)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 17, 2019
@bors-servo
Auto merge of #23323 - nehalem501:master, r=jdm
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…
208d6cb 
Implement connectStart in PerformanceResourceTiming

<!-- Please describe your changes on the following line: -->
- Added connectStart where needed (```ResourceFetchTiming```, ```ResourceAttribute```) in ```components/net_traits/lib.rs ```
- Before calling ```client.request``` in ```obtain_response```, we now add the ```connectStart``` attribute (```components/net/http_loader.rs```)
- Updated tests to reflect those changes

---
<!-- 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 #21261 (at least partially)

<!-- Either: -->
- [X] 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23323)
<!-- Reviewable:end -->
@atouchet
Copy link
Contributor

@atouchet atouchet commented Jun 30, 2019

In #23323 it says that this was only fixed partially. Is there more to do here?
@jdm
Copy link
Member

@jdm jdm commented Jun 30, 2019

Support for persistent connections doesn't exist yet, but we should file a new issue about that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-perf-measurement C-assigned E-easy
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Implement connectStart in PerformanceResourceTiming
6 participants
@jdm @nehalem501 @highfive @avadacatavra @VishalCR7 @atouchet
You can’t perform that action at this time.