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

PerformanceResourceTiming: FetchStart #21258

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

PerformanceResourceTiming: FetchStart #21258

avadacatavra opened this issue Jul 27, 2018 · 13 comments

Comments

@avadacatavra
Copy link
Contributor

@avadacatavra avadacatavra commented Jul 27, 2018

In components/net/http_loader.rs::http_fetch we need to set fetch_start.

On getting, the fetchStart attribute MUST return as follows:

  1. The time immediately before the user agent starts to fetch the final resource in the redirection, if there are HTTP redirects or equivalent.
  2. The time immediately before the user agent starts to fetch the resource otherwise.

https://w3c.github.io/resource-timing/#dom-performanceresourcetiming-fetchstart

@aditj
Copy link
Contributor

@aditj aditj commented Jan 8, 2019

Hey ! Can I take up on this? :D

@jdm
Copy link
Member

@jdm jdm commented Jan 8, 2019

Sure! Please ask questions if anything is unclear.

@ferjm ferjm added the C-assigned label Jan 9, 2019
@aditj
Copy link
Contributor

@aditj aditj commented Jan 9, 2019

Sure! Please ask questions if anything is unclear.

Actually I do have a couple of questions ,
a) So I call the FetchStart function here and I define it here ?
b) Do I set the fetch_start of a PeformanceResourceTiming object already in the http_fetch file , if so which object? (If there is a structure having PerformanceResourceTiming as one of its fields(whose fetch_start has to be in turn set ) then please tell which one .) :)

@aditj
Copy link
Contributor

@aditj aditj commented Jan 10, 2019

I dug into the code a little bit more and I found the Response being returned by http_fetch has a ResourceFetchTiming entry (which in turn has no of redirects , request_start ) , so maybe this is where I get the values to be set from . But I am still unclear about which PerformanceResourceTiming's fetch_start i have to set ? ( Is it something in the cache , or in some other structure )

@aditj
Copy link
Contributor

@aditj aditj commented Jan 14, 2019

Can I implement fetch_start in ResourceFetchTiming?
And is there a way to link RFT and PerformanceResourceTimings?

@aditj
Copy link
Contributor

@aditj aditj commented Jan 14, 2019

Hey! How do I know whether the Resource being fetched is the last one in the http_fetch ? Help please @jdm 😓

@jdm
Copy link
Member

@jdm jdm commented Jan 14, 2019

Sorry about the lack of response; I'll try to answer your questions in reverse order:

How do I know whether the Resource being fetched is the last one in the http_fetch

A resource being fetched is the last one if there is no redirection. You can see this information being set in substep 2-3 and substep 4.

Can I implement fetch_start in ResourceFetchTiming? And is there a way to link RFT and PerformanceResourceTimings?

Yes. They are linked via the PerformanceResourcetiming::from_resource_timing method, which creates a new PerformanceResourceTiming object and initializes it from the members of ResourceFetchTiming.

As you seem to have found, the timing data is collected in ResourceFetchTiming objects in the net crate (which doesn't know anything about PerformanceResourceTiming), then the final timings are turned into DOM values in PerformanceResourceTiming objects in the script crate. Does that help?

@aditj
Copy link
Contributor

@aditj aditj commented Jan 15, 2019

On getting, the fetchStart attribute MUST return as follows:

  1. The time immediately before the user agent starts to fetch the final resource in the redirection, if there are HTTP redirects or equivalent.
  2. The time immediately before the user agent starts to fetch the resource otherwise.

But @jdm the specs say "the final resource in the redirection" , if there is no redirection then whats the point of mentioning it.

@aditj
Copy link
Contributor

@aditj aditj commented Jan 15, 2019

As you seem to have found, the timing data is collected in ResourceFetchTiming objects in the net crate (which doesn't know anything about PerformanceResourceTiming), then the final timings are turned into DOM values in PerformanceResourceTiming objects in the script crate. Does that help?

I do have 3 questions @jdm 😅 :
a) I create a PerformanceResourceTiming Object , but what do i do with it ? Do I submit timings using submit_timing in network_listener ?
b) Is there a need to set up a network_listener if I am submitting the timings using submit_timing ?
c)Also how do I get other parameters required for from_resource_timing method (namely url, initiator_type and next_hop protocol)

@jdm
Copy link
Member

@jdm jdm commented Jan 15, 2019

On getting, the fetchStart attribute MUST return as follows:

  1. The time immediately before the user agent starts to fetch the final resource in the redirection, if there are HTTP redirects or equivalent.
  2. The time immediately before the user agent starts to fetch the resource otherwise.

But @jdm the specs say "the final resource in the redirection" , if there is no redirection then whats the point of mentioning it.

If there is no location header, the request is the final resource. We will need to store the time before starting the request, then check if there's a location header when the initial response is received; if there is no location header then that we can store the earlier time as the FetchStart value.

@jdm
Copy link
Member

@jdm jdm commented Jan 15, 2019

As you seem to have found, the timing data is collected in ResourceFetchTiming objects in the net crate (which doesn't know anything about PerformanceResourceTiming), then the final timings are turned into DOM values in PerformanceResourceTiming objects in the script crate. Does that help?

I do have 3 questions @jdm 😅 :
a) I create a PerformanceResourceTiming Object , but what do i do with it ? Do I submit timings using submit_timing in network_listener ?
b) Is there a need to set up a network_listener if I am submitting the timings using submit_timing ?
c)Also how do I get other parameters required for from_resource_timing method (namely url, initiator_type and next_hop protocol)

You should not need to write code that creates a PerformanceResourceTiming object. That already happens in this code which is called from every implementor of this trait, which is invoked automatically in this code.

@aditj
Copy link
Contributor

@aditj aditj commented Jan 16, 2019

@jdm I think I was over complicating the issue , anyways are there existing tests for fetch_start ?
And should I open a pull request for the same (without the tests that is)?

@jdm
Copy link
Member

@jdm jdm commented Jan 16, 2019

You can try running ./mach test-wpt tests/wpt/web-platform-tests/resource-timing and see if any test results change. There are a bunch of tests in that directory that make use of it.

@aditj aditj mentioned this issue Jan 17, 2019
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Jan 21, 2019
Added fetch_start functionality

<!-- Please describe your changes on the following line: -->
Added the fetch_start functionality in http_loader.rs (http_fetch function)

---
<!-- 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 #21258 (GitHub issue number if applicable)

<!-- 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/22714)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jan 28, 2019
Added fetch_start functionality

<!-- Please describe your changes on the following line: -->
Added the fetch_start functionality in http_loader.rs (http_fetch function)

---
<!-- 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 #21258 (GitHub issue number if applicable)

<!-- 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/22714)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jan 28, 2019
Added fetch_start functionality

<!-- Please describe your changes on the following line: -->
Added the fetch_start functionality in http_loader.rs (http_fetch function)

---
<!-- 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 #21258 (GitHub issue number if applicable)

<!-- 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/22714)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jan 28, 2019
Added fetch_start functionality

<!-- Please describe your changes on the following line: -->
Added the fetch_start functionality in http_loader.rs (http_fetch function)

---
<!-- 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 #21258 (GitHub issue number if applicable)

<!-- 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/22714)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jan 28, 2019
Added fetch_start functionality

<!-- Please describe your changes on the following line: -->
Added the fetch_start functionality in http_loader.rs (http_fetch function)

---
<!-- 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 #21258 (GitHub issue number if applicable)

<!-- 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/22714)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jan 28, 2019
Added fetch_start functionality

<!-- Please describe your changes on the following line: -->
Added the fetch_start functionality in http_loader.rs (http_fetch function)

---
<!-- 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 #21258 (GitHub issue number if applicable)

<!-- 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/22714)
<!-- 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.

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