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

Request's associated element #912

Open
yoavweiss opened this issue Jun 24, 2019 · 14 comments
Open

Request's associated element #912

yoavweiss opened this issue Jun 24, 2019 · 14 comments

Comments

@yoavweiss
Copy link
Collaborator

Element Timing has a concept of a Request's associated element, which is currently not well defined in terms of Fetch. I'd like to better define that, by being able to pass along the associated element when fetching an image at updating the image data.

Any objections to take that approach? Better ways to achieve the same?

/cc @annevk @domenic @npm1

@wanderview
Copy link
Member

Is this something that would need to be passed through a service worker that does evt.respondWith(fetch(evt.request))?

Also, how would this interact with the element lifecycle? For example, if the element is removed and discarded during the fetch, will the fetch keep it alive?

@yoavweiss
Copy link
Collaborator Author

Is this something that would need to be passed through a service worker that does evt.respondWith(fetch(evt.request))?

At least for the usecase we have in mind, that's not necessary.

Also, how would this interact with the element lifecycle? For example, if the element is removed and discarded during the fetch, will the fetch keep it alive?

Ideally we'd want to keep around a weak pointer to that element, so that a pending fetch, or a buffered ElementTiming entry won't keep it alive. Not sure what's the equivalent spec concept.

@annevk
Copy link
Member

annevk commented Jun 28, 2019

How long do you need the element reference for? Could this be something where we ensure everyone invokes "fetch" from the "main thread" and before "fetch" returns (and goes in parallel) it's able to drop the reference?

I guess with the img element such architecture might be somewhat problematic as it's not always clear what thread updates are requested from? (Although in browser reality it might just be that the img element definition is wrong as there are other "main thread" considerations here too, such as the currently active CSP and such.)

@yoavweiss
Copy link
Collaborator Author

How long do you need the element reference for? Could this be something where we ensure everyone invokes "fetch" from the "main thread" and before "fetch" returns (and goes in parallel) it's able to drop the reference?

We'd need the element's reference for longer than the lifetime of the fetch, as the reported entries are likely to be looked at after that. That is at least true if we want to maintain a link from the entries themselves to the element in question (which would be the most ergonomic solution, assuming we can pull it off).

@annevk
Copy link
Member

annevk commented Jul 2, 2019

It doesn't seem like Fetch would need to hold a reference for that. In its "main thread" section Fetch could create an entry and pass on the relevant information. And then at various points update the entry. Is the element needed for the updates?

@yoavweiss
Copy link
Collaborator Author

Yeah, Fetch would just need to pass the reference to the entry, not necessary keep it beyond that.

Regarding the need for the element during updates, I don't think that's needed. @npm1 - thoughts?

@npm1
Copy link
Contributor

npm1 commented Jul 2, 2019

Yea that should be fine. Besides the Element, the other information we need related to the request is the 'responseEnd' (from ResourceTiming...).

@yutakahirano
Copy link
Member

yutakahirano commented Jul 4, 2019

Implementation-wise this could be problematic because multiple elements can share one fetch request - with cache, for example. I guess we would have the same problem in the spec world.

cc @hiroshige-g

@annevk
Copy link
Member

annevk commented Jul 4, 2019

If the caches are defined in terms of Fetch and this were defined in terms of Fetch, all of that would fall out "naturally". To the extent things are not defined there would indeed be challenges.

@yutakahirano
Copy link
Member

https://html.spec.whatwg.org/multipage/images.html#the-list-of-available-images is a cache defined outside of the fetch spec.

@hiroshige-g
Copy link

hiroshige-g commented Jul 8, 2019

In general, I'd like to avoid adding dependencies from Fetch spec to specific elements, because Chromium is separating fetch implementation from HTML/DOM implementation (different processes, different directories, etc.). I'm afraid that fetch request's element can be used for more frequent interactions between Fetch and elements in the future (in unrelated contexts), which might be hard to implement.

Also, I feel there might be better places (outside Fetch spec) to be associated with elements.
Element Timing spec says

Every image resource that is fetched from a URL has an associated image request, which is a fetch request.

But image request and fetch request are different.

For example, in the case of

<img id="img1" src="a.jpg">
<img id="img2" src="a.jpg">

there would be two image requests (one for each <img>) and one fetch request.
If Element Timing wants to report two entries (i.e. one PerformanceElementTiming entry for each <img>) in this case, I expect we want to associate Element with image requests (in HTML spec) or something else, not fetch requests.
(I'm not sure how CSS spec should be modified though; are image requests also used for CSS image loading?)

@annevk
Copy link
Member

annevk commented Jul 10, 2019

@hiroshige-g the process split makes sense and I think other implementations are moving in similar directions. However, I think it's okay for Fetch to cover both sides. Before we go "in parallel" we have access to state like this, after we go "in parallel" such state is no longer present.

(Ideally CSS uses a similar image loading code path. I'm pretty sure it does in implementations, but the CSS specifications are not updated with this kind of information.)

@hiroshige-g
Copy link

Before we go "in parallel" we have access to state like this, after we go "in parallel" such state is no longer present.

Is this enforced in the spec? (e.g. if we add request's element in the spec, can we reject future PRs that acccess request's element after "in parallel"?)

@annevk
Copy link
Member

annevk commented Jul 12, 2019

We could explicitly set it to null and then assert it's null after passing the boundary and I agree that we'd never want to change that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants