Bug 212939 - Swift Overlay API refinements
Summary: Swift Overlay API refinements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Savage
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-08 16:27 PDT by James Savage
Modified: 2020-06-10 12:26 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.59 KB, patch)
2020-06-08 16:50 PDT, James Savage
no flags Details | Formatted Diff | Diff
Patch (8.29 KB, patch)
2020-06-08 17:15 PDT, James Savage
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Savage 2020-06-08 16:27:17 PDT
* JavaScript completion handlers should be Optional.
* Standardize use of `completionHandler` as argument label.
* Fix missing imports in Umbrella Header, this isn't part of the overlay, but prevents use of WKScriptMessageHandlerWithReply from Swift code.
Comment 1 Radar WebKit Bug Importer 2020-06-08 16:27:42 PDT
<rdar://problem/64140013>
Comment 2 James Savage 2020-06-08 16:50:27 PDT
Created attachment 401400 [details]
Patch
Comment 3 Darin Adler 2020-06-08 17:04:59 PDT
Comment on attachment 401400 [details]
Patch

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

> Source/WebKit/Shared/API/Cocoa/WebKit.h:52
>  #import <WebKit/WKScriptMessageHandler.h>
> +#import <WebKit/WKScriptMessageHandlerWithReply.h>

Doubt we need both of these. Could remove the other one.
Comment 4 James Savage 2020-06-08 17:15:19 PDT
Created attachment 401401 [details]
Patch
Comment 5 James Savage 2020-06-08 17:17:42 PDT
Comment on attachment 401400 [details]
Patch

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

>> Source/WebKit/Shared/API/Cocoa/WebKit.h:52
>> +#import <WebKit/WKScriptMessageHandlerWithReply.h>
> 
> Doubt we need both of these. Could remove the other one.

We could move the definition of WKScriptMessageHandlerWithReply to WKScriptMessageHandler.h, but the umbrella header must import every .h in the WebKit.framework/Headers directory, otherwise it cause issues for modular imports (which are the only type available to Swift, or @import WebKit in Objective-C).
Comment 6 Darin Adler 2020-06-09 11:28:17 PDT
(In reply to James Savage from comment #5)
> the umbrella header must import every .h in
> the WebKit.framework/Headers directory

Directly, not indirectly? OK. Understood, I guess.
Comment 7 James Savage 2020-06-09 17:39:20 PDT
Yeah, there is technically another way to achieve this. I could instead modify the WebKit.modulemap file to include an explicit "header" directive for any headers not included in the umbrella, but that is not a standard practice. The documentation around this is at http://clang.llvm.org/docs/Modules.html#header-declaration. In either case transitive imports are a problem, it just moves around where the explicit declaration goes, and keeping everything in the umbrella header is much more straightforward.

If we did anything I'd suggest defining both protocols in the same WKScriptMessageHandler.h, but the two protocols don't inherit from each other, and it appears that WebKit generally avoids declaring multiple protocol or class types in the same API header.
Comment 8 EWS 2020-06-10 12:26:19 PDT
Committed r262849: <https://trac.webkit.org/changeset/262849>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401401 [details].