-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Report EventSource & track ResourceTiming entries #7553
Conversation
source
Outdated
various media element state changes, as per the notes below, user-agents download media | ||
resources in various implementation-specific ways (range requests, buffering). The | ||
ResourceTiming entries reflect the de-facto fetches performed, rather than | ||
the resource at its entirety.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof. I am not too comfortable with this meshing of the rigorous processResponseEndOfBody with the non-rigorous paragraphs below this, even if you have a note paragraph explaining the mismatch. Honestly I'm not too happy with the rigorous request setup and fetch call, when in reality it sounds like there will be multiple fetch calls all of which share some characteristics of the request?
I don't really know how to square this. I'd like to call in @annevk for help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I was surprised that the whole range-request thing is a free-for-all which makes it hard to add reporting on top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should state here that user-agents may make several rigorous fetch calls that look a certain way with the optional range headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's the desired setup (though with certain limitations). @jakearchibald worked on this in #2814 but wasn't able to finish it. whatwg/fetch#144 (comment) also describes some of the complexity here.
The basic setup is that there needs to be an initial request (that gets the start of the response to scan) and identifies the post-redirect response URL and then zero or more subsequent requests (that go directly to the post-redirect response URL from the initial request). Perhaps writing that down and iterating on it would be a good way to go here?
@padenot has been looking at this in Firefox to address some security issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind taking this on. Since this is becoming an issue of its own though, I'm thinking to split it it out from the EventSource
patch.
49a203c
to
f63a190
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big <3 for doing the extra work of making EventSource rigorous, instead of just taking my suggestion to move all the non-rigorous stuff into the constructor.
f63a190
to
51d1ded
Compare
d27e6a9
to
22a75e2
Compare
Are there tests for |
At least two implementers are interested (and none opposed):
Tests are written and can be reviewed and commented upon at:
<track>
.Implementation bugs are filed:
(See WHATWG Working Mode: Changes for more details.)
/infrastructure.html ( diff )
/media.html ( diff )
/server-sent-events.html ( diff )