Add spec for No-Vary-Search hint in speculation rules #278
Conversation
| Preview: |
no-vary-search.bs
Outdated
| 1. If |result| is null, then return null. | ||
| 1. Otherwise return |result|. | ||
|
|
||
| <p class="note">Speculation rule parsing is strict, so the return value of this algorithm surfaces parsing errors instead of silently returning the [=default URL search variance=]. |
There was a problem hiding this comment.
I'm not sure that logic applies here. Here's my reasoning:
Whatever the meaning of the NVS header, recognized or unrecognized is, what we want to know is "would the prefetch cache, when it arrives, consider it to match?". In that light, I think it makes sense to match the behavior of the prefetch cache (and therefore, to treat this no more strictly than it does).
There was a problem hiding this comment.
Ah, I think I see your point. Also the motivation for the strictness described in "parse speculation rules" talks about misunderstood rules leading to more speculation than was intended. But here, we know the consequence of an invalid hint and it would not cause more speculation.
For debuggability, we should presumably still have the implementation warn about this.
There was a problem hiding this comment.
Tagging @liviutinta in case of any concerns.
There was a problem hiding this comment.
I've updated the parsing of the hint to fallback to the default variance. Note that in "parse a speculation rule" this still treats non-string values for expects_no_vary_search as invalid.
| * |cutoffTime| is null or |record|'s [=prefetch record/creation time=] is less than |cutoffTime|. | ||
| 1. If |potentialRecords| [=list/is empty=], return null. | ||
| 1. Wait until the [=prefetch record/state=] of any element of |document|'s [=Document/prefetch records=] changes. | ||
| 1. If |cutoffTime| is null and any element of |potentialRecords| has a [=prefetch record/state=] that is not "`ongoing`", set |cutoffTime| to the [=current high resolution time=] for the [=relevant global object=] of |document|. |
There was a problem hiding this comment.
Shouldn't this be set before the wait, possibly outside the loop? I assume the goal here is "don't block on prefetches which started after we first started looking"? The current semantics are, at least, confusing to me.
There was a problem hiding this comment.
The note below this explains the motivation. The goal is to allow prefetches started after we started blocking, which is why the cutoff isn't set until this point. Would it help to move the note next to this line?
There was a problem hiding this comment.
Discussed a little aloud. I see the informative note below, but I'd appreciate it if it could be expanded to be explicit about the intuitive understanding of this heuristic (blocking only on prefetch requests which started before a prefetch completed), why we're using it (we want to allow a slightly later prefetch which is a better match, but don't want a fixed timeout that might cause issues with slow servers), and what its properties are (e.g., in the worst case, we block on two nearly-consecutive prefetches and then the actual navigation if we still failed to match).
Parentheticals are merely inspiration from my understanding and you can write a more cohesive note. :)
We introduce the "expects_no_vary_search" field for speculation rules and specify how it is parsed.
When matching prefetch records, we now have it:
Note that this spec text describes blocking on any available prefetch. The current chromium implementation blocks only on a single ongoing prefetch.