Bug 178655 – [WPE] webkit_web_view_new() should enable specifying wpe_view_backend object

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Zan Dobersek]

The webkit_web_view_new() entrypoint, or any related webkit_web_view_new_with_*() entrypoint, doesn't make it possible to specify the wpe_view_backend object that should be used internally. This was perfectly possible with WKViewCreateWithViewBackend(), and that functionality should stay available with the new GLib API.

[Carlos Garcia Campos]

Right. We just need to figure out how to expose WPE lib objects in the API, because libwpe is not introspectable and it doesn't have documentation either. How does the user know which bakends are available? how to create them? who owns it once passed to yhe new function? should it be manually freed after being passed? or when the web view is destroyed? etc.

[Zan Dobersek]

(In reply to Carlos Garcia Campos from comment #1)
> Right. We just need to figure out how to expose WPE lib objects in the API,
> because libwpe is not introspectable and it doesn't have documentation
> either.

I don't think we should treat introspection support as a priority.

As far as C headers go, forward-declaring struct wpe_view_backend type is enough.

> How does the user know which bakends are available? how to create them?

That depends on what platform-specific WPEBackend implementation (which provides its own headers) the user is leveraging.

> who owns it once passed to yhe new function? should it be manually
> freed after being passed? or when the web view is destroyed? etc.

If it's passed to the constructor, ownership is not passed. If WPE::View has to load the interface-implementing object manually, then it's owned by that WPE::View object. Note this isn't currently implemented in such a manner, and should be fixed.

[Carlos Garcia Campos]

(In reply to Zan Dobersek from comment #2)
> (In reply to Carlos Garcia Campos from comment #1)
> > Right. We just need to figure out how to expose WPE lib objects in the API,
> > because libwpe is not introspectable and it doesn't have documentation
> > either.
> 
> I don't think we should treat introspection support as a priority.

It's not a priority, but we should take it into account when designing the API.

> As far as C headers go, forward-declaring struct wpe_view_backend type is
> enough.

I think public headers should include the wpe headers, except the platform-specific ones, of course.

> > How does the user know which bakends are available? how to create them?
> 
> That depends on what platform-specific WPEBackend implementation (which
> provides its own headers) the user is leveraging.
>
> > who owns it once passed to yhe new function? should it be manually
> > freed after being passed? or when the web view is destroyed? etc.
> 
> If it's passed to the constructor, ownership is not passed. If WPE::View has
> to load the interface-implementing object manually, then it's owned by that
> WPE::View object. Note this isn't currently implemented in such a manner,
> and should be fixed.

We need to clarify that. But my point in general is that if we expose wpe objects in our public API, wpe libraries should be documented too.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #3)
> We need to clarify that. But my point in general is that if we expose wpe
> objects in our public API, wpe libraries should be documented too.

gtk-doc and introspection support for WPEBackends are both high on my short-term TODO list. They go hand in hand, in fact: if the docs are done correctly, then adding introspection should be very little additional effort. It's good marketing for us, and it's also the right time to validate that our API is not going to require changes to make introspectable.

[Zan Dobersek]

(In reply to Zan Dobersek from comment #2)
> If it's passed to the constructor, ownership is not passed. If WPE::View has
> to load the interface-implementing object manually, then it's owned by that
> WPE::View object. Note this isn't currently implemented in such a manner,
> and should be fixed.

Bug #178773.

[Michael Catanzaro]

I see two options here. The simple option is to add a new new function:

WebKitWebView *webkit_web_view_new_with_backend (wpe_view_backend *backend);

and add a corresponding construct-only property, gpointer view-backend.

The alternative is to wrap wpe_view_backend in a GBoxed WebKitViewBackend. Then the new new function take a WebKitViewBackend* instead of a wpe_view_backend*, and the property would be a WebKitViewBackend* instead of a gpointer.

Either way, a new property is surely required, and that implies the WebKitWebView must own its view backend (to be discussed in bug #178773).

[Carlos Garcia Campos]

Created attachment 327156 [details]
Patch

[Carlos Garcia Campos]

Created attachment 327157 [details]
Fix GTK build

[Zan Dobersek]

LGTM.

[EWS Watchlist]

Comment on attachment 327157 [details]
Fix GTK build

Attachment 327157 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5271803

New failing tests:
http/tests/appcache/offline-access.html

[EWS Watchlist]

Created attachment 327161 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Michael Catanzaro]

Comment on attachment 327157 [details]
Fix GTK build

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

> Source/WebKit/ChangeLog:14
> +        Update all WebKitWebView constructors to receive a WebKitWebViewBackend as argument. It's now required to
> +        provide a backend to create a web view, but it can be NULL to use the default one. WebKitWebViewBackend is a
> +        boxed type wrapping a struct wpe_view_backend* used as construct only property of WebKitWebView. The view always
> +        takes the ownership of the WebKitWebViewBackend which owns the struct wpe_view_backend*. An optional
> +        GDestroyNotify and user data pointer can be passed to the WebKitWebViewBackend constructor to provide a custom
> +        deleter for the backend. In the C API the struct wpe_view_backend* is also mandatory now, but it can't be NULL
> +        and it's owned by the caller, not the view.

Excellent plan.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:854
> +     * The #WebKoitWebViewBackend of the view.

Koit!

> Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:232
> -webkit_web_view_new                                  (void);
> +webkit_web_view_new                                  (WebKitWebViewBackend      *backend);

I'm OK with doing this.

But I think I'd prefer to add a new webkit_web_view_new_with_backend() constructor, and not add WebKitWebViewBackend parameters to all the other constructors. As long as the backend is optional, I don't see the point in making it special. And these constructors are pretty much never going to be sufficient for constructing a WebKitWebView, because you're almost always going to need to use g_object_new() anyway.

But this is fine too... either way, it does not matter very much.

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.cpp:54
> +    _WebKitWebViewBackend()
> +        : backend(wpe_view_backend_create())
> +        , notifyCallback(reinterpret_cast<GDestroyNotify>(wpe_view_backend_destroy))
> +        , notifyCallbackData(backend)
> +    {
> +    }

I think this constructor is unused, right? You should delete it.

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.cpp:96
> + * @user_data: user data to pass to @notify

Does user_data not need to be marked (nullable) as well? I guess that's implied?

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.cpp:101
> + * The returned #WebKitWebViewBackend should never be freed by the user, it must be passed to a

It's a comma splice. Change the comma to a semicolon, or split it into two sentences.

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.h:38
> +webkit_web_view_backend_get_type  (void);

(void) not aligned with the other function calls' parameters.

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.h:44
> +WEBKIT_API struct wpe_view_backend*

Missing space:

wpe_view_backend *

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:131
> +    g_assert(!wpeBackend);

Maybe it was obvious to you, but I think this check was clever!

[Carlos Garcia Campos]

Comment on attachment 327157 [details]
Fix GTK build

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

>> Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:232
>> +webkit_web_view_new                                  (WebKitWebViewBackend      *backend);
> 
> I'm OK with doing this.
> 
> But I think I'd prefer to add a new webkit_web_view_new_with_backend() constructor, and not add WebKitWebViewBackend parameters to all the other constructors. As long as the backend is optional, I don't see the point in making it special. And these constructors are pretty much never going to be sufficient for constructing a WebKitWebView, because you're almost always going to need to use g_object_new() anyway.
> 
> But this is fine too... either way, it does not matter very much.

The thing is that the backend is not optional, but you can pass null. I know, in practice it's the same, but that's why I added it explicitly, to make it clear that it's not optional. I also considered to add a webkit_web_view_backend_new_default() and not allow null in constructors, but it not as convenient as allowing null.

>> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.cpp:54
>> +    }
> 
> I think this constructor is unused, right? You should delete it.

See webkitWebViewBackendCreateDefault() below.

>> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.cpp:96
>> + * @user_data: user data to pass to @notify
> 
> Does user_data not need to be marked (nullable) as well? I guess that's implied?

I guess it's implied because I've never seen nullable in a user data parameter.

>> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.cpp:101
>> + * The returned #WebKitWebViewBackend should never be freed by the user, it must be passed to a
> 
> It's a comma splice. Change the comma to a semicolon, or split it into two sentences.

Ok.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #13)
> The thing is that the backend is not optional, but you can pass null. I
> know, in practice it's the same, but that's why I added it explicitly, to
> make it clear that it's not optional. I also considered to add a
> webkit_web_view_backend_new_default() and not allow null in constructors,
> but it not as convenient as allowing null.

I don't understand... allowing NULL means it is most definitely optional.

Anyway, it's fine.

> >> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.cpp:54
> >> +    }
> > 
> > I think this constructor is unused, right? You should delete it.
> 
> See webkitWebViewBackendCreateDefault() below.

Ah, OK.

[Carlos Garcia Campos]

Committed r225044: <https://trac.webkit.org/changeset/225044>