-
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
Add notRestoredReasons to PerformanceNavigationTiming attribute #195
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.
Overall I think this PR would be better if you added a small abstraction over in the HTML PR. Something like:
A document's not restored reasons is its node navigable's active session history entry's document state's not restored reasons, if document's node navigable is a top-level traversable; otherwise null.
(Or maybe the "if... otherwise" is not necessary, because you leave it null for those cases? That would be better.)
Then this PR could have only a single dependency across the Performance Timing -> HTML boundary:
If |document|'s [=Document/not restored reasons=] is not null, then set |navigationTimingEntry|'s [=PerformanceNavigationTiming/not restored reasons=] to the result of [=creating a NotRestoredReasons object=] given |document|'s [=Document/not restored reasons=].
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.
Looks great!
index.html
Outdated
@@ -615,6 +622,10 @@ <h2>Creating a navigation timing entry</h2> | |||
<li>Set |document|'s <span>navigation timing entry</span> to |navigationTimingEntry|. | |||
<li>Set |navigationTimingEntry|'s <a data-for="PerformanceNavigationTiming">`Critical-CH` restart time</a> | |||
to |criticalCHRestart|. | |||
<li>If |document|'s [=Document/not restored reasons=] is not null, then set |
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.
document's or document's state's? I guess those are the same thing, but oddly enough the HTML spec pr talks about them both.
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 added this abstraction below in the HTML spec:
A document's not restored reasons is its node navigable's active session history entry's document state's not restored reasons, if document's node navigable is a top-level traversable; otherwise null.
The abstraction is to avoid having to refer to document's node navigable's active session history's entry's document state multiple times here.
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.
Hmm, why do we need the null check here. Not restored reasons should be initialized with some value. Either with null or valid reasons object.
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.
You're right, I removed the null check.
See explainer at https://github.com/WICG/bfcache-not-restored-reason/blob/main/NotRestoredReason.md and the counterpart addition to the Navigation Timing API at w3c/navigation-timing#195.
This PR adds notRestoredReasons as a new attribute of PerformanceNavigationTiming.
Preview | Diff