Skip to content

Add may-block reasons to NotRestoredReasons - #10154

Merged
domenic merged 34 commits into
whatwg:mainfrom
rubberyuzu:nrr-add
Oct 9, 2024
Merged

domenic merged 34 commits into
whatwg:mainfrom
rubberyuzu:nrr-add

Conversation

@rubberyuzu

@rubberyuzu rubberyuzu commented Feb 22, 2024 •

Copy link
Copy Markdown
Contributor

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 )

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

ghost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
and reached the timeout limit.</dd>

<dt>"session-restored"</dt>
<dd>Session was restored for this page.</dd>

ghost Mar 11, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what this means.

ghost Mar 14, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread source Outdated
Comment thread source Outdated
Comment thread source
Comment thread source Outdated
Comment thread source Outdated
@rubberyuzu

ghost commented Mar 14, 2024

Copy link
Copy Markdown
Contributor Author

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

ghost commented Mar 15, 2024

Copy link
Copy Markdown
Contributor Author

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

ghost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
<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>

ghost Mar 15, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean?

ghost Mar 19, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rubberyuzu

ghost commented Mar 29, 2024

Copy link
Copy Markdown
Contributor Author

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

ghost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
@rubberyuzu

ghost commented Apr 5, 2024

Copy link
Copy Markdown
Contributor Author

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

ghost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread source Outdated
@rubberyuzu

ghost commented Apr 8, 2024

Copy link
Copy Markdown
Contributor Author

Thanks! I added existing spec references.

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

@rubberyuzu

ghost commented Apr 26, 2024

Copy link
Copy Markdown
Contributor Author

@smaug---- Can you PTAL? Thanks

@rubberyuzu

ghost commented May 7, 2024

Copy link
Copy Markdown
Contributor Author

@smaug---- @petervanderbeken Another ping.

@smaug----

ghost commented May 7, 2024

Copy link
Copy Markdown

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

@smaug----

ghost commented May 7, 2024

Copy link
Copy Markdown

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.

Comment thread source Outdated
Comment thread source Outdated
<span data-x="ua-specific-blocking-reasons">user-agent specific blocking reasons</span>.
</dl>

<p>In addition to the list above, a user agent might prevent the page from being restored from

ghost May 7, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure how to phrase this, but I think this should be a little clearer that "a user agent might choose to expose a reason that prevented …".

For the reasons where there might be less consensus, it would be good to allow UAs not to expose certain reasons even if they correspond to something in this second list. I know masked allows it, but this sentence seems to imply that if a UA blocks for one of these then it will expose that reason.

ghost May 13, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rubberyuzu

ghost commented Aug 27, 2024

Copy link
Copy Markdown
Contributor Author

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

Comment thread source Outdated
Comment thread source Outdated
@rubberyuzu

ghost commented Sep 6, 2024

Copy link
Copy Markdown
Contributor Author

Thanks, PTAL again

Comment thread 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>

ghost Sep 17, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ghost Oct 1, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread 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>

ghost Sep 17, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or should it be "serviceworker-message" ?

ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rubberyuzu

ghost commented Oct 1, 2024

Copy link
Copy Markdown
Contributor Author

Can you ptal @petervanderbeken ? Thanks in advance!

@petervanderbeken

ghost commented Oct 3, 2024

Copy link
Copy Markdown

LGTM now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

addition/proposal New features or enhancements topic: history

Development

Successfully merging this pull request may close these issues.

5 participants