Bug 205555

Summary: ServiceWorkerJobData should have a move constructor
Product: WebKit Reporter: youenn fablet <youennf>
Component: Service WorkersAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, commit-queue, darin, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

youenn fablet
Reported 2019-12-23 03:38:22 PST
ServiceWorkerJobData should have a move constructor
Attachments
Patch (7.66 KB, patch)
2019-12-23 03:48 PST, youenn fablet 		no flags
DetailsFormatted DiffDiff
Patch (3.60 KB, patch)
2019-12-30 03:03 PST, youenn fablet 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
youenn fablet
Comment 1 2019-12-23 03:42:22 PST
<rdar://problem/57853373>
youenn fablet
Comment 2 2019-12-23 03:48:39 PST
Created attachment 386337 [details] Patch
Darin Adler
Comment 3 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?
youenn fablet
Comment 4 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.
youenn fablet
Comment 5 2019-12-30 03:03:35 PST
Created attachment 386521 [details] Patch
Darin Adler
Comment 6 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?
Darin Adler
Comment 7 2019-12-30 13:30:42 PST
Sorry I keep asking the same question.
youenn fablet
Comment 8 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.
WebKit Commit Bot
Comment 9 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.
WebKit Commit Bot
Comment 10 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>
WebKit Commit Bot
Comment 11 2020-01-01 05:05:09 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.