Bug 205555 - ServiceWorkerJobData should have a move constructor
Summary: ServiceWorkerJobData should have a move constructor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-12-23 03:38 PST by youenn fablet
Modified: 2020-01-01 05:05 PST (History)
6 users (show)

See Also:


Attachments
Patch (7.66 KB, patch)
2019-12-23 03:48 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (3.60 KB, patch)
2019-12-30 03:03 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 2019-12-23 03:38:22 PST
ServiceWorkerJobData should have a move constructor
Comment 1 youenn fablet 2019-12-23 03:42:22 PST
<rdar://problem/57853373>
Comment 2 youenn fablet 2019-12-23 03:48:39 PST
Created attachment 386337 [details]
Patch
Comment 3 Darin Adler 2019-12-26 20:45:24 PST
Comment on attachment 386337 [details]
Patch

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

> Source/WebCore/workers/service/ServiceWorkerJobData.h:43
> -    ServiceWorkerJobData(SWServerConnectionIdentifier, const DocumentOrWorkerIdentifier& sourceContext);
> -    ServiceWorkerJobData(const ServiceWorkerJobData&) = default;
> -    ServiceWorkerJobData() = default;
> +    static ServiceWorkerJobData withIdentifier(SWServerConnectionIdentifier, const DocumentOrWorkerIdentifier& sourceContext);

I don’t understand why adding a move constructor requires changing the two item constructor to a named constructor. It should be as simple as removing the explicit copy constructor, or adding this:

    ServiceWorkerJobData(ServiceWorkerJobData&&) = default;

Or did I miss something?

> Source/WebCore/workers/service/ServiceWorkerJobData.h:95
> -    ServiceWorkerJobData jobData { WTFMove(*identifier) };
> +    ServiceWorkerJobData jobData;
> +    jobData.m_identifier = *identifier;

Why no WTFMove?
Comment 4 youenn fablet 2019-12-30 02:56:56 PST
Comment on attachment 386337 [details]
Patch

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

>> Source/WebCore/workers/service/ServiceWorkerJobData.h:43
>> +    static ServiceWorkerJobData withIdentifier(SWServerConnectionIdentifier, const DocumentOrWorkerIdentifier& sourceContext);
> 
> I don’t understand why adding a move constructor requires changing the two item constructor to a named constructor. It should be as simple as removing the explicit copy constructor, or adding this:
> 
>     ServiceWorkerJobData(ServiceWorkerJobData&&) = default;
> 
> Or did I miss something?

True, I could have kept ServiceWorkerJobData(SWServerConnectionIdentifier, const DocumentOrWorkerIdentifier& sourceContext).
Will add it back.
I wanted to have ServiceWorkerJobData() private so that we ensure ServiceWorkerJobData  always has a valid identifier.

>> Source/WebCore/workers/service/ServiceWorkerJobData.h:95
>> +    jobData.m_identifier = *identifier;
> 
> Why no WTFMove?

identifier is a struct of two ObjectIdentifier so WTFMove does not bring anything.
Comment 5 youenn fablet 2019-12-30 03:03:35 PST
Created attachment 386521 [details]
Patch
Comment 6 Darin Adler 2019-12-30 13:30:14 PST
Comment on attachment 386521 [details]
Patch

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

> Source/WebCore/workers/service/ServiceWorkerJobData.h:95
> +    jobData.m_identifier = *identifier;

Why no WTFMove?
Comment 7 Darin Adler 2019-12-30 13:30:42 PST
Sorry I keep asking the same question.
Comment 8 youenn fablet 2019-12-31 01:55:38 PST
(In reply to Darin Adler from comment #7)
> Sorry I keep asking the same question.

I can add it back if it seems better.
Comment 9 WebKit Commit Bot 2020-01-01 05:04:15 PST
The commit-queue encountered the following flaky tests while processing attachment 386521 [details]:

fetch/fetch-worker-crash.html bug 187257 (author: youennf@gmail.com)
The commit-queue is continuing to process your patch.
Comment 10 WebKit Commit Bot 2020-01-01 05:05:08 PST
Comment on attachment 386521 [details]
Patch

Clearing flags on attachment: 386521

Committed r253967: <https://trac.webkit.org/changeset/253967>
Comment 11 WebKit Commit Bot 2020-01-01 05:05:09 PST
All reviewed patches have been landed.  Closing bug.