Bug 179192

Summary: Use a single identifier type to identify Service Workers
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, benjamin, buildbot, cmarcelo, dbates, ggaren, japhet, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2017-11-02 12:41:19 PDT
Use a String identifier consistently to identify Service Workers, as per the specification. We currently have both a String identifier and a uint64_t identifier for each service worker.
Comment 1 Chris Dumez 2017-11-02 12:46:47 PDT
Created attachment 325751 [details]
Patch
Comment 2 Brady Eidson 2017-11-02 13:04:38 PDT
Comment on attachment 325751 [details]
Patch

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

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.h:46
> +    const String& identifier() const { return m_serviceWorkerThread->identifier(); }

Who calls this? If there's a way to get rid of callers to this, then we don't need to worry.

If we truly do need to maintain this accessor, then we have two options:
1 - verify it is only used on a given thread and ASSERT it's only called on that thread going forward
2 - make it return an isolated copy each time. :(
Comment 3 Build Bot 2017-11-02 13:46:39 PDT
Comment on attachment 325751 [details]
Patch

Attachment 325751 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5079447

New failing tests:
svg/wicd/test-rightsizing-a.xhtml
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting.https.html
Comment 4 Build Bot 2017-11-02 13:46:40 PDT
Created attachment 325763 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 5 Chris Dumez 2017-11-02 21:42:28 PDT
Created attachment 325841 [details]
Patch
Comment 6 Chris Dumez 2017-11-02 21:43:31 PDT
Created attachment 325842 [details]
Patch
Comment 7 Chris Dumez 2017-11-02 21:44:15 PDT
Comment on attachment 325842 [details]
Patch

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

> Source/WTF/wtf/ObjectIdentifier.h:34
> +    ObjectIdentifier() = default;

Unfortunately, the IPC code currently requires me to have a default constructor. See error at https://pastebin.com/vNGBKWVK.
Comment 8 Chris Dumez 2017-11-02 21:51:30 PDT
Created attachment 325846 [details]
Patch
Comment 9 Build Bot 2017-11-02 21:53:11 PDT
Attachment 325846 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/ObjectIdentifier.h:36:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 2 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Chris Dumez 2017-11-02 21:54:58 PDT
Created attachment 325847 [details]
Patch
Comment 11 Build Bot 2017-11-02 21:56:15 PDT
Attachment 325847 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/ObjectIdentifier.h:36:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 2 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Chris Dumez 2017-11-02 22:02:59 PDT
Created attachment 325849 [details]
Patch
Comment 13 Build Bot 2017-11-02 22:05:19 PDT
Attachment 325849 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/ObjectIdentifier.h:36:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 2 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Chris Dumez 2017-11-02 22:09:50 PDT
Created attachment 325850 [details]
Patch
Comment 15 Build Bot 2017-11-02 22:12:32 PDT
Attachment 325850 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/ObjectIdentifier.h:36:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 2 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Chris Dumez 2017-11-02 22:14:19 PDT
Created attachment 325852 [details]
Patch
Comment 17 Build Bot 2017-11-02 22:15:24 PDT
Attachment 325852 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/ObjectIdentifier.h:36:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 2 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Chris Dumez 2017-11-02 22:22:11 PDT
Created attachment 325853 [details]
Patch
Comment 19 Build Bot 2017-11-02 22:24:21 PDT
Attachment 325853 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/ObjectIdentifier.h:36:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 2 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Chris Dumez 2017-11-02 22:40:17 PDT
Created attachment 325856 [details]
Patch
Comment 21 Build Bot 2017-11-02 22:42:37 PDT
Attachment 325856 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/ObjectIdentifier.h:36:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 2 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Chris Dumez 2017-11-02 22:56:39 PDT
Comment on attachment 325856 [details]
Patch

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

> Source/WTF/wtf/ObjectIdentifier.h:34
> +    ObjectIdentifier() = default;

Unfortunately, the IPC code currently requires me to have a default constructor. See error at https://pastebin.com/vNGBKWVK.
Comment 23 Brady Eidson 2017-11-03 09:21:06 PDT
Comment on attachment 325856 [details]
Patch

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

> Source/WebCore/workers/service/ServiceWorkerIdentifier.h:34
> +enum ServiceWorkerType { };

Maybe ServiceWorkerIdentifierType to reduce the possibility of collisions in the future?
Comment 24 Chris Dumez 2017-11-03 09:25:46 PDT
Created attachment 325902 [details]
Patch
Comment 25 Build Bot 2017-11-03 09:27:23 PDT
Attachment 325902 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/ObjectIdentifier.h:36:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 2 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Chris Dumez 2017-11-03 09:37:10 PDT
Committed r224403: <https://trac.webkit.org/changeset/224403>
Comment 27 Radar WebKit Bug Importer 2017-11-15 12:22:40 PST
<rdar://problem/35567415>