Summary: | [WK2] Missing C API for UserContentExtensionStore prevents instantiating content extensions | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adrian Perez <aperez> | ||||
Component: | WebKit2 | Assignee: | Adrian Perez <aperez> | ||||
Status: | RESOLVED DUPLICATE | ||||||
Severity: | Normal | CC: | achristensen, beidson, bugs-noreply, mcatanzaro, sam, ysuzuki, zan | ||||
Priority: | P2 | ||||||
Version: | Other | ||||||
Hardware: | Unspecified | ||||||
OS: | All | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 154553, 169607 | ||||||
Attachments: |
|
Description
Adrian Perez
2017-03-01 07:41:00 PST
I have already a WIP patch for this, as a side effect of working on bug #167941 and wanting to unskip the layout tests in the GTK+ port. Once I figure out a couple of rough edges and can run the tests I will upload the patch. The API bits being added would be: // Source/WebKit2/UIProcess/API/C/WKUserContentExtensionStoreRef.h WK_EXPORT WKUserContentExtensionStoreRef WKUserContentExtensionStoreCreate(WKStringRef path); enum { kWKUserContentExtensionStoreSuccess = 0, kWKUserContentExtensionStoreLookupFailed, kWKUserContentExtensionStoreVersionMismatch, kWKUserContentExtensionStoreCompileFailed, kWKUserContentExtensionStoreRemoveFailed, }; typedef uint32_t WKUserContentExtensionStoreResult; typedef void (*WKUserContentExtensionStoreFilterFunction)(WKUserContentExtensionStoreRef, WKUserContentFilterRef, WKUserContentExtensionStoreResult, void*); void WKUserContentExtensionStoreCompileFilter(WKUserContentExtensionStoreRef, WKStringRef identifier, WKStringRef jsonSource, void* context, WKUserContentExtensionStoreFilterFunction callback); void WKUserContentExtensionStoreLookupFilter(WKUserContentExtensionStoreRef, WKStringRef identifier, void* context, WKUserContentExtensionStoreFilterFunction callback); void WKUserContentExtensionStoreRemoveFilter(WKUserContentExtensionStoreRef, WKStringRef identifier, void* context, WKUserContentExtensionStoreFilterFunction callback); The order of parameters follows the underlying API::UserContentExtensionsStore methods, and where a C++ lambda is accepted, a callback function is used instead. As in other functions in the C API which take callbacks, the “context” parameter (a “void*” passed back to the callback) appears in the signature _before_ the “callback” parameter. Ping. Any comment regarding this? Unless stated otherwise, I'll go ahead and upload a patch adding the proposed API in the next days. Created attachment 304335 [details]
Patch
I have uploaded the patch in its current state to gather some feedback. In particular, I have two questions/doubts: - How would one go about adding unit tests for this? There is no “Tools/TestWebKitAPI/Tests/WebKit2C/*” or similar. - Should the patch include also a function “WKUserContentExtensionStoreGetDefaultUserContentExtensionStore()”? - With this API added, the usage of the Objective-C API in WKTR to test content extensions could be dropped. Should that be included here, or in a follow-up patch/bug? (In reply to comment #4) > I have uploaded the patch in its current state to gather some feedback. > In particular, I have two questions/doubts: > > - How would one go about adding unit tests for this? There is no > “Tools/TestWebKitAPI/Tests/WebKit2C/*” or similar. Put them in Tools/TestWebKitAPI/Tests/WebKit2/. > - Should the patch include also a function > “WKUserContentExtensionStoreGetDefaultUserContentExtensionStore()”? Probably. Why not? > - With this API added, the usage of the Objective-C API in WKTR to test > content extensions could be dropped. Should that be included here, or > in a follow-up patch/bug? I think either approach is fine, but my preference is for two different patches. (In reply to comment #5) > (In reply to comment #4) > > I have uploaded the patch in its current state to gather some feedback. > > In particular, I have two questions/doubts: > > > > - How would one go about adding unit tests for this? There is no > > “Tools/TestWebKitAPI/Tests/WebKit2C/*” or similar. Oh, somehow I didn't check this one directory. Thanks! > Put them in Tools/TestWebKitAPI/Tests/WebKit2/. > > > - Should the patch include also a function > > “WKUserContentExtensionStoreGetDefaultUserContentExtensionStore()”? > > Probably. Why not? The idea so far is that the GTK+ port will require specifying always the path, but given that the C API can be used by other ports, it is probably better to add it as well. > > - With this API added, the usage of the Objective-C API in WKTR to test > > content extensions could be dropped. Should that be included here, or > > in a follow-up patch/bug? > > I think either approach is fine, but my preference is for two different > patches. I also prefer to handle it separately. It is now filed as bug #169607 Comment on attachment 304335 [details]
Patch
I assume this is for WebKitTestRunner, right? Could this be adopted in the same patch?
Is there an intent to add a gtk-specific C API corresponding to this?
(In reply to comment #7) > Comment on attachment 304335 [details] > Patch > > I assume this is for WebKitTestRunner, right? Yes, I tend to abbreviate WebKitTestRunner == WKTR. O:-) > [...] Could this be adopted in the same patch? Our plan was to do the WebKitTestRunner part as bug #169607, as it seemed easier for review smaller but I do not have a strong opinion for doing two separate patches/bugs. I'll add the WebKitTestRunner changes together with this, hopefully that would ease the process. > Is there an intent to add a gtk-specific C API corresponding to this? Absolutely. That is being tracked as bug #169037. There's a preview patch there with a tentative API. Our plan is to add the C API and put it to use in WebKitTestRunner first; then try using the proposed GTK+ API in Epiphany, see how it feels and improve it if needed, and then land the new GTK+ API. I think these changes and WKTR changes should be in the same patch so we can see why we are making the changes and that they actually work. Sometimes with callbacks that need to capture things like lambda captures we use a listener object in the C API. I'm not sure if you'll want to do that here. Right now we probably don't need state, but it would probably be useful to know at least which request to compile a content extension you are responding to. For example, if you initiate the compiling of two different content extensions, then with your proposed API the completion handler wouldn't be able to know which content extension compiling just completed at runtime. (In reply to comment #9) > I think these changes and WKTR changes should be in the same patch so we can > see why we are making the changes and that they actually work. Let's do it that way, then. > Sometimes with callbacks that need to capture things like lambda captures we > use a listener object in the C API. I'm not sure if you'll want to do that > here. Right now we probably don't need state, but it would probably be > useful to know at least which request to compile a content extension you are > responding to. For example, if you initiate the compiling of two different > content extensions, then with your proposed API the completion handler > wouldn't be able to know which content extension compiling just completed at > runtime. I did briefly considered adding a listener, but it seemed a bit too heavy in this case. As for knowing which request has completed, one could save the identifier string into a “struct” passed as the “context” pointer that gets passed back to the completion callback... But even when that's possible, I have to agree that it is far from having good ergonomics. Probably passing back the identifier string to the completion callback is a very reasonable option, using the following signature for callback the completion callback functions: typedef void (*WKUserContentExtensionStoreFilterFunction)(WKUserContentExtensionStoreRef, WKStringRef identifier, WKUserContentFilterRef, WKUserContentExtensionStoreResult, void*); The “identifier” argument passed to callbacks would always be non-NULL. How does this sound? *** Bug 169607 has been marked as a duplicate of this bug. *** Comment on attachment 304335 [details]
Patch
Every bot is red here ;)
I'll be handling this as part of #167941 *** This bug has been marked as a duplicate of bug 167941 *** |