PerformanceResourceTiming: startTime #21254

avadacatavra · 2018-07-27T00:28:18Z

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 · 2018-11-29T01:35:40Z

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 · 2018-11-29T17:58:34Z

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 · 2018-11-29T18:02:47Z

@highfive: assign me

Taking it up for now :D

highfive · 2018-11-29T18:02:49Z

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

avadacatavra · 2018-12-03T21:13:33Z

Hi @chirgjn--thanks for taking this on!

Let me know if there's any help you need

chirgjn · 2018-12-03T23:12:28Z

@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 · 2019-01-07T20:02:17Z

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

aditj · 2019-01-29T06:03:52Z

@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 · 2019-01-29T07:24:54Z

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 · 2019-01-29T07:33:50Z

@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 · 2019-01-29T08:14:17Z

@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 · 2019-01-29T13:53:27Z

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 · 2019-01-29T14:04:18Z

@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 · 2019-01-29T14:23:41Z

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

davidweitzenfeld · 2019-03-06T03:41:26Z

@highfive: assign me

highfive · 2019-03-06T03:41:28Z

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

jdm · 2019-03-22T18:55:08Z

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

jdm · 2019-04-22T14:27:31Z

@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 · 2019-05-26T03:47:16Z

Unassigning due to lack of response.

pag4k · 2019-05-29T19:26:39Z

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 · 2019-05-29T19:26:42Z

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

pag4k · 2019-06-02T16:24:22Z

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.

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?
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 · 2019-06-02T16:29:01Z

Yes, that sounds fine.
No, that's #21270.

pag4k · 2019-06-02T17:43:53Z

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

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.
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 · 2019-06-02T18:05:15Z

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 · 2019-10-10T18:24:31Z

Unassigning due to lack of activity.

pajamapants3000 · 2019-10-12T08:41:52Z

@highfive: assign me (please!)

highfive · 2019-10-12T08:41:54Z

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

pajamapants3000 · 2019-10-13T14:58:27Z

@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 · 2019-10-15T12:38:00Z

I like that idea!

pajamapants3000 · 2019-10-19T12:25:42Z

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 · 2019-10-19T14:20:34Z

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


        Auto merge of #24508 - pajamapants3000:21254, r=jdm

Add start_time to resource timing.  `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`. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #21254  - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___


        Auto merge of #24508 - pajamapants3000:21254, r=jdm

Add start_time to resource timing.  `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`. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #21254  - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___


        Auto merge of #24508 - pajamapants3000:21254, r=jdm

Add start_time to resource timing.  `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`. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #21254  - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___

This was referenced Jul 27, 2018

Performance API Followup metabug #21253

Open

Updating performance implementation and putting more measurements in #20459

Merged

avadacatavra added E-easy A-perf-measurement labels Nov 29, 2018

highfive added the C-assigned label Nov 29, 2018

jdm added S-blocked-on-external and removed C-assigned labels Jan 7, 2019

highfive added the C-assigned label Mar 6, 2019

davidweitzenfeld mentioned this issue Mar 8, 2019

Add PerformanceResourceTiming: startTime davidweitzenfeld/servo#1

Closed

8 of 10 tasks complete

nagashayan mentioned this issue Apr 20, 2019

PerformanceResourceTiming: duration #21255

Closed

jdm removed the S-blocked-on-external label Apr 22, 2019

jdm removed the C-assigned label May 26, 2019

highfive added the C-assigned label May 29, 2019

jdm removed the C-assigned label Oct 10, 2019

highfive added the C-assigned label Oct 12, 2019

pajamapants3000 mentioned this issue Oct 12, 2019

Add an API to ResourceFetchTiming called set_attribute_from #24430

Closed

pajamapants3000 mentioned this issue Oct 20, 2019

Add start_time to resource timing. #24508

Merged

4 of 5 tasks complete

bors-servo closed this in bf587f2 Oct 22, 2019

servo / servo

PerformanceResourceTiming: startTime #21254

PerformanceResourceTiming: startTime #21254

avadacatavra commented Jul 27, 2018

highfive commented Nov 29, 2018

chirgjn commented Nov 29, 2018

chirgjn commented Nov 29, 2018

highfive commented Nov 29, 2018

avadacatavra commented Dec 3, 2018

chirgjn commented Dec 3, 2018 •

edited

jdm commented Jan 7, 2019

aditj commented Jan 29, 2019

chirgjn commented Jan 29, 2019

aditj commented Jan 29, 2019

chirgjn commented Jan 29, 2019

jdm commented Jan 29, 2019

aditj commented Jan 29, 2019

jdm commented Jan 29, 2019

davidweitzenfeld commented Mar 6, 2019

highfive commented Mar 6, 2019

jdm commented Mar 22, 2019

jdm commented Apr 22, 2019

jdm commented May 26, 2019

pag4k commented May 29, 2019

highfive commented May 29, 2019

pag4k commented Jun 2, 2019

jdm commented Jun 2, 2019

pag4k commented Jun 2, 2019

pag4k commented Jun 2, 2019 •

edited

jdm commented Oct 10, 2019

pajamapants3000 commented Oct 12, 2019

highfive commented Oct 12, 2019

pajamapants3000 commented Oct 13, 2019 •

edited

jdm commented Oct 15, 2019

pajamapants3000 commented Oct 19, 2019

jdm commented Oct 19, 2019 •

edited

servo / servo

Join GitHub today

PerformanceResourceTiming: startTime #21254

PerformanceResourceTiming: startTime #21254

Comments

avadacatavra commented Jul 27, 2018

highfive commented Nov 29, 2018

chirgjn commented Nov 29, 2018

chirgjn commented Nov 29, 2018

highfive commented Nov 29, 2018

avadacatavra commented Dec 3, 2018

chirgjn commented Dec 3, 2018 • edited

jdm commented Jan 7, 2019

aditj commented Jan 29, 2019

chirgjn commented Jan 29, 2019

aditj commented Jan 29, 2019

chirgjn commented Jan 29, 2019

jdm commented Jan 29, 2019

aditj commented Jan 29, 2019

jdm commented Jan 29, 2019

davidweitzenfeld commented Mar 6, 2019

highfive commented Mar 6, 2019

jdm commented Mar 22, 2019

jdm commented Apr 22, 2019

jdm commented May 26, 2019

pag4k commented May 29, 2019

highfive commented May 29, 2019

pag4k commented Jun 2, 2019

jdm commented Jun 2, 2019

pag4k commented Jun 2, 2019

pag4k commented Jun 2, 2019 • edited

jdm commented Oct 10, 2019

pajamapants3000 commented Oct 12, 2019

highfive commented Oct 12, 2019

pajamapants3000 commented Oct 13, 2019 • edited

jdm commented Oct 15, 2019

pajamapants3000 commented Oct 19, 2019

jdm commented Oct 19, 2019 • edited

chirgjn commented Dec 3, 2018 •

edited

pag4k commented Jun 2, 2019 •

edited

pajamapants3000 commented Oct 13, 2019 •

edited

jdm commented Oct 19, 2019 •

edited