Bug 180052 – Add support for FetchEvent.clientId

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Add support for FetchEvent.clientId

[youenn fablet]

Created attachment 327777 [details]
Patch

[youenn fablet]

Created attachment 327786 [details]
Patch

[EWS Watchlist]

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.

[Chris Dumez]

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.

[youenn fablet]

> 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.

[youenn fablet]

Filed https://github.com/w3c/web-platform-tests/pull/8507

[youenn fablet]

Created attachment 327885 [details]
Patch

[Chris Dumez]

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?

[youenn fablet]

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.

[youenn fablet]

Created attachment 327894 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 327894 [details]
Patch for landing

Clearing flags on attachment: 327894

Committed r225294: <https://trac.webkit.org/changeset/225294>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35756355>