Spec update: Support multiple & cross-origin worklets

xyaoinum commented 8 months ago

selectURL() and run() will be exposed to the shared storage worklet object. When calling on the default scoped worklet (i.e. sharedStorage.worklet.selectURL()/run()), the behavior is equivalent to sharedStorage.selectURL()/run().
Users can create new worklets via const worklet = await sharedStorage.createWorklet(url, options). This can be used to start multiple and potentially cross-origin worklets from a single document.
User settings error won't be exposed to the caller (i.e. will be treated as success) in the case of creating or using a cross-origin worklet. This is to prevent leaking user's settings for an arbitrary site.

Preview | Diff

xyaoinum commented 8 months ago

@pythagoraskitty: PTAL. Thanks! (After it's looking okay from your end, I will also request review from a spec mentor.)

pythagoraskitty commented 8 months ago

Suggest we move the {#reporting} and {#budgets} subsubsections of {#window} up to be subsections of the {#worklet} section instead. I have no strong preference about the ordering within that section, but you could probably just tack them on the end of it after {#scope-algo}.

pythagoraskitty commented 8 months ago

I've looked at most, except need to give a closer read to the new versions of selectURL() and run() sometime when I am less tired. :)

Hopefully will get to in the next couple of days. Otherwise looks good % comments.

pythagoraskitty commented 8 months ago

Finished checking. Looks good % comments. :)

xyaoinum commented 8 months ago

Thanks Cammie. I did some more changes around dividing the permissions checks, as explained in the comment above. PTAL again!

@domfarolino : PTAL from the spec's perspective. Thanks!

xyaoinum commented 7 months ago

@domfarolino : friendly ping! Also adding @wanderview as an alternative reviewer, who may be able to cover the review this week when Dominic is OOO. Thanks! The idea is to make the spec good enough to unblock launch. We can track non-blocking issues separately if applicable.

xyaoinum commented 7 months ago

Thanks @domfarolino for the detailed review. I've addressed / responded to your comments. PTAL again. Thanks!

xyaoinum commented 7 months ago

@domfarolino For comment https://github.com/WICG/shared-storage/pull/131#discussion_r1508141219, strangely, there's no way to respond, so I'm writing my response here:

|workletOrigin| is a valid URL's (i.e. |moduleURLRecord|) origin. How can it be null? Do you mean opaque? In that case, opaque origin can be handled by "determine whether shared storage is allowed by context".

Also, I think it might make it more readable to put the [=addModule initiated=] check to be front of the algorithm, and assert that |worklet|'s [=worklet origin=] is null afterwards. WDYT?

xyaoinum commented 7 months ago

@domfarolino I addressed you last comments. PTAL again. Thanks!

domfarolino commented 6 months ago

Responding to https://github.com/WICG/shared-storage/pull/131#issuecomment-1972259132.

|workletOrigin| is a valid URL's (i.e. |moduleURLRecord|) origin. How can it be null? Do you mean opaque? In that case, opaque origin can be handled by "determine whether shared storage is allowed by context".

Hmm, I can't remember exactly what I was thinking, but I probably was responding to the PR when we handled the URL manually before steps 1-3 in https://html.spec.whatwg.org/C#dom-worklet-addmodule did all of the URL parsing work.

https://github.com/WICG/shared-storage/blob/6606cc0e380b8ff1c2aa2d927c3dd3b03febadbb/spec.bs#L416-L419

In that case, the URL could be invalid and I guess a failure (but not null, you're right)? Either way, it looks OK now, since we're doing this after HTML's addModule() gets a first crack at parsing the URL.

Also, I think it might make it more readable to put the [=addModule initiated=] check to be front of the algorithm, and assert that |worklet|'s [=worklet origin=] is null afterwards. WDYT?

I think that sounds fine.