Add may-block reasons to NotRestoredReasons #10154

rubberyuzu · 2024-02-22T07:06:43Z

This is a follow-up PR of #9360, where we added NotRestoredReasons.
This PR adds a list of NotRestoredReasons that user agents may choose to block with.

/document-lifecycle.html ( diff )
/infrastructure.html ( diff )
/nav-history-apis.html ( diff )

domenic

Thanks for doing all this work! I think this is crucial to getting interoperability if other browsers want to implement the bfcache blocking reasons API.

I know the most important thing to get settled here are the names. I added various small comments about some problematic ones, but I additionally have the following more wide-ranging concerns:

Insufficient consistency around reasons related to the initial navigation and the HTTP response. I think all of the following are related to properties of the HTTP response: http-status-not-ok, non-http-https-scheme, http-not-get, no-response-head, cache-control-no-store, cache-control-no-cache, http-auth-required. Sometimes you prefix with http-, sometimes you don't. Sometimes you include response, sometimes you don't. For status-not-ok, you mention the noun, but for not-get, you don't.

My suggestions: response-status-not-ok, response-scheme-not-http-or-https, response-method-not-get, response-head-missing (?), response-cache-control-no-store, response-cache-control-no-cache, response-auth-required.
Inconsistency on how API names are incorporated. So far we have one API name reason, websocket, derived from the API named WebSocket. Some of your new reasons match this: serviceworker-*, webtransport, broadcastchannel. Some introduce hyphens: SharedWorker -> shared-worker, MediaStream -> media-stream. And for some I can't find the connection to the objects at all: SmartCardConnection -> smart-card, IdleDetector -> idle-manager, SpeechRecognition -> speech-recognizer, PaymentRequest -> payment-manager, OTPCredential -> sms. (I wonder if in these case it's some Chromium internal codebase object?) Also, ??? -> indexed-db-event. Finally, some cases use specification names: picture-in-picture, keyboard-lock, webserial, webrtc.

My suggestion would be to pick one of two paths here. If the failure is due to a very specific object being used, e.g. a WebSocket or a SpeechRecognition object, then use that object's name, converted to all-lowercase with no hyphens. Otherwise, if tons of different APIs from a spec cause bfcache failures and you want to use one umbrella reason for all of them, then use the specification name. Prefer the former when possible, because it helps avoid exposing existing inconsistencies in how we name specs.

My next biggest concern after these inconsistencies is about the cases I've identified where I have no idea what the reason is indicating. I suspect if I don't know, other implementers won't, and so we won't get interoperability. We can help these cases by expanding on the descriptions until they become clearer.

In the end we may need more cross-specification links to do this. But I know adding those is a pain, so it's fine to leave that until the end of the process.

source

domenic · 2024-03-11T07:52:36Z

source

+   and reached the timeout limit.</dd>
+
+   <dt>"session-restored"</dt>
+   <dd>Session was restored for this page.</dd>


I don't know what this means.

This means the page is loaded via session history because of tab restore. Reworded this.

source

rubberyuzu · 2024-03-14T14:14:36Z

Thanks Domenic for the review! I think I addressed all of your comments. I changed the reason name to use object name as much as possible, and only when that's not possible the name is the generic API name.

rubberyuzu · 2024-03-15T03:25:59Z

@smaug---- Can you PTAL at this? Thanks in advance!

domenic

Great work. I can understand almost all of these reasons now, and the naming feels very consistent. The work on the service worker reasons in particular is very, very helpful.

I left a lot of editorial comments, but to highlight the important ones that impact reason names or similar:

We should remove database
cookie-flushed is a bit strange; maybe -deleted or -removed would be better
keepalive-request -> response-keepalive, I think? If I understand it correctly?
I don't understand response-head-missing
loading + subframe-navigating should probably become either navigating + frame-navigating, or loading + frame-loading
I'm not sure I understand timeout

Note to @smaug----, with my HTML editor hat on:

This list includes a lot of reasons that are specific to Chromium implementation choices. I strongly believe that's OK, and furthermore I commend the Chromium team and @rubberyuzu for bringing this through the standards process, creating this sort of centralized registry. We should encourage this style, instead of specs which have free-form fields for this sort of analytics data. So we should not criticize some of these reasons as Chromium-only. I think that goes even for Chromium-only reasons based on Chromium-only specs (e.g. smart card API). It's better to have everything documented and centralized. Having a smartcardconnection bfcache blocking reason in HTML, doesn't sanction the existence of the Smart Card API in general.

If any other browser engine, like Gecko, wants to add their own very-specific reasons to the list, we should welcome that too.

But we would very much appreciate eyes from a second implementer to ensure that the reasons we're adding here are reusable if other browsers end up with similar implementation constraints. This means, basically, that the descriptions given here are clear enough to be interoperably implementable.

What I'm picturing here is a web developer, staring at their mountains of analytics data, and doing things like:

Noticing that reason X is very high in both Firefox and Chrome. So they should fix their site!
Noticing that reason X is high in Chrome, but not in Firefox. So they should bug Chromium developers to adopt whatever strategy Firefox took to avoid the problem!

These kind of use cases are enabled by us collaborating on the reason list, instead of letting it be a free-for-all string.

source

smaug---- · 2024-03-15T09:56:23Z

source

+   <dt>"<dfn
+   data-x="blocking-non-trivial-bcg" export>
+   <code>non-trivial-browsing-context-group</code></dfn>"</dt>
+   <dd>The <span>browsing context group</span> of this <code>Document</code> was non-trivial.</dd>


What does this mean?

This means that related active contents exists for this document, i.e. the document called window.open().

This CL renames the reporting strings of NotRestoredReasons API so that they match the spec draft[1]. [1]: whatwg/html#10154 Bug:331754704 Change-Id: If805ad7e30c14ff64b440c8d9515eaee38370fd8

This CL renames the reporting strings of NotRestoredReasons API so that they match the spec draft[1]. [1]: whatwg/html#10154 Bug: 331754704 Change-Id: If805ad7e30c14ff64b440c8d9515eaee38370fd8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5392283 Commit-Queue: Yuzu Saijo <[email protected]> Reviewed-by: Rakina Zata Amni <[email protected]> Cr-Commit-Position: refs/heads/main@{#1280059}

rubberyuzu · 2024-03-29T05:37:42Z

@smaug---- Please let me know if there's anything unclear. Thanks!

domenic

Sorry for not re-reviewing after the last round. I found a few more potential renames...

source

rubberyuzu · 2024-04-05T08:36:02Z

Thanks Domenic and sorry for not noticing your review earlier!
I updated the names following your suggestions. PTAL again :)

domenic

This LGTM with a nit. I do not have any more substantial feedback or any rename suggestions.

I think it would be nicer for spec readers if everything that talked about another spec had a reference, like we currently do for "websocket" with [WEBSOCKETS] and "lock" with [WEBLOCKS].

However, adding these is such an annoying process when editing HTML. And they are only used in this one context. I don't feel like it should be required to merge this PR.

As a compromise, I would appreciate it if you could add such references for specs that are already in the bibliography. So e.g. add various [SERVICEWORKERS] and [INDEXEDDB] and so on. But you don't need to add new bibliography entries for every single reason.

source

rubberyuzu · 2024-04-08T10:36:24Z

Thanks! I added existing spec references.

@smaug---- Can you PTAL? Thanks in advance

…ec, a=testonly Automatic update from web-platform-tests [bfcache] Rename reasons to match the spec This CL renames the reporting strings of NotRestoredReasons API so that they match the spec draft[1]. [1]: whatwg/html#10154 Bug: 331754704 Change-Id: If805ad7e30c14ff64b440c8d9515eaee38370fd8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5392283 Commit-Queue: Yuzu Saijo <[email protected]> Reviewed-by: Rakina Zata Amni <[email protected]> Cr-Commit-Position: refs/heads/main@{#1280059} -- wpt-commits: e99b8186f7272559b3d419445faef703958bfdd1 wpt-pr: 45408

rubberyuzu · 2024-04-26T03:36:59Z

@smaug---- Can you PTAL? Thanks

rubberyuzu · 2024-05-07T02:51:49Z

@smaug---- @petervanderbeken Another ping.

smaug---- · 2024-05-07T08:17:30Z

We'll go through this later today. Sorry about delay.

smaug---- · 2024-05-07T13:24:10Z

We're ok to expose those reasons which the web site can affect to (and which are based on some specification), but nothing happening possibly in cross-origin iframes (like "frame-navigating") or in the UA itself (like "cookie-disabled" or "extension-messaging") should be exposed.

source

rubberyuzu · 2024-08-27T08:32:49Z

I removed all the "before unloading" and used "while unloading" if applicable.
PTAL again, thanks!

source

rubberyuzu · 2024-09-06T06:38:42Z

Thanks, PTAL again

smaug---- · 2024-09-17T13:49:17Z

source

+   <code>SyncManager.register</code>, <code>PeriodicSyncManager.register</code>
+   , or <code>BackgroundFetchManager.fetch</code>.</dd>
+
+   <dt>"<dfn data-x="blocking-broadcastchannel-message" export><code>broadcastchannel-message</code></dfn>"</dt>


You removed the plain "broadcastchannel", right? Maybe that name could be used here.

I would rather keep message to make it clear that the existence of broadcastchannel is not blocking, and also with the history of having broadcastchannel as a reason.

smaug---- · 2024-09-17T13:53:38Z

source

+   while the page was in <a href="#note-bfcache">back/forward cache</a>.
+   <ref>SW</ref></dd>
+
+   <dt>"<dfn data-x="blocking-sw-message" export><code>serviceworker-postmessage</code></dfn>"</dt>


or should it be "serviceworker-message" ?

smaug----

My two comments aren't blocking, but consider to use those, IMO clearer reason names. @petervanderbeken will also review

rubberyuzu · 2024-10-01T08:30:28Z

Can you ptal @petervanderbeken ? Thanks in advance!

petervanderbeken · 2024-10-03T10:07:15Z

LGTM now.

This CL updates the reason names reported by NotRestoredReasonsAPI so that they match the ones in the spec. On the spec PR we decided some reasons should not be exposed, i.e. reported as masked, and changed other reasons names. Mainly we are changing the reporting name to be 'masked' in this CL. The change is guarded with a runtime flag. Note that session restore should be reported as null but with this CL it is still reported as "session-restored". I will address this case and add a test in a separate CL. Spec PR: whatwg/html#10154 PSA: https://groups.google.com/a/chromium.org/g/blink-dev/c/keA8QE_uFxo/m/OqtfRXq6BwAJ?utm_medium=email&utm_source=footer Bug: 331754704 Change-Id: I0d4224f599bc4cdae7a70b9bbb38ff798bae2917 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5907581 Commit-Queue: Yuzu Saijo <[email protected]> Reviewed-by: Fergal Daly <[email protected]> Reviewed-by: Kent Tamura <[email protected]> Cr-Commit-Position: refs/heads/main@{#1381607}

rubberyuzu marked this pull request as ready for review March 6, 2024 07:57

domenic reviewed Mar 11, 2024

View reviewed changes

domenic reviewed Mar 15, 2024

View reviewed changes

smaug---- reviewed Mar 15, 2024

View reviewed changes

rubberyuzu force-pushed the nrr-add branch 3 times, most recently from 9ae1fff to 6abbb90 Compare March 21, 2024 03:58

tunetheweb mentioned this pull request Mar 27, 2024

Adds docs for notRestoredReasons mdn/content#27947

Closed

chromium-wpt-export-bot mentioned this pull request Mar 28, 2024

[bfcache] Rename reasons to match the spec web-platform-tests/wpt#45408

Merged

domenic reviewed Mar 29, 2024

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

domenic approved these changes Apr 8, 2024

View reviewed changes

source Outdated Show resolved Hide resolved

rubberyuzu force-pushed the nrr-add branch from 71e55ec to d8b69ac Compare April 8, 2024 10:27

rubberyuzu force-pushed the nrr-add branch from abb228b to be5b6e5 Compare April 8, 2024 13:45

domenic approved these changes Apr 11, 2024

View reviewed changes

smaug---- reviewed May 7, 2024

View reviewed changes

source Outdated Show resolved Hide resolved

rubberyuzu added 8 commits August 27, 2024 08:31

add more reference

c531056

rewrite sentence

6c2de7a

fix compile error

c0d4792

add unloading

3312d54

fix typo

e690cc4

fix while unloading

fd3752b

rephrase and add bc message

0558d57

remove before unloading

23fcbdb

rubberyuzu force-pushed the nrr-add branch from 3e7ca19 to 23fcbdb Compare August 27, 2024 08:32

petervanderbeken reviewed Sep 3, 2024

View reviewed changes

source Outdated Show resolved Hide resolved

smaug---- reviewed Sep 3, 2024

View reviewed changes

source Outdated Show resolved Hide resolved

remove some items

efce98d

rubberyuzu force-pushed the nrr-add branch from 68ec0eb to efce98d Compare September 6, 2024 06:38

smaug---- reviewed Sep 17, 2024

View reviewed changes

smaug---- approved these changes Sep 17, 2024

View reviewed changes

rubberyuzu and others added 3 commits October 3, 2024 21:16

Merge branch 'whatwg:main' into nrr-add

a79234d

Nits and line wrapping

ebde193

More minor tweaks

171f1e8

domenic added addition/proposal New features or enhancements topic: navigation topic: history and removed topic: navigation labels Oct 9, 2024

domenic merged commit 33cf828 into whatwg:main Oct 9, 2024
2 checks passed

smaug---- mentioned this pull request Nov 7, 2024

Back/forward cache NotRestoredReasons API mozilla/standards-positions#766

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add may-block reasons to NotRestoredReasons #10154

Add may-block reasons to NotRestoredReasons #10154

rubberyuzu commented Feb 22, 2024 •

edited by pr-preview bot

Loading

domenic left a comment

domenic Mar 11, 2024

rubberyuzu Mar 14, 2024

rubberyuzu commented Mar 14, 2024

rubberyuzu commented Mar 15, 2024

domenic left a comment

smaug---- Mar 15, 2024

rubberyuzu Mar 19, 2024

rubberyuzu commented Mar 29, 2024

domenic left a comment

rubberyuzu commented Apr 5, 2024

domenic left a comment

rubberyuzu commented Apr 8, 2024

rubberyuzu commented Apr 26, 2024

rubberyuzu commented May 7, 2024

smaug---- commented May 7, 2024

smaug---- commented May 7, 2024

rubberyuzu commented Aug 27, 2024

rubberyuzu commented Sep 6, 2024

smaug---- Sep 17, 2024

rubberyuzu Oct 1, 2024

smaug---- Sep 17, 2024

smaug---- left a comment

rubberyuzu commented Oct 1, 2024

petervanderbeken commented Oct 3, 2024

Add may-block reasons to NotRestoredReasons #10154

Add may-block reasons to NotRestoredReasons #10154

Conversation

rubberyuzu commented Feb 22, 2024 • edited by pr-preview bot Loading

domenic left a comment

Choose a reason for hiding this comment

domenic Mar 11, 2024

Choose a reason for hiding this comment

rubberyuzu Mar 14, 2024

Choose a reason for hiding this comment

rubberyuzu commented Mar 14, 2024

rubberyuzu commented Mar 15, 2024

domenic left a comment

Choose a reason for hiding this comment

smaug---- Mar 15, 2024

Choose a reason for hiding this comment

rubberyuzu Mar 19, 2024

Choose a reason for hiding this comment

rubberyuzu commented Mar 29, 2024

domenic left a comment

Choose a reason for hiding this comment

rubberyuzu commented Apr 5, 2024

domenic left a comment

Choose a reason for hiding this comment

rubberyuzu commented Apr 8, 2024

rubberyuzu commented Apr 26, 2024

rubberyuzu commented May 7, 2024

smaug---- commented May 7, 2024

smaug---- commented May 7, 2024

rubberyuzu commented Aug 27, 2024

rubberyuzu commented Sep 6, 2024

smaug---- Sep 17, 2024

Choose a reason for hiding this comment

rubberyuzu Oct 1, 2024

Choose a reason for hiding this comment

smaug---- Sep 17, 2024

Choose a reason for hiding this comment

smaug---- left a comment

Choose a reason for hiding this comment

rubberyuzu commented Oct 1, 2024

petervanderbeken commented Oct 3, 2024

rubberyuzu commented Feb 22, 2024 •

edited by pr-preview bot

Loading