Bug 169037

Summary: [WK2] Missing C API for UserContentExtensionStore prevents instantiating content extensions
Product: WebKit Reporter: Adrian Perez <aperez>
Component: WebKit2Assignee: 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 Flags
Patch mcatanzaro: review-

Description Adrian Perez 2017-03-01 07:41:00 PST
The WebKit2 C API does not provide a way to instantiate an UserContentExtensionStore,
nor provides any other means of compiling content extensions.

Having this would allow using the C API in WKTR to handle tests which make use of
content extensions. In turn, this would make it unnecessary to use the Cocoa API
in WKTR, and by using the C API it would be trivial for the GTK+ port to unskip
the content extensions layout tests.
Comment 1 Adrian Perez 2017-03-01 07:50:18 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.
Comment 2 Adrian Perez 2017-03-13 15:00:19 PDT
Ping. Any comment regarding this? Unless stated otherwise, I'll go ahead
and upload a patch adding the proposed API in the next days.
Comment 3 Adrian Perez 2017-03-13 19:19:36 PDT
Created attachment 304335 [details]
Patch
Comment 4 Adrian Perez 2017-03-13 19:29:58 PDT
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?
Comment 5 Michael Catanzaro 2017-03-13 20:33:47 PDT
(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.
Comment 6 Adrian Perez 2017-03-14 08:19:25 PDT
(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 7 Alex Christensen 2017-03-14 14:48:01 PDT
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?
Comment 8 Adrian Perez 2017-03-14 15:32:15 PDT
(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.
Comment 9 Alex Christensen 2017-03-14 17:25:17 PDT
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.
Comment 10 Adrian Perez 2017-03-14 18:09:24 PDT
(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?
Comment 11 Adrian Perez 2017-03-14 18:11:17 PDT
*** Bug 169607 has been marked as a duplicate of this bug. ***
Comment 12 Michael Catanzaro 2018-01-15 12:54:54 PST
Comment on attachment 304335 [details]
Patch

Every bot is red here ;)
Comment 13 Adrian Perez 2018-12-13 00:12:40 PST
I'll be handling this as part of #167941

*** This bug has been marked as a duplicate of bug 167941 ***