Add support for FetchEvent.clientId
Created attachment 327777 [details] Patch
Created attachment 327786 [details] Patch
Attachment 327786 [details] did not pass style-queue: ERROR: LayoutTests/imported/w3c/ChangeLog:9: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 327786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327786&action=review > Source/WebCore/loader/FetchOptions.h:171 > + std::optional<uint64_t> identifier; Why not encoder << clientIdentifier; ? > Source/WebCore/loader/FetchOptions.h:224 > + options.clientIdentifier = makeObjectIdentifier<DocumentIdentifierType>(clientIdentifier.value()); ObjectIdentifiers are serializable so we should not need to go to a uint64_t and then back. > LayoutTests/imported/w3c/ChangeLog:9 > + * web-platform-tests/service-workers/service-worker/resources/clients-get-worker.js Please explain why this changes are correct. We'd also need to send an upstream PR.
> View in context: > https://bugs.webkit.org/attachment.cgi?id=327786&action=review > > > Source/WebCore/loader/FetchOptions.h:171 > > + std::optional<uint64_t> identifier; > > Why not encoder << clientIdentifier; ? > > > Source/WebCore/loader/FetchOptions.h:224 > > + options.clientIdentifier = makeObjectIdentifier<DocumentIdentifierType>(clientIdentifier.value()); > > ObjectIdentifiers are serializable so we should not need to go to a uint64_t > and then back. FetchOptions are serialized for both IPC and Persistency. Persistency coders have not be modernized like IPC coders have and we cannot reuse directly ObjectIdentifiers serialization without updating persistency coders or adapting ObjectIdentifier serialization. The alternative would be to have two serializations for FetchOptions, one for persistency and one for IPC which would reuse the persistency one and add some additional parameters such as these client ids. I'll check that further. > > LayoutTests/imported/w3c/ChangeLog:9 > > + * web-platform-tests/service-workers/service-worker/resources/clients-get-worker.js > > Please explain why this changes are correct. We'd also need to send an > upstream PR. Sure, the spec changed from nullable to "" as a default. I'll upstream the PR before landing.
Filed https://github.com/w3c/web-platform-tests/pull/8507
Created attachment 327885 [details] Patch
Comment on attachment 327885 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327885&action=review r=me with comment > Source/WebCore/ChangeLog:12 > + If the fetch is for a subresouce, clientId is used directly. Typo: subresouce > Source/WebCore/loader/FetchOptions.h:227 > +template<class Decoder> inline bool FetchOptions::decode(Decoder& decoder, FetchOptions& options) Shouldn't this return an std::optional<> instead of using the legacy pattern?
Thanks for the review. (In reply to Chris Dumez from comment #8) > Comment on attachment 327885 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=327885&action=review > > r=me with comment > > > Source/WebCore/ChangeLog:12 > > + If the fetch is for a subresouce, clientId is used directly. > > Typo: subresouce > > > Source/WebCore/loader/FetchOptions.h:227 > > +template<class Decoder> inline bool FetchOptions::decode(Decoder& decoder, FetchOptions& options) > > Shouldn't this return an std::optional<> instead of using the legacy pattern? It should but it will probably break some not yet modernized code. I'll have a try though.
Created attachment 327894 [details] Patch for landing
Comment on attachment 327894 [details] Patch for landing Clearing flags on attachment: 327894 Committed r225294: <https://trac.webkit.org/changeset/225294>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35756355>