[webkit-reviews] review granted: [Bug 181381] SWClientConnection should not keep references to service worker jobs : [Attachment 330699] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 181381: SWClientConnection should not keep references to service worker
jobs
https://bugs.webkit.org/show_bug.cgi?id=181381

Attachment 330699: Patch

https://bugs.webkit.org/attachment.cgi?id=330699&action=review




--- Comment #5 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 330699
  --> https://bugs.webkit.org/attachment.cgi?id=330699
Patch

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

r=me with comments.

> Source/WebCore/workers/service/SWClientConnection.cpp:48
> +void SWClientConnection::scheduleJob(ServiceWorkerJobIdentifier
jobIdentifier, DocumentOrWorkerIdentifier contextIdentifier, const
ServiceWorkerJobData& jobData)

I do not believe you need the jobIdentifier parameters, you can get it from the
jobData, no?

> Source/WebCore/workers/service/SWClientConnection.cpp:115
> +	   failedFetchingScript(jobIdentifier, registrationKey, ResourceError {
errorDomainWebKitInternal, 0, registrationKey.scope(), ASCIILiteral("Failed to
fetch service worker script") });

Seems odd to use the scope here. The URL we tried to fetch is the scriptURL so
we should use that URL in the error message.

> Source/WebCore/workers/service/SWClientConnection.cpp:253
>      auto jobs = WTFMove(m_scheduledJobs);

These are no longer jobs but jobSources

> Source/WebCore/workers/service/SWClientConnection.h:107
> +    enum class ShouldTakeJob { No, Yes };

I find this shouldTakeJob a bit confusing. We do not really take the job, we
remove a job source from the HashMap of job sources. Maybe this could be a enum
class IsJobComplete { No, Yes }; and then we'd remove the source from the map
if the job is complete?

> Source/WebCore/workers/service/SWClientConnection.h:110
> +    HashMap<ServiceWorkerJobIdentifier, DocumentOrWorkerIdentifier>
m_scheduledJobs;

m_scheduledJobSources? since it no longer contains jobs but source identifiers.