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: startTime #21254

Closed
avadacatavra opened this issue Jul 27, 2018 · 32 comments · Fixed by #24508
Closed

PerformanceResourceTiming: startTime #21254

avadacatavra opened this issue Jul 27, 2018 · 32 comments · Fixed by #24508
Labels
A-perf-measurement C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor.

Comments

@avadacatavra
Copy link
Contributor

In components/net/http_loader.rs::http_fetch we need to set start_time for the PerformanceEntry member of PerformanceResourceTiming. This will be equal to either fetch_start or redirect_start.

Spec: https://w3c.github.io/resource-timing/#dfn-starttime

@highfive
Copy link

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. 😄

@chirgjn
Copy link

chirgjn commented Nov 29, 2018

Hey @avadacatavra 👋
Came here from your tweet, would love to take this up. But I'm fairly new to rust, I've got plenty of time this week though. Should I assign it to myself?

@chirgjn
Copy link

chirgjn commented Nov 29, 2018

@highfive: assign me

Taking it up for now :D

@highfive
Copy link

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

@highfive highfive added the C-assigned There is someone working on resolving the issue label Nov 29, 2018
@avadacatavra
Copy link
Contributor Author

Hi @chirgjn--thanks for taking this on!

Let me know if there's any help you need

@chirgjn
Copy link

chirgjn commented Dec 3, 2018

@avadacatavra thanks. I'm able to build servo on my machine but apart from that I'm having a hard time.
I've read through the comments and the spec, and what I understand is that first need to find out the fetch_start and redirect_start right?
Can you point me where to look?

@jdm
Copy link
Member

jdm commented Jan 7, 2019

@chirgjn Since this attribute relies on some other attributes that aren't implemented yet (like #21258) it probably makes more sense to start with those ones.

@jdm jdm added S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. and removed C-assigned There is someone working on resolving the issue labels Jan 7, 2019
@aditj
Copy link
Contributor

aditj commented Jan 29, 2019

@jdm Since FetchStart is now set up, Can I work on this? 😄
(Note: startTime can be set to either FetchStart or RedirectStart as mentioned in the specs).

@chirgjn
Copy link

chirgjn commented Jan 29, 2019

Hi @aditj
Just went through your PR for #21258
Great work 👏
This is my first servo issue though and I would like to work complete it
Maybe we can collaborate?
Is that fine with you @jdm ?

@aditj
Copy link
Contributor

aditj commented Jan 29, 2019

@chirgjn :Thanks 😄 and you can take it up if it can actually be done(i.e. to say there are no other dependencies left :P)
Also i dont have a problem in collaborating but I dont know how that is gonna work :/

@chirgjn
Copy link

chirgjn commented Jan 29, 2019

@aditj I'll take it up then.
This is currently blocked on #21256 I think which will be resolved after #22319 gets merged after that I don't think there are any blockers.

@jdm
Copy link
Member

jdm commented Jan 29, 2019

I suggest that we add an API to ResourceFetchTiming called set_attribute_from that takes two ResourceAttribute arguments and sets the first attribute based on the value of the second attribute.

@aditj
Copy link
Contributor

aditj commented Jan 29, 2019

@jdm should we file a seperate issue for that ?
And if no can I create a new pull request with this API?
(and @chirgjn can maybe implement it for startTime)

@jdm
Copy link
Member

jdm commented Jan 29, 2019

I was thinking it could be added as part of fixing this issue.

@davidweitzenfeld
Copy link

@highfive: assign me

@highfive
Copy link

highfive commented Mar 6, 2019

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

@highfive highfive added the C-assigned There is someone working on resolving the issue label Mar 6, 2019
@jdm
Copy link
Member

jdm commented Mar 22, 2019

@davidweitzenfeld It looks like you made a pull request against your fork, not servo/servo :)

@jdm
Copy link
Member

jdm commented Apr 22, 2019

@davidweitzenfeld It looks like you made progress but never returned to make the PR against the right repository. Are you still planning to complete this work?

@jdm jdm removed the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Apr 22, 2019
@jdm jdm removed the C-assigned There is someone working on resolving the issue label May 26, 2019
@jdm
Copy link
Member

jdm commented May 26, 2019

Unassigning due to lack of response.

@pag4k
Copy link

pag4k commented May 29, 2019

Hello! This sounds like a perfect first issue for me. I some Rust experience, but I never contributed to such a project.

If it is possible, then: @highfive: assign me :-)

@highfive highfive added the C-assigned There is someone working on resolving the issue label May 29, 2019
@highfive
Copy link

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

@pag4k
Copy link

pag4k commented Jun 2, 2019

Hello!
I have a two small questions about this part of the specification: If there are HTTP redirects or equivalent when fetching the resource, and if the timing allow check algorithm passes, this attribute MUST return the same value as redirectStart. Otherwise, this attribute MUST return the same value as fetchStart.

  1. Would it be fine to first set startTime to fetchStart when the latter is set (here) and then only changing it to redirectStart later if all the above conditions are respected?

  2. Speaking of the conditions, looking at http_loader.rs, it seems that the timing allow check is not implemented yet. Is there something I am missing?

Thank you!

@jdm
Copy link
Member

jdm commented Jun 2, 2019

  1. Yes, that sounds fine.
  2. No, that's PerformanceResourceTiming: TimingAllowCheck #21270.

@pag4k
Copy link

pag4k commented Jun 2, 2019

More questions!
This time about performanceresourcetiming.rs.
Two things that are probably related:

  1. I'm not too sure how to deal with start_time in new_inherited(). Should it also be set to the fetch_start that is passed in argument? In a previous version of the file, it used to be, but since it is not certain they will be the same value, I'm not sure.
  2. It seems that the //TODO fetch start should be in RFT above from_resource_timing() is no longer relevant since we have fetch_start: resource_timing.fetch_start as f64, since this.
    Thank you!

@pag4k
Copy link

pag4k commented Jun 2, 2019

Well, I think I can answer my first question: new_inherited() is only used by PerformanceNavigationTiming and its specifications says that startTime attribute MUST return a DOMHighResTimeStamp with a time value of 0. So based on that, startTime should be left to 0, but I'm not sure to what extent we can generalize based on the case PerformanceNavigationTiming...

@jdm
Copy link
Member

jdm commented Oct 10, 2019

Unassigning due to lack of activity.

@jdm jdm removed the C-assigned There is someone working on resolving the issue label Oct 10, 2019
@pajamapants3000
Copy link
Contributor

@highfive: assign me (please!)

@highfive
Copy link

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

@highfive highfive added the C-assigned There is someone working on resolving the issue label Oct 12, 2019
@pajamapants3000
Copy link
Contributor

pajamapants3000 commented Oct 13, 2019

@jdm I created #24430 for adding the set_attribute_from() method you suggested, but I was wondering about a different possible approach. I was thinking of adding a ResourceTimerValue enum to, eventually, replace RedirectStartValue and RedirectEndValue and also be used for the future implementation of additional timers (like startTime). It would have variants Zero, Now, FetchStart, plus any additional timers whose values need to be copied. We could retain the existing API, just continuing to use set_attribute, and each of the variants of ResourceAttribute that refer to timer attributes could take a ResourceTimerValue to indicate how it is being set.
If that doesn't sound good, let me know and also I would appreciate some feedback on #24430 if you get a chance. Of course if the enum sounds better I'm happy to drop #24430. But in order to add startTime I need some way of setting it! Thank you :)

UPDATE: this commit shows what I'm thinking. If this seems preferable, let me know and I'll make a PR and close the one in #24430.
Still figuring out the testing updates; I'll post any questions I have, but any guidance is welcome of course.

@jdm
Copy link
Member

jdm commented Oct 15, 2019

I like that idea!

@pajamapants3000
Copy link
Contributor

Hey folks, just an update: I didn't get lost; still working on this. I think I have a solution, I'm just figuring out the whole testing issue. This has proven rather time-consuming, given how long it takes to run the full test-wpt suite. That, and just familiarizing myself with everything.

I am currently attempting to see if my changes have resulted in any difference in the test-wpt results. It appears that most/all of the relevant wpt encompass multiple features, many of which (e.g. duration) have not yet been implemented. If implementing start time isn't enough to turn any FAILs to PASSes, I may add a unit-test or two and possibly some tests under tests/wpt/mozilla.

If anyone has anything to suggest or add, please let me know! Otherwise, I'll continue on and hopefully have a PR ready sometime this weekend.

@jdm
Copy link
Member

jdm commented Oct 19, 2019

How are you running your tests? You should really only be running a single directory like resource-timing, not the whole testsuite.

bors-servo pushed a commit that referenced this issue Oct 21, 2019
Add start_time to resource timing.

<!-- Please describe your changes on the following line: -->
`start_time` property added to `ResourceFetchTiming`, which enables the setting of `start_time` in the `PerformanceEntry` member of `PerformanceResourceTiming`.

Following the specification at https://w3c.github.io/resource-timing/#dfn-starttime, `start_time` is set to the value of `redirect_start` if redirection occurs and the timing allow check passes. Otherwise it has the same value as `fetch_start`.

---
<!-- 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 #21254

<!-- 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. -->
bors-servo pushed a commit that referenced this issue Oct 22, 2019
Add start_time to resource timing.

<!-- Please describe your changes on the following line: -->
`start_time` property added to `ResourceFetchTiming`, which enables the setting of `start_time` in the `PerformanceEntry` member of `PerformanceResourceTiming`.

Following the specification at https://w3c.github.io/resource-timing/#dfn-starttime, `start_time` is set to the value of `redirect_start` if redirection occurs and the timing allow check passes. Otherwise it has the same value as `fetch_start`.

---
<!-- 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 #21254

<!-- 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. -->
bors-servo pushed a commit that referenced this issue Oct 22, 2019
Add start_time to resource timing.

<!-- Please describe your changes on the following line: -->
`start_time` property added to `ResourceFetchTiming`, which enables the setting of `start_time` in the `PerformanceEntry` member of `PerformanceResourceTiming`.

Following the specification at https://w3c.github.io/resource-timing/#dfn-starttime, `start_time` is set to the value of `redirect_start` if redirection occurs and the timing allow check passes. Otherwise it has the same value as `fetch_start`.

---
<!-- 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 #21254

<!-- 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. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-perf-measurement C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor.
Projects
None yet
8 participants