Bug 169037 – [WK2] Missing C API for UserContentExtensionStore prevents instantiating content extensions

• Component: WebKit2
• Status: RESOLVED DUPLICATE

[Adrian Perez]

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.

[Adrian Perez]

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.

[Adrian Perez]

Ping. Any comment regarding this? Unless stated otherwise, I'll go ahead
and upload a patch adding the proposed API in the next days.

[Adrian Perez]

Created attachment 304335 [details]
Patch

[Adrian Perez]

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?

[Michael Catanzaro]

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

[Adrian Perez]

(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

[Alex Christensen]

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?

[Adrian Perez]

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

[Alex Christensen]

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.

[Adrian Perez]

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

[Adrian Perez]

*** Bug 169607 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

Comment on attachment 304335 [details]
Patch

Every bot is red here ;)

[Adrian Perez]

I'll be handling this as part of #167941

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