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

Consider timingInfo handling in static routing API #19

Open
sisidovski opened this issue Jan 15, 2024 · 6 comments
Open

Consider timingInfo handling in static routing API #19

sisidovski opened this issue Jan 15, 2024 · 6 comments

Comments

@sisidovski
Copy link
Collaborator

sisidovski commented Jan 15, 2024

In https://www.w3.org/TR/service-workers/#handle-fetch, timingInfo is set with start time, fetch event dispatch time appropriately. As the static routing API change the flow of the handle fetch algorithm, we have to define when/what value should be set to timingInfo.

Spec update to the ServiceWorker and WPTs are needed.

@yoshisatoyanagisawa
Copy link
Collaborator

In current Chromium implementation, if "cache" source is used, the timing is set like:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/service_worker/service_worker_cache_storage_matcher.cc;l=118;drc=c686e8f4fd379312469fe018f5c390e9c8f20d0d
I just put random timing info to adjust to the DidDispatchFetchEvent interface.

However, we should revisit what information is set there and given as the timing Info. There might be three options:
Option A: giving new meanings to existing timing Info fields.
Option B: new timing Info field is defined for the static routing API.
Option C: no timing Info are provided for the static routing API.

My recommendation is Option B.
No implementation is needed for Option C, but developers lose the timing Information when the API is used, which sounds not good when the API is widely used.
Option A can be easy to implement, but developers may want to know more details on the route. Option A cannot fulfill that.
Option B may bring dedicated information for the static routing API, and fix concerns for Option A and C. Drawback should be design and implementation cost.

@domenic
Copy link
Contributor

domenic commented Jan 18, 2024

It looks like this impacts the two properties workerStart and fetchStart of the navigation timing entries.

Option B involves adding new properties, right? Do you have a proposal for those?

But even if you do option B, we still need to decide what values the existing properties get. Have you thought about what is preferred there? Answering this seems more important, as you can always add new properties later, after gathering more input from partners and the standards community.

To me, setting them both to 0 seems reasonable. (Or whatever default value falls out of the processing, if it is not 0.)

@yoshisatoyanagisawa
Copy link
Collaborator

Yes, I have drafted the proposal for Option B. I suggested to add 4 fields there. Since timing is got inside the Handle Fetch algorithm, I suggested to refer the Service Worker timing in the Resource timing.

Existing properties are left as-is. Values may set upon the source. For the cache source, since we do not call the Run Service Worker algorithm, workerStart is left 0. The new cache lookup related field will be filled.

As you know, I am working on drafting the Service Worker static routing specification (PR that gathers all changes, working branch). The proposal will be adjusted upon its progress.

@sisidovski
Copy link
Collaborator Author

To me, setting them both to 0 seems reasonable. (Or whatever default value falls out of the processing, if it is not 0.)

Agree. If the router source is network or cache, setting 0 is reasonable. If the source is race-network-and-fetch-handler, wondering what values are appropriate, especially when network wins the race. Should we set the actual values to workerStart and fetchStart, or we can just set 0 in that case?

aarongable pushed a commit to chromium/chromium that referenced this issue Feb 2, 2024
Even if the ServiceWorker static routing API is used and source that
does not run the fetch handler is used, we set the timing info.
To avoid unnecessary confusion, let me avoid setting them until the
following discussion reaches some agreement:
WICG/service-worker-static-routing-api#19

Change-Id: I87bc4bb22b3b72ca62ea9a6c27d29f34e6540dbe
Bug: 1523917
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5256859
Reviewed-by: Shunya Shishido <[email protected]>
Commit-Queue: Yoshisato Yanagisawa <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1255459}
@yoshisatoyanagisawa
Copy link
Collaborator

There is a github issue to discuss about this in the resource-timing repo.
w3c/resource-timing#389

@yoshisatoyanagisawa
Copy link
Collaborator

I suggest to continue the discussion in w3c/resource-timing#389 instead.

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

No branches or pull requests

3 participants