Improve computation of service worker FetchEvent.resultingClientId

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

Improve computation of service worker FetchEvent.resultingClientId

[youenn fablet]

Created attachment 448925 [details]
Patch

[youenn fablet]

Created attachment 449365 [details]
Patch

[youenn fablet]

Comment on attachment 449365 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449365&action=review

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/clients-get-resultingClientId.https-expected.txt:-5
> -PASS get(resultingClientId) for document sandboxed by CSP header

Regression fixed in https://bugs.webkit.org/show_bug.cgi?id=235310

[youenn fablet]

Created attachment 449390 [details]
Patch

[Darin Adler]

Comment on attachment 449390 [details]
Patch

The code should use ScriptExecutionContextIdentifier, not const std::optional<ScriptExecutionIdentifier>&. These identifiers are not particularly huge structures and don’t need to be passed by reference. And also, there is already a null value for identifiers, so we don’t need to wrap them in std::optional just to have a null meaning nothing is specified. We can create the null value with the default constructor, and detect whether it’s null using the operator bool.

I’m not sure how we decided in which cases to have a default value and in which cases callers have to explicitly pass { }. Since specifying a pre-determined identifier is fairly unusual, we should consider adding a default everywhere, and these calls to HTMLDocument::create would not have to change.

[youenn fablet]

(In reply to Darin Adler from comment #5)
> Comment on attachment 449390 [details]
> Patch
> 
> The code should use ScriptExecutionContextIdentifier, not const
> std::optional<ScriptExecutionIdentifier>&. These identifiers are not
> particularly huge structures and don’t need to be passed by reference. And
> also, there is already a null value for identifiers, so we don’t need to
> wrap them in std::optional just to have a null meaning nothing is specified.
> We can create the null value with the default constructor, and detect
> whether it’s null using the operator bool.

OK, will change.

> I’m not sure how we decided in which cases to have a default value and in
> which cases callers have to explicitly pass { }. Since specifying a
> pre-determined identifier is fairly unusual, we should consider adding a
> default everywhere, and these calls to HTMLDocument::create would not have
> to change.

I had the HTMLDocument constructor parameter optional initially.
I discovered I had to pass it to TextDocument as well. At that point, I moved it to a mandatory parameter so that  I was sure to make am explicit decision for this parameter.

[Radar WebKit Bug Importer]

<rdar://problem/87763610>

[youenn fablet]

Created attachment 449475 [details]
Patch for landing

[Darin Adler]

Comment on attachment 449475 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=449475&action=review

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/clients-get-resultingClientId.https-expected.txt:5
> +FAIL get(resultingClientId) for document sandboxed by CSP header assert_equals: promiseValue expected "undefinedValue" but got "client"

Do you know why this is now failing?

[youenn fablet]

(In reply to Darin Adler from comment #9)
> Comment on attachment 449475 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449475&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/clients-get-resultingClientId.https-expected.txt:5
> > +FAIL get(resultingClientId) for document sandboxed by CSP header assert_equals: promiseValue expected "undefinedValue" but got "client"
> 
> Do you know why this is now failing?

Yes, we are starting to always reuse IDs while we were never doing it previously.
The test will pass after https://bugs.webkit.org/show_bug.cgi?id=235310.

[EWS]

Committed r288201 (246173@main): <https://commits.webkit.org/246173@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449475 [details].