Bug 180052 - Add support for FetchEvent.clientId
Summary: Add support for FetchEvent.clientId
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-27 13:34 PST by youenn fablet
Modified: 2017-11-29 13:48 PST (History)
9 users (show)

See Also:


Attachments
Patch (17.21 KB, patch)
2017-11-28 12:44 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (18.28 KB, patch)
2017-11-28 14:11 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (20.72 KB, patch)
2017-11-29 12:04 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (22.45 KB, patch)
2017-11-29 13:27 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-11-27 13:34:34 PST
Add support for FetchEvent.clientId
Comment 1 youenn fablet 2017-11-28 12:44:28 PST
Created attachment 327777 [details]
Patch
Comment 2 youenn fablet 2017-11-28 14:11:30 PST
Created attachment 327786 [details]
Patch
Comment 3 EWS Watchlist 2017-11-28 14:14:05 PST
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 4 Chris Dumez 2017-11-29 09:25:09 PST
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.
Comment 5 youenn fablet 2017-11-29 10:48:14 PST
> 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.
Comment 6 youenn fablet 2017-11-29 11:42:44 PST
Filed https://github.com/w3c/web-platform-tests/pull/8507
Comment 7 youenn fablet 2017-11-29 12:04:44 PST
Created attachment 327885 [details]
Patch
Comment 8 Chris Dumez 2017-11-29 12:11:28 PST
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?
Comment 9 youenn fablet 2017-11-29 13:04:34 PST
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.
Comment 10 youenn fablet 2017-11-29 13:27:41 PST
Created attachment 327894 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2017-11-29 13:47:41 PST
Comment on attachment 327894 [details]
Patch for landing

Clearing flags on attachment: 327894

Committed r225294: <https://trac.webkit.org/changeset/225294>
Comment 12 WebKit Commit Bot 2017-11-29 13:47:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2017-11-29 13:48:33 PST
<rdar://problem/35756355>