-
Notifications
You must be signed in to change notification settings - Fork 30
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
Rename and update semantics for navigation duration measurement #27
Conversation
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.
Retroactive and belated LGTM
@@ -550,12 +550,14 @@ This can be useful for cleaning up any information in secondary stores, such as | |||
|
|||
The `window.appHistory` object has an event, `currententrychange`, which allows the application to react to any updates to the `appHistory.currentEntry` property. This includes both navigations that change its value, and calls to `appHistory.updateCurrentEntry()` that change its `state` or `url` properties. This cannot be intercepted or canceled, as it occurs after the navigation has already happened; it's just an after-the-fact notification. | |||
|
|||
This event has one special property, `event.navigationStartTimeStamp`, which for same-document navigations gives the value of `event.timeStamp` for the corresponding `navigate` event. This allows it to be used for determining the overall load time, including the time it took for a promise passed to `e.respondWith()` to settle: | |||
This event has one special property, `event.startTime`, which for [same-document](#appendix-types-of-navigations) navigations gives the value of `performance.now()` when the navigation was initiated. This includes for navigations that were originally [cross-document](#appendix-types-of-navigations), like the user clicking on `<a href="https://example.com/another-page">`, but were transformed into same-document navigations by [navigation interception](#navigation-monitoring-and-interception). For completely cross-document navigations, `startTime` will be `null`. |
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.
s/includes for navigations/includes navigations/?
_TODO: could we populate this for cross-document navigations too? Then it kind of overlaps with existing timing APIs, and is probably a lot harder to implement..._ | ||
_TODO: reconsider cross-document navigations. There will only be one (the initial load of the page); should we even fire this event in that case? (That's [#31](https://github.com/WICG/app-history/issues/31).) Could we give `startTime` a useful value there, if we do?_ | ||
|
||
_TODO: is this property-on-the-event design the right one, or should we integrate with the performance timeline APIs, or...? Discuss in [#33](https://github.com/WICG/app-history/issues/33)._ |
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.
We should totally integrate this with the performance APIs, but that doesn't necessarily mean this timestamp is not needed. Are there other use cases for this timestamp, other than reporting?
e.g. I know that we have Event.timestamp
, which is awfully similar.
Closes #8 by renaming. Closes #12 by changing the semantics to when the navigation is initiated.
This might be refined further as we figure out navigation queuing.
/cc @esprehn @mmocny @yoavweiss @igrigorik
I do worry a bit about the name
navigationStart
, as apparently in the navigation timing API definition, that is relative to the global monotonic clock, and not comparable to the values returned byevent.timeStamp
andperformance.now()
? So I worry this is a misleading symmetry. WDYT?