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

Specify bfcache behavior with proper tests #81

Open
saschanaz opened this issue Oct 15, 2021 · 12 comments
Open

Specify bfcache behavior with proper tests #81

saschanaz opened this issue Oct 15, 2021 · 12 comments

Comments

@saschanaz
Copy link
Member

saschanaz commented Oct 15, 2021

I intend to add a bfcache test where any active lock/request prevents caching, matching the current Chrome Dev 96 behavior.

Is this something you want to change or are you okay with the current Chrome behavior? To me it sounds reasonable enough, and in that case I think we should at least add a note about it, if it's not something can be specced.

@inexorabletash
Copy link
Member

Yes, thank you! I think we're okay with the current Chrome behavior.

@saschanaz
Copy link
Member Author

@inexorabletash In my observation Chrome disables bfcache at the first lock request and never enables it again, is this something Google wants to enhance or does it actually enable bfcache again?

@inexorabletash
Copy link
Member

I've reached out to our BFCache folks for an opinion.

@inexorabletash
Copy link
Member

Per @rakina - ideally we'd only make pages disable bfcache when it is necessary, so we could consider pages that have released all locks to be eligible for bfcache again.

So... might be a bug in Chrome (I haven't tested myself!). But let's not spec (or codify via WPT) an assumption that Chrome's current behavior is desired.

@saschanaz
Copy link
Member Author

Cool. Gecko also wants to enable bfcache again when eligible, and I'm planning to add tests for that. It seems bfcache tests are marked as optional (by using assert_optional_implements) so I guess doing so shouldn't flag Chrome behavior as invalid, at least for now.

@fergald
Copy link

fergald commented Oct 20, 2021

Yeah, it's not a bug in Chrome in the sense that it's perfectly fine to block BFCaching for any reason. It's not the situation we want long-term though, we just took the fastest path to correctness, knowing that we were over-blocking with a plan to come back and optimise.

As for the test, yes, it should give PRECONDITION_FAILED, please make use of the helpers, e.g. assert_bfcached

@hiroshige-g who is also adding BFCache tests FYI.

@saschanaz
Copy link
Member Author

From #78 (comment)

Coincidently, the following guidance just got merged into the TAG design guidelines:
https://w3ctag.github.io/design-principles/#support-non-fully-active

It seems this also covers bfcache which should help us spec the behavior.

@saschanaz saschanaz changed the title Add tests for bfcache Specify bfcache behavior with proper tests Nov 5, 2021
@saschanaz
Copy link
Member Author

Another FYI, WPT now has some bfcache tests specifically for Web Locks, e.g. https://wpt.live/web-locks/bfcache/held.tentative.https.html

@rakina
Copy link
Member

rakina commented Apr 27, 2022

It seems like we have a WPT already. Is there anything that needs to be updated on the spec side for this? BTW there's a guide on how to handle non-fully active documents (which includes BFCached documents).

@saschanaz
Copy link
Member Author

Is there anything that needs to be updated on the spec side for this?

It currently says nothing about blocking bfcache, so maybe we should add some?

It seems setting salvagable to false does that work, but in this way we can't safely set it true again.

@annevk, is salvagable the right tool to use here?

@annevk
Copy link
Member

annevk commented May 23, 2022

Yeah, you'd set salvageable to false during the unloading document cleanup steps.

cc @domenic

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

6 participants