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

Make performance.mark to take a FetchEvent instead of adding performanceMark on FetchEvent #2

Closed
rniwa opened this Issue Oct 26, 2018 · 1 comment

Comments

Projects
None yet
2 participants
@rniwa
Copy link

rniwa commented Oct 26, 2018

Instead of adding an interface specific to adding a performance entry related information to FetchEvent as currently proposed, add a FetchEvent as an optional argument to performance.mark.

This would avoid duplicating a very similar logic in two different places and avoids the layering violation of FetchEvent being aware of performance API.

This will allow the passing of meta data and other API benefits currently being proposed as well as future improvements for performance.mark to automatically benefit service workers.

@wanderview

This comment has been minimized.

Copy link
Owner

wanderview commented Oct 29, 2018

Thanks for the feedback. I've been thinking about this since the meeting and I think removing the method from FetchEvent makes a lot of sense.

At lunch @toddreifsteck further suggested maybe allowing the entry to be passed to a method on FetchEvent instead of explicitly tightly coupling the APIs. So, in User Timing L3 where performance.mark() returns its entry we could do something like this:

self.addEventListener("fetch", evt => {
  let entry = performance.mark("foo");
  evt.addTimingEntry(entry);
});

This would have a couple of nice benefits.

  1. As with your suggestion it would allow any changes to User Timing API to automatically benefit the service worker case.
  2. It opens the possibility to associate other entry types with the FetchEvent in the future. For example, maybe we want to allow the script to associate nested PerformanceResourceTiming entries from the service worker context for fetch() calls it makes. (I'm not proposing to do this right now, but its nice to have the API flexibility to support it.)

I'm going to plan to incorporate some change like this in the proposal this week. Thanks again.

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