Bug 180911 – Add ability to API test Service Workers via a custom protocol

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brady Eidson]

Add ability to API test Service Workers via a custom protocol

[Brady Eidson]

Created attachment 329586 [details]
Patch

[EWS Watchlist]

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.

[Brady Eidson]

> 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.

[Brady Eidson]

Created attachment 329592 [details]
Patch

[EWS Watchlist]

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.

[youenn fablet]

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.

[Brady Eidson]

(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.

[youenn fablet]

> 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.

[Chris Dumez]

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?

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 329659 [details]
Patch

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 329659 [details]
Patch

Clearing flags on attachment: 329659

Committed r226088: <https://trac.webkit.org/changeset/226088>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36120769>