Bug 180911 - Add ability to API test Service Workers via a custom protocol
Summary: Add ability to API test Service Workers via a custom protocol
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-16 17:03 PST by Brady Eidson
Modified: 2017-12-18 17:18 PST (History)
12 users (show)

See Also:


Attachments
Patch (41.77 KB, patch)
2017-12-16 17:13 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (41.81 KB, patch)
2017-12-16 19:13 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (42.19 KB, patch)
2017-12-18 12:22 PST, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2017-12-16 17:03:23 PST
Add ability to API test Service Workers via a custom protocol
Comment 1 Brady Eidson 2017-12-16 17:13:13 PST
Created attachment 329586 [details]
Patch
Comment 2 Build Bot 2017-12-16 17:14:31 PST
Attachment 329586 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:81:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:107:  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:121:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:126:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:131:  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:133:  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:139:  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: 8 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Brady Eidson 2017-12-16 17:18:05 PST
> ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:107:  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]


This style rule should be changed at least for TestWebKitAPI, because such literals + custom scheme handlers are a clear win for tests.
Comment 4 Brady Eidson 2017-12-16 19:13:57 PST
Created attachment 329592 [details]
Patch
Comment 5 Build Bot 2017-12-16 19:16:08 PST
Attachment 329592 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:80:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:106:  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:120:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:122:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:125:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:130:  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:132:  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:138:  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: 8 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 youenn fablet 2017-12-17 08:46:19 PST
It is not clear to me whether the goal here is to add custom protocol service workers as we think applications will use these or if it is mainly for enabling service worker API tests. I guess it is both?

An alternative for SW API tests would be to make run-api-tests start WPT/HTTP/WS servers when a test requires it. This might also be handy in other API test areas.
Comment 7 Brady Eidson 2017-12-17 09:24:38 PST
(In reply to youenn fablet from comment #6)
> It is not clear to me whether the goal here is to add custom protocol
> service workers as we think applications will use these or if it is mainly
> for enabling service worker API tests. I guess it is both?

Yes it is both.

> An alternative for SW API tests would be to make run-api-tests start
> WPT/HTTP/WS servers when a test requires it. This might also be handy in
> other API test areas.

This is often bandied about every time somebody notices "it would be nice to have httpd during api tests" and then abandoned once somebody looks at it closer.

I'm not saying it wouldn't be great, or should not happen, but I think it's a tangent to the goal here.
Comment 8 youenn fablet 2017-12-17 10:03:48 PST
> An alternative for SW API tests would be to make run-api-tests start
> WPT/HTTP/WS servers when a test requires it. This might also be handy in
> other API test areas.

I filed bug 180915 for that purpose.
Comment 9 Chris Dumez 2017-12-18 09:36:05 PST
Comment on attachment 329592 [details]
Patch

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

> Source/WebCore/page/NavigatorServiceWorker.idl:31
> +    [SecureContextOrServiceWorkerScheme, SameObject] readonly attribute ServiceWorkerContainer serviceWorker;

This makes our IDL diverge from the spec, which is the opposite of what we're going for these days. I would be fine adding an *extra* extended Attribute to get special behavior, but I do not think it is OK to replace the standard [SecureContext] with something else.
Maybe [SecureContext, AllowServiceWorkerScheme]. Or since it is only used in one place, we could just add a special case to the bindings generator, instead of using a new IDL extended attribute.

Sam may have an opinion on this.

Or maybe simply override isSecureContext() in ServiceWorkerGlobalScope? Then we would not need any bindings change, right? Any reason we should not do that?
Comment 10 Brady Eidson 2017-12-18 09:55:20 PST
(In reply to Chris Dumez from comment #9)
> Comment on attachment 329592 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329592&action=review
> 
> > Source/WebCore/page/NavigatorServiceWorker.idl:31
> > +    [SecureContextOrServiceWorkerScheme, SameObject] readonly attribute ServiceWorkerContainer serviceWorker;
> 
> This makes our IDL diverge from the spec, which is the opposite of what
> we're going for these days. I would be fine adding an *extra* extended
> Attribute to get special behavior, but I do not think it is OK to replace
> the standard [SecureContext] with something else. Maybe [SecureContext, AllowServiceWorkerScheme].

That's the first thing I tried - The bindings generator only appends these as && clauses, so adding the second clause doesn't help when you can't pass the first clause.

> Or maybe simply override isSecureContext() in ServiceWorkerGlobalScope? Then
> we would not need any bindings change, right? Any reason we should not do
> that?

It has to pass on Documents/Workers as well.

> Or since it is only used in
> one place, we could just add a special case to the bindings generator,
> instead of using a new IDL extended attribute.

I guess I'll do this.
Comment 11 Brady Eidson 2017-12-18 12:22:40 PST
Created attachment 329659 [details]
Patch
Comment 12 Build Bot 2017-12-18 12:24:17 PST
Attachment 329659 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:80:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:106:  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:120:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:122:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:125:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:130:  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:132:  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:138:  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: 8 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 WebKit Commit Bot 2017-12-18 17:17:01 PST
Comment on attachment 329659 [details]
Patch

Clearing flags on attachment: 329659

Committed r226088: <https://trac.webkit.org/changeset/226088>
Comment 14 WebKit Commit Bot 2017-12-18 17:17:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2017-12-18 17:18:29 PST
<rdar://problem/36120769>