Drive-by feedback

[domfarolino]

Handling inactive/detached documents

The spec right now unconditionally assumes that a Window's browsing context is non-null: example, example, example, more.

In order to handle the detached frame case:

const win = iframe.contentWindow;
iframe.remove();
win.SharedStorage.set()/append()/foo();

... and not crash, we have to bail-out early from algorithms that should not operate in this case. You can do this by detecting the null browsing context as you are, but then you miss the bf-cache case as well, so your best bet is to try and detect non-fully active documents as described by https://w3ctag.github.io/design-principles/#support-non-fully-active. Here is a good example usage: https://w3c.github.io/permissions/#dom-permissions-query of this.

Nothing defines how the shared storage interface is exposed

At least I don't think anything defines this. The examples in the Introduction show usage of window.sharedStorage, but I don't see the sharedStorage member actually being defined on the Window interface (if I'm just missing it, forgive me). The closes I see is in https://wicg.github.io/shared-storage/#shared-storage-interface which defines that the interfaces are "exposed" on Window, stops short of defining a name and getter. I think you can fix this by adding something similar to https://wicg.github.io/fenced-frame/#window-extension, where we define how the Fence interface is exposed via window.fence. This would clear up the many lines like this:

Let context be WindowSharedStorage's Window's browsing context.

... and this:

Let context be WorkletSharedStorage's SharedStorageWorkletGlobalScope's outside settings's target browsing context.

... which I don't think are 100% right.

(You might even want to consider doing what we (the fenced frame spec people) should probably do, which is make the getter off of Window just return null in the non-fully-active Document case. Then you don't have to keep adding the inactive checks in all of the other individual APIs.)

Indirect member references

Lines like:

Let queue be context’s associated shared storage database queue.

Are a tiny bit confusing since there is no member called "shared storage database queue" on browsing context. It might be better to make this:

Let queue be context’s associated database's shared storage database queue.

Bad promise resolution

This includes both in parallel promise resolution, as well as cross-event loop promise resolution. I noticed that in selectURL(), we resolve indexPromise in an event loop other than the other it was created in, which I don't think is allowed. The closest documentation I found for this is this example in HTML stating that you cannot resolve a promise in parallel, but I think more generally the guidance is you must resolve promises in the event loop that they were created in.

Another example of this is in https://wicg.github.io/shared-storage/#dom-workletsharedstorage-append, where while in parallel, step 8 > step 4 resolves the promise, but I don't think this is valid. This must queue a task to the outer event loop per the previous HTML link I referenced. There may be other examples (delete() seems like one) in your specification where this could be fixed.

Misc

• I noticed in https://wicg.github.io/shared-storage/#report-budget, we define a new member on each "top-level traversable". Just as an FYI, this excludes the top-level browsing context inside of a fenced frame. Is that the intention? In Chrome-implementation-speak, HTML's top-level traversable concept is 1:1 with "primary" frame in the browsing context, whereas "traversable navigable" is any main frame, including ones inside fenced frames. Just wanted to call this out to make sure we thought it through since our terminology is not super duper clear heh :/
• The https://wicg.github.io/shared-storage/#dom-windowsharedstorage-selecturl algorithm steps seem to be numerically split by the "Issue:" after step 6. Maybe the "Issue:" needs to be indented more?

[jkarlin]

@pythagoraskitty PTAL, thanks!

[pythagoraskitty]

@domfarolino Thank you so much for the feedback!

At your convenience, could you PTAL at PR 85, which attempts to address your feedback?

[domfarolino]

Sure, I'm currently on vacation for a bit so I will give it a thorough review mid next week if that is OK. (I don't think the feedback is serious enough to block any intended launch process steps so this spec should be OK from that front).

[pythagoraskitty]

Sounds good, thank you!

[pythagoraskitty]

This was closed prematurely due to the need to merge the associated PR. Hence, reopening (and will start a new PR to finish addressing previous comments).

[pythagoraskitty]

Linking comments from previous PR that I need to address:

• 1
• 2
• 3
• 4
• 5
• 6
• 7
• 8
• 9
• 10