Summary: | Update SWServerJobQueue to follow the Service Worker specification more closely | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | beidson, ggaren, webkit-bug-importer, youennf | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Chris Dumez
2017-11-01 18:56:52 PDT
Created attachment 325662 [details]
Patch
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 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. Created attachment 325727 [details]
Patch
Created attachment 325729 [details]
Patch
Comment on attachment 325729 [details] Patch Clearing flags on attachment: 325729 Committed r224341: <https://trac.webkit.org/changeset/224341> All reviewed patches have been landed. Closing bug. |