Bug 179147 - Update SWServerJobQueue to follow the Service Worker specification more closely
Summary: Update SWServerJobQueue to follow the Service Worker specification more closely
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: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-01 18:56 PDT by Chris Dumez
Modified: 2017-11-15 12:24 PST (History)
4 users (show)

See Also:


Attachments
Patch (12.93 KB, patch)
2017-11-01 19:00 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (13.41 KB, patch)
2017-11-02 10:07 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (13.16 KB, patch)
2017-11-02 10:22 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-11-01 18:56:52 PDT
Update SWServerJobQueue to follow the Service Worker specification more closely.
Comment 1 Chris Dumez 2017-11-01 19:00:06 PDT
Created attachment 325662 [details]
Patch
Comment 2 youenn fablet 2017-11-02 10:02:50 PDT
Comment on attachment 325662 [details]
Patch

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

> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:103
> +    m_jobTimer.startOneShot(0_s);

Do we need a timer? Can't we do that with a lambda?

> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:108
> +    ASSERT(!m_jobQueue.isEmpty());

Is it needed? Is firstJob() not asserting somehow that the queue is not empty?
Ditto elsewhere

> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:212
>  void SWServerJobQueue::finishCurrentJob()

Since we remove the notion of current job, should we also remove it from some of the methods like this one.
Comment 3 Chris Dumez 2017-11-02 10:05:41 PDT
Comment on attachment 325662 [details]
Patch

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

>> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:103
>> +    m_jobTimer.startOneShot(0_s);
> 
> Do we need a timer? Can't we do that with a lambda?

How? if you're suggesting callOnMainThread([] { }), then I prefer a timer because it cannot outlive the SWServerJobQueue object.

>> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:108
>> +    ASSERT(!m_jobQueue.isEmpty());
> 
> Is it needed? Is firstJob() not asserting somehow that the queue is not empty?
> Ditto elsewhere

Not needed.

>> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:212
>>  void SWServerJobQueue::finishCurrentJob()
> 
> Since we remove the notion of current job, should we also remove it from some of the methods like this one.

I did not remove the notion of current job. I only removed the m_currentJob member and the code now treats firstJob() as the current job.
Comment 4 Chris Dumez 2017-11-02 10:07:47 PDT
Created attachment 325727 [details]
Patch
Comment 5 Chris Dumez 2017-11-02 10:22:22 PDT
Created attachment 325729 [details]
Patch
Comment 6 Chris Dumez 2017-11-02 10:39:02 PDT
Comment on attachment 325729 [details]
Patch

Clearing flags on attachment: 325729

Committed r224341: <https://trac.webkit.org/changeset/224341>
Comment 7 Chris Dumez 2017-11-02 10:39:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2017-11-15 12:24:33 PST
<rdar://problem/35567457>