Improve computation of service worker FetchEvent.resultingClientId
Created attachment 448925 [details] Patch
Created attachment 449365 [details] Patch
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
Created attachment 449390 [details] Patch
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.
(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.
<rdar://problem/87763610>
Created attachment 449475 [details] Patch for landing
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?
(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.
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].