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: duration #21255

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

PerformanceResourceTiming: duration #21255

avadacatavra opened this issue Jul 27, 2018 · 24 comments

Comments

@avadacatavra
Copy link
Contributor

@avadacatavra avadacatavra commented Jul 27, 2018

In components/net/http_loader.rs::http_fetch we need to set duration for the PerformanceEntry member of PerformanceResourceTiming. This will be the difference between response_end and start_time

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

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

@haedaal
Copy link

@haedaal haedaal commented Dec 10, 2018

@highfive assign me :)

@highfive
Copy link

@highfive highfive commented Dec 10, 2018

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

@highfive highfive added the C-assigned label Dec 10, 2018
@jdm
Copy link
Member

@jdm jdm commented Jan 7, 2019

@haedaal Are you still working on this? Do you have any questions?

@jdm jdm removed the C-assigned label Jan 14, 2019
@vn-ki
Copy link
Contributor

@vn-ki vn-ki commented Feb 2, 2019

@highfive assign me

@highfive
Copy link

@highfive highfive commented Feb 2, 2019

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

@highfive highfive added the C-assigned label Feb 2, 2019
@vn-ki
Copy link
Contributor

@vn-ki vn-ki commented Feb 3, 2019

@jdm Can you please give some pointers where to start? I looked into components/net/http_loader.rs::http_fetch. I'm not sure what to change.

@jdm
Copy link
Member

@jdm jdm commented Feb 3, 2019

I believe that this does not require any changes to the code in components/net; it only requires computing the difference between the responseEnd and fetchStart values in components/script/dom/performanceresourcetiming.rs and adding the interface attribute in components/script/dom/webidls/PerformanceResourceTiming.webidl. However, we do not implement responseEnd yet, so you should probably fix #21263 first.

@vn-ki
Copy link
Contributor

@vn-ki vn-ki commented Feb 4, 2019

components/script/dom/performanceresourcetiming.rs

I can't find a file named that.

@jdm jdm removed the C-assigned label Mar 10, 2019
@nagashayan
Copy link

@nagashayan nagashayan commented Apr 18, 2019

@highfive assign me

@highfive highfive added the C-assigned label Apr 18, 2019
@highfive
Copy link

@highfive highfive commented Apr 18, 2019

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

@nagashayan
Copy link

@nagashayan nagashayan commented Apr 18, 2019

I see responseEnd and fetchStart is fixed now. this should be good to go now.

@jdm
Copy link
Member

@jdm jdm commented Apr 18, 2019

Great!

@nagashayan
Copy link

@nagashayan nagashayan commented Apr 20, 2019

The duration attribute MUST return a DOMHighResTimeStamp equal to the difference between responseEnd and startTime, respectively.

@nagashayan
Copy link

@nagashayan nagashayan commented Apr 20, 2019

startTime issue #21254 should be completed before we can work on duration. I see the PR is here davidweitzenfeld#1 but its for forked branch. @jdm right?

@jdm
Copy link
Member

@jdm jdm commented Apr 22, 2019

Indeed. If you want to cherry-pick that commit and build on top of it, that would be fine.

@nagashayan
Copy link

@nagashayan nagashayan commented Apr 22, 2019

right! I will do that. thanks.

@nagashayan
Copy link

@nagashayan nagashayan commented May 25, 2019

I started by cherry-picking on #21254 PR (in another forked repo), it seems that PR itself is not complete. starttime is not implemented in performanceresourcetiming.rs file.

@jdm
Copy link
Member

@jdm jdm commented Jul 12, 2019

@nagashayan Are you still working on this?

@nagashayan
Copy link

@nagashayan nagashayan commented Jul 13, 2019

@jdm yes, this weekend I will try to finish.

@nagashayan
Copy link

@nagashayan nagashayan commented Jul 13, 2019

@jdm Just refreshed my memory I am waiting on 21254 issue to be resolved. I need that value to complete this PR. what do you think?

@jdm
Copy link
Member

@jdm jdm commented Oct 10, 2019

Unassigning due to lack of activity.

@jdm jdm removed the C-assigned label Oct 10, 2019
@aug2uag
Copy link

@aug2uag aug2uag commented Oct 18, 2019

@jdm i cherry-picked the commits from David as mentioned a couple messages above and everything builds nice

no tests i don't know where to put them besides w3c_navigation_timing.js and there its completely empty at the moment plus the documentation for how to run it seems to be missing?

if PR #24493 looks good to you please assign me, if assigned please advise whether adding a setter function in PerformanceEntry that's called from PerformanceResourceTiming to set duration is the way to go-- also webidls look up-to-date and about the tests

bors-servo added a commit that referenced this issue Oct 31, 2019
…<try>

implement PerformanceResourceTiming duration

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

I wanted to give this a shot, but I'm not sure this is the appropriate implementation. In particular, I'm not sure if adding duration to the webidl interface is correct, because that doesn't seem to align with the web version (linked above the interface)?

Also, I'd like to write a test for this method, but I'm not sure where that test should go.

My apologies if this is way off target.

---
<!-- 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 #21255 (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 bors-servo closed this in a355f41 Nov 1, 2019
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.

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