Bug 205555 – ServiceWorkerJobData should have a move constructor

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

ServiceWorkerJobData should have a move constructor

[youenn fablet]

<rdar://problem/57853373>

[youenn fablet]

Created attachment 386337 [details]
Patch

[Darin Adler]

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?

[youenn fablet]

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.

[youenn fablet]

Created attachment 386521 [details]
Patch

[Darin Adler]

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?

[Darin Adler]

Sorry I keep asking the same question.

[youenn fablet]

(In reply to Darin Adler from comment #7)
> Sorry I keep asking the same question.

I can add it back if it seems better.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 386521 [details]:

fetch/fetch-worker-crash.html bug 187257 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 386521 [details]
Patch

Clearing flags on attachment: 386521

Committed r253967: <https://trac.webkit.org/changeset/253967>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.