Bug 182301 - Delay service worker process creation until actually needed by SWServer
Summary: Delay service worker process creation until actually needed by SWServer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 182375
Blocks:
  Show dependency treegraph
 
Reported: 2018-01-30 11:43 PST by youenn fablet
Modified: 2018-02-01 16:26 PST (History)
7 users (show)

See Also:


Attachments
Patch (8.38 KB, patch)
2018-01-30 11:45 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (14.76 KB, patch)
2018-01-31 15:30 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (14.56 KB, patch)
2018-01-31 15:44 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (11.19 KB, patch)
2018-02-01 10:26 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (14.50 KB, patch)
2018-02-01 12:11 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (15.42 KB, patch)
2018-02-01 15:01 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-01-30 11:43:07 PST
This would allow not creating a service worker process if there is no need for it.
Comment 1 youenn fablet 2018-01-30 11:45:00 PST
Created attachment 332676 [details]
Patch
Comment 2 youenn fablet 2018-01-30 11:48:16 PST
Patch tested locally and it waits to create a service worker for a page to match.
Comment 3 youenn fablet 2018-01-30 11:48:28 PST
Would need API test probably.
Comment 4 youenn fablet 2018-01-31 15:30:48 PST
Created attachment 332809 [details]
Patch
Comment 5 EWS Watchlist 2018-01-31 15:33:41 PST
Attachment 332809 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:179:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:193:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:195:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:198:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:203:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 5 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 youenn fablet 2018-01-31 15:44:43 PST
Created attachment 332813 [details]
Patch
Comment 7 EWS Watchlist 2018-01-31 15:47:37 PST
Attachment 332813 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:734:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:748:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:750:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:753:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:758:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 5 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Chris Dumez 2018-01-31 21:27:08 PST
Comment on attachment 332813 [details]
Patch

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

> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp:210
> +    if (!StorageProcess::singleton().globalServerToContextConnection())

We no longer need this part if we land the patch at Bug 182375 first.
Comment 9 youenn fablet 2018-02-01 10:26:08 PST
Created attachment 332886 [details]
Patch
Comment 10 EWS Watchlist 2018-02-01 10:27:49 PST
Attachment 332886 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:734:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:748:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:750:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:753:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:758:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 5 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Chris Dumez 2018-02-01 11:36:57 PST
Comment on attachment 332813 [details]
Patch

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

> Source/WebCore/workers/service/server/SWServer.h:96
> +        virtual void scheduleJobInServer(ServiceWorkerJobData&&) = 0;

This seems weird. This is called due to IPC iirc. Therefore, I would expect this method to not be virtual and be on WebSWServerConnection instead.
Also, it looks like you kept SWServer::Connection::scheduleJobInServer() in the cpp?
Comment 12 youenn fablet 2018-02-01 12:11:06 PST
Created attachment 332901 [details]
Patch
Comment 13 EWS Watchlist 2018-02-01 12:13:55 PST
Attachment 332901 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:734:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:748:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:750:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:753:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:758:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Chris Dumez 2018-02-01 12:18:35 PST
Comment on attachment 332901 [details]
Patch

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

r=me with comment.

> Source/WebCore/workers/service/server/SWServer.cpp:219
> +void SWServer::Connection::scheduleJob(ServiceWorkerJobData&& jobData)

I do not think we need this method on SWServer::Connection.

> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp:210
> +    scheduleJob(WTFMove(jobData));

we can call server().scheduleJob(WTFMove(jobData)); directly, like we do in the other methods. No needs for a scheduleJob on Connection.
Comment 15 youenn fablet 2018-02-01 13:46:12 PST
(In reply to Chris Dumez from comment #14)
> Comment on attachment 332901 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332901&action=review
> 
> r=me with comment.
> 
> > Source/WebCore/workers/service/server/SWServer.cpp:219
> > +void SWServer::Connection::scheduleJob(ServiceWorkerJobData&& jobData)
> 
> I do not think we need this method on SWServer::Connection.
> 
> > Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp:210
> > +    scheduleJob(WTFMove(jobData));
> 
> we can call server().scheduleJob(WTFMove(jobData)); directly, like we do in
> the other methods. No needs for a scheduleJob on Connection.

Initially I did that, I kept it there as there is some logging done in SWServer.
I can convert it to release logging in WebKit2 code.
Comment 16 Chris Dumez 2018-02-01 13:49:31 PST
Comment on attachment 332901 [details]
Patch

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

>>> Source/WebCore/workers/service/server/SWServer.cpp:219
>>> +void SWServer::Connection::scheduleJob(ServiceWorkerJobData&& jobData)
>> 
>> I do not think we need this method on SWServer::Connection.
> 
> Initially I did that, I kept it there as there is some logging done in SWServer.
> I can convert it to release logging in WebKit2 code.

Yes please.
Comment 17 youenn fablet 2018-02-01 15:01:58 PST
Created attachment 332912 [details]
Patch for landing
Comment 18 EWS Watchlist 2018-02-01 15:02:59 PST
Attachment 332912 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:734:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:748:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:750:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:753:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:758:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 5 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 WebKit Commit Bot 2018-02-01 16:25:17 PST
Comment on attachment 332912 [details]
Patch for landing

Clearing flags on attachment: 332912

Committed r227989: <https://trac.webkit.org/changeset/227989>
Comment 20 WebKit Commit Bot 2018-02-01 16:25:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2018-02-01 16:26:30 PST
<rdar://problem/37136277>