Summary: | Delay service worker process creation until actually needed by SWServer | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||||
Component: | Service Workers | Assignee: | youenn fablet <youennf> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | achristensen, beidson, cdumez, commit-queue, ews-watchlist, ggaren, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 182375 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
youenn fablet
2018-01-30 11:43:07 PST
Created attachment 332676 [details]
Patch
Patch tested locally and it waits to create a service worker for a page to match. Would need API test probably. Created attachment 332809 [details]
Patch
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.
Created attachment 332813 [details]
Patch
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 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. Created attachment 332886 [details]
Patch
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 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? Created attachment 332901 [details]
Patch
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 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. (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 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. Created attachment 332912 [details]
Patch for landing
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 on attachment 332912 [details] Patch for landing Clearing flags on attachment: 332912 Committed r227989: <https://trac.webkit.org/changeset/227989> All reviewed patches have been landed. Closing bug. |